Page MenuHomePhabricator

[ASTImporter] Support functions with placeholder return types ...
ClosedPublic

Authored by martong on Nov 28 2019, 6:12 AM.

Details

Summary

Support functions with placeholder return types even in cases when the type is
declared in the body of the function.
Example: auto f() { struct X{}; return X(); }

Currently, the import of such functions result in an infinite recursion.
The fix is that we import a simplified type for the function and once the FunctionDecl and the body is created, then we import and set the original type.

Diff Detail

Event Timeline

martong created this revision.Nov 28 2019, 6:12 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
martong edited the summary of this revision. (Show Details)Nov 28 2019, 8:00 AM
martong edited the summary of this revision. (Show Details)

Hello Gabor,
That's an interesting case, thank for fixing this!

clang/lib/AST/ASTImporter.cpp
3019

ASTImporter is not very const-friendly, but this function is pure, so I think it's better to const-qualify the parameters.

3031

Is it possible for getAs() to return nullptr at this point?

3186

If FromFPT is not used outside of the condition, we can move the initialization inside if().

clang/unittests/AST/ASTImporterTest.cpp
5609

Nit: a space before '{'.

5625

That's interesting. Have you tried '-frewrite-includes' for expanding inclusions only without macro expansion?

martong marked 9 inline comments as done.Dec 2 2019, 9:02 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
3019

Ok, added the const qualifiers.

3031

No. So, I added an assertion to reflect this.

3186

Ok, I moved it inside the if.

clang/unittests/AST/ASTImporterTest.cpp
5625

I hadn't tried it before, but I've just done that now. Unfortunately, the AST produced with -E -frewrite-includes produces a type for the operator() that already has a canonical type: long (long) const

) ./bin/clang-check -p cxx_cmd_json ~w/llvm3/git/llvm-project/libcxx/src/filesystem/directory_iterator.cpp -ast-dump -ast-dump-filter "posix_utimes" | ag "CXXMethodDecl.*int_type"                                           --- COMMAND ---
  |           | |-CXXMethodDecl 0x3007ec0 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x30131f0 <col:18, line:400:3> line:396:18 implicit __invoke 'int_type (long)' static inline
  |           | |-CXXMethodDecl 0x2ddfaf0 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x2deae20 <col:18, line:400:3> line:396:18 implicit __invoke 'int_type (long)' static inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang-check -p cxx_cmd_json ~w/llvm3/git/llvm-project/libcxx/src/filesystem/directory_iterator.cpp -ast-dump -ast-dump-filter "posix_utimes" | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x2833020 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x287fa20 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang -x c++ directory_iterator.cpp.E.orig -fsyntax-only -Xclang -ast-dump -Xclang -ast-dump-filter -Xclang posix_utimes | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x31c6ad0 <col:30, line:400:3> line:396:18 used operator() 'long (long) const' inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang -x c++ directory_iterator.cpp.E.ri -fsyntax-only -Xclang -ast-dump -Xclang -ast-dump-filter -Xclang posix_utimes | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x25ad7f0 <col:30, line:400:3> line:396:18 used operator() 'long (long) const' inline
martong updated this revision to Diff 231725.Dec 2 2019, 9:02 AM
martong marked 4 inline comments as done.
  • Address Alexei's comments

Alexei, thanks for the assiduous review!

a_sidorin accepted this revision.Dec 7 2019, 4:30 AM

LGTM, thanks!
Let's wait for Shafik's comments before committing this.

clang/lib/AST/ASTImporter.cpp
48

Do we use this new include?

This revision is now accepted and ready to land.Dec 7 2019, 4:30 AM
martong marked an inline comment as done.
martong added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5625

So, this means that the type information is transformed during the include expansion. That is very strange.
I think, I'll have an email to cfe-dev, maybe others have some insights on this.

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

shafik accepted this revision.Dec 10 2019, 10:57 AM

Besides my comments about different scenarios this LGTM

clang/lib/AST/ASTImporter.cpp
3029

I added a comment with several examples of C++ code that I am not sure if this will catch or not. Since C++17 we allow types to be declared in several more places.

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Thanks for these cases! I am going to write unit tests for these as well.

martong marked 2 inline comments as done.Dec 12 2019, 8:13 AM

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Thanks for these cases! I am going to write unit tests for these as well.

So I checked and wrote a test case for all of these. All of them passed!

clang/lib/AST/ASTImporter.cpp
48

Thanks for catching this! I used clangd this time and that added this include, I am not sure why, anyway I set up clangd to do not add includes.

This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.