[ASTImporter] Support TypeTraitExpr Importing

[tk1012]

Summary

This fixes unsupporting of importing TypeTraitExpr in ASTImporter.
TypeTraitExpr is caused by "__builtin_types_compatible_p()" that is usually used in the assertion in C code.
For example, PostgreSQL uses the builtin function in its assertion, and ASTImporter currently fails to import the assertion because of diag::err_unsupported_ast_node.

[a.sidorin]

Hello Takafumi,

Thank you for this patch! I feel positive about it. You can find my comments inline.

lib/AST/ASTImporter.cpp

5540 ↗

(On Diff #121853)

We should fail (return nullptr) if import fails and ToTI is nullptr.

5543 ↗

(On Diff #121853)

No need to create an ArrayRef - SmallVector can be implicitly converted to it so you can just pass SmallVector to the function.

5545 ↗

(On Diff #121853)

The style looks a bit broken. Could you clang-format?

5551 ↗

(On Diff #121853)

This will assert if E is value-dependent. You need to check it explicitly.

[tk1012]

Hello, Aleksei.

I'm sorry for the long response time.
I update the diff to follow your comments.

Updates

1. I apply clang-format -style=llvm to ASTMatchers.h and ASTImporter.cpp. I don't apply it to ASTImporterTest.cpp for readability of the matching types.
2. I add return nullptr; if ToTI is nullptr.
3. I directly pass ToArgVec to TypeTraitExpr::Create()
4. I add a check for the value-dependent of `E'.

[tk1012]

[tk1012]

Description of the Sema::BuildTypeTrait()

lib/AST/ASTImporter.cpp

5631 ↗

(On Diff #123539)

According to Sema::BuildTypeTrait() in lib/Sema/SemaExprCXX.cpp,
if an argument of TypeTraitExpr has a dependent type,
TypeTraitExpr's value (Result) is always false.

ExprResult Sema::BuildTypeTrait(TypeTrait Kind, SourceLocation KWLoc,
....
 bool Dependent = false;
 for (unsigned I = 0, N = Args.size(); I != N; ++I) {
   if (Args[I]->getType()->isDependentType()) {
     Dependent = true;
     break;
   }
 }

 bool Result = false;
 if (!Dependent)
   Result = evaluateTypeTrait(*this, Kind, KWLoc, Args, RParenLoc);

 return TypeTraitExpr::Create(Context, ResultType, KWLoc, Kind, Args,
                              RParenLoc, Result);
...

[a.sidorin]

LGTM, thank you!

[a.sidorin]

Hello Aaron,

This patch is OK for me but could you please take a look at ASTMatchers changes?

[xazax.hun]

The checker documentation should be updated as well.

include/clang/ASTMatchers/ASTMatchers.h

2251 ↗

(On Diff #123539)

Note that the matchers are no longer defined in the header. They are just declared and the definition is moved to a cpp file.

[tk1012]

Hello Gábor,

Thank you for responding.

I move the definition of the mather typeTraitExpr into unittests/AST/ASTImporterTest.cpp.
I also slightly modify the test code.

[aaron.ballman]

lib/AST/ASTImporter.cpp

5622 ↗

(On Diff #123792)

const auto *?

5631 ↗

(On Diff #123792)

Remove spurious parens.

unittests/AST/ASTImporterTest.cpp

548 ↗

(On Diff #123792)

Please add a value-dependent test as well.

[a.sidorin]

lib/AST/ASTImporter.cpp

5622 ↗

(On Diff #123792)

By the way, you can replace the loop with ImportContainerChecked().

[tk1012]

Hello there,

I update the diff, reflecting the comments.

Updates:

1. Use ImportContainerChecked() for importing TypeSourceInfo.
2. Modify bool ToValue = (...) ? ... : false;.
3. Add a test case for the value-dependent TypeTraitExpr.

[a.sidorin]

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

1. Rebase your patch onto latest master
2. Launch script docs/tools/dump_ast_matchers.py
3. Add changes made by this script to your commit.

After these changes are made, I will approve the patch. Thank you!

unittests/AST/ASTImporterTest.cpp

554 ↗

(On Diff #124057)

Please add void f() { declToImport<int>().m(); } after declToImport` definition. The reason is that in MSVC mode, uninstantiated templates are ignored so the test will fail. We need this to avoid this.

[aaron.ballman]

In D39722#933699, @a.sidorin wrote:

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

1. Rebase your patch onto latest master
2. Launch script docs/tools/dump_ast_matchers.py
3. Add changes made by this script to your commit.

I'm a bit confused -- I don't see any ASTMatcher changes as part of this patch aside from what's in the test file. Are you suggesting the matcher there should be hoisted as a separate patch?

[xazax.hun]

In D39722#933828, @aaron.ballman wrote:

In D39722#933699, @a.sidorin wrote:

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

1. Rebase your patch onto latest master
2. Launch script docs/tools/dump_ast_matchers.py
3. Add changes made by this script to your commit.

I'm a bit confused -- I don't see any ASTMatcher changes as part of this patch aside from what's in the test file. Are you suggesting the matcher there should be hoisted as a separate patch?

In a previous version the matcher was public. I think it either should be public with documentation or it can remain in the test file and no doc update required. I am fine with both solution.

[a.sidorin]

Oh, sorry! I was thinking that a matcher is still in ASTMatchers.h (in previous revisions it was there). If matcher is internal (current revision), there is no need to update docs.

[aaron.ballman]

In D39722#933838, @xazax.hun wrote:

In D39722#933828, @aaron.ballman wrote:

In D39722#933699, @a.sidorin wrote:

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

1. Rebase your patch onto latest master
2. Launch script docs/tools/dump_ast_matchers.py
3. Add changes made by this script to your commit.

I'm a bit confused -- I don't see any ASTMatcher changes as part of this patch aside from what's in the test file. Are you suggesting the matcher there should be hoisted as a separate patch?

In a previous version the matcher was public. I think it either should be public with documentation or it can remain in the test file and no doc update required. I am fine with both solution.

I'm fine with either exposing it as a public matcher or leaving it private for the test file, but if it gets exposed, I think it should be in a separate patch.

[tk1012]

Hello there,

I update the diff and add void f() { declToImport<int>().m(); } after declToImport definition.
I leave the matcher for private in the test file, so I don't update the documentation.

[aaron.ballman]

unittests/AST/ASTImporterTest.cpp

553 ↗

(On Diff #124124)

Drop the spurious semicolon at the end of the function definition.

566 ↗

(On Diff #124124)

Is this type actually correct for C++? I would expect that for C code, but not for C++.

[tk1012]

Hello Aaron,

I remove the semicolon.

Is this type actually correct for C++?

Yes, it is.
clang generates the AST for declToImport struct like this.

|-CXXRecordDecl 0x8b19fe0 <col:22, col:29> col:29 implicit struct declToImport
`-CXXMethodDecl 0x8b1a0d0 <line:2:3, col:27> col:8 m 'void (void)'
  `-CompoundStmt 0x8b1a1e8 <col:12, col:27>
    `-TypeTraitExpr 0x8b1a1c8 <col:14, col:24> '_Bool'

[aaron.ballman]

In D39722#934781, @tk1012 wrote:

Hello Aaron,

I remove the semicolon.

Is this type actually correct for C++?

Yes, it is.
clang generates the AST for declToImport struct like this.

|-CXXRecordDecl 0x8b19fe0 <col:22, col:29> col:29 implicit struct declToImport
`-CXXMethodDecl 0x8b1a0d0 <line:2:3, col:27> col:8 m 'void (void)'
  `-CompoundStmt 0x8b1a1e8 <col:12, col:27>
    `-TypeTraitExpr 0x8b1a1c8 <col:14, col:24> '_Bool'

Hmm, that looks like a bug to me; but not one that should impact this review.

LGTM!

[aaron.ballman]

In D39722#934783, @aaron.ballman wrote:

In D39722#934781, @tk1012 wrote:

Hello Aaron,

I remove the semicolon.

Is this type actually correct for C++?

Yes, it is.
clang generates the AST for declToImport struct like this.

|-CXXRecordDecl 0x8b19fe0 <col:22, col:29> col:29 implicit struct declToImport
`-CXXMethodDecl 0x8b1a0d0 <line:2:3, col:27> col:8 m 'void (void)'
  `-CompoundStmt 0x8b1a1e8 <col:12, col:27>
    `-TypeTraitExpr 0x8b1a1c8 <col:14, col:24> '_Bool'

Hmm, that looks like a bug to me; but not one that should impact this review.

Yeah, it seems as though the ASTDumper does not receive a printing policy, and the default behavior of QualType::getAsString() is to gin up a very dumb printing policy from default language options, hence the poor quality of the name here.

[a.sidorin]

LGTM, thank you!

unittests/AST/ASTImporterTest.cpp

566 ↗

(On Diff #124124)

hasType(booleanType())?

[tk1012]

Hello there,

I update the diff to follow your comments.

[a.sidorin]

Thank you!

cfe/trunk/lib/AST/ASTImporter.cpp

public:
     Expr *VisitMemberExpr(MemberExpr *E);
     Expr *VisitCallExpr(CallExpr *E);
     Expr *VisitInitListExpr(InitListExpr *E);
     Expr *VisitArrayInitLoopExpr(ArrayInitLoopExpr *E);
     Expr *VisitArrayInitIndexExpr(ArrayInitIndexExpr *E);
     Expr *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E);
     Expr *VisitCXXNamedCastExpr(CXXNamedCastExpr *E);
     Expr *VisitSubstNonTypeTemplateParmExpr(SubstNonTypeTemplateParmExpr *E);
+    Expr *VisitTypeTraitExpr(TypeTraitExpr *E);
 
 
     template<typename IIter, typename OIter>
     void ImportArray(IIter Ibegin, IIter Iend, OIter Obegin) {
       typedef typename std::remove_reference<decltype(*Obegin)>::type ItemT;
       ASTImporter &ImporterRef = Importer;
       std::transform(Ibegin, Iend, Obegin,
                      [&ImporterRef](ItemT From) -> ItemT {
Expr *ASTNodeImporter::VisitSubstNonTypeTemplateParmExpr(
   if (!Replacement)
     return nullptr;
 
   return new (Importer.getToContext()) SubstNonTypeTemplateParmExpr(
         T, E->getValueKind(), Importer.Import(E->getExprLoc()), Param,
         Replacement);
 }
 
+Expr *ASTNodeImporter::VisitTypeTraitExpr(TypeTraitExpr *E) {
+  QualType ToType = Importer.Import(E->getType());
+  if (ToType.isNull())
+    return nullptr;
+
+  SmallVector<TypeSourceInfo *, 4> ToArgs(E->getNumArgs());
+  if (ImportContainerChecked(E->getArgs(), ToArgs))
+    return nullptr;
+
+  // According to Sema::BuildTypeTrait(), if E is value-dependent,
+  // Value is always false.
+  bool ToValue = false;
+  if (!E->isValueDependent())
+    ToValue = E->getValue();
+
+  return TypeTraitExpr::Create(
+      Importer.getToContext(), ToType, Importer.Import(E->getLocStart()),
+      E->getTrait(), ToArgs, Importer.Import(E->getLocEnd()), ToValue);
+}
+
 void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
                                       CXXMethodDecl *FromMethod) {
   for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
     ToMethod->addOverriddenMethod(
       cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(
                                             FromOverriddenMethod))));
 }
 

cfe/trunk/unittests/AST/ASTImporterTest.cpp

EXPECT_TRUE(testImport("template <typename... Args>"
                                has(
                                  returnStmt(
                                    has(
                                      implicitCastExpr(
                                        has(
                                          declRefExpr()))))))))));
 }
 
+/// \brief Matches __builtin_types_compatible_p:
+/// GNU extension to check equivalent types
+/// Given
+/// \code
+///   __builtin_types_compatible_p(int, int)
+/// \endcode
+//  will generate TypeTraitExpr <...> 'int'
+const internal::VariadicDynCastAllOfMatcher<Stmt, TypeTraitExpr> typeTraitExpr;
+
+TEST(ImportExpr, ImportTypeTraitExpr) {
+  MatchVerifier<Decl> Verifier;
+  EXPECT_TRUE(testImport("void declToImport() { "
+                         "  __builtin_types_compatible_p(int, int);"
+                         "}",
+                         Lang_C, "", Lang_C, Verifier,
+                         functionDecl(
+                           hasBody(
+                             compoundStmt(
+                               has(
+                                 typeTraitExpr(hasType(asString("int")))))))));
+}
+
+TEST(ImportExpr, ImportTypeTraitExprValDep) {
+  MatchVerifier<Decl> Verifier;
+  EXPECT_TRUE(testImport("template<typename T> struct declToImport {"
+                         "  void m() { __is_pod(T); }"
+                         "};"
+                         "void f() { declToImport<int>().m(); }",
+                         Lang_CXX11, "", Lang_CXX11, Verifier,
+                         classTemplateDecl(
+                           has(
+                             cxxRecordDecl(
+                               has(
+                                 functionDecl(
+                                   hasBody(
+                                     compoundStmt(
+                                       has(
+                                         typeTraitExpr(
+                                           hasType(booleanType())
+                                           )))))))))));
+}
 
 } // end namespace ast_matchers
 } // end namespace clang