[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

I've removed the offending test (which in this case is the correct fix - it is 
not a temporary fix).

@RKSimon  Is there some way I can see what is causing the failures on the other 
platforms? Was it because of this test? How can I monitor this? I knew how to 
see it in lab.llvm.org, but it seems things are missing or moved in the new 
buildbot interface. I otherwise have no way of knowing what problems were 
caused by this, and whether a new version of this patch fixes the problems?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added subscribers: stephenkelly, RKSimon.
RKSimon added a comment.

@stephenkelly This is breaking a large number of bots - please can you revert 
your patch series to get things green again?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Hi. I suspect it might be this patch or D90984 
 that might be leading to the test failure 
we're seeing on our arm64 builders

  FAIL: Clang-Unit :: 
AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit (15871 of 26886)
   TEST 'Clang-Unit :: 
AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit' FAILED 

  Note: Google Test filter = Traverse.IgnoreUnlessSpelledInSourceImplicit
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from Traverse
  [ RUN  ] Traverse.IgnoreUnlessSpelledInSourceImplicit
  clang/unittests/AST/ASTTraverserTest.cpp:1114: Failure
Expected: dumpASTString(TK_AsIs, TUDecl)
Which is: "\nTranslationUnitDecl\n|-TypedefDecl '__int128_t'\n| ...

Would you mind taking a look? Thanks.

Builder link: 
https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8863364626069720736


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(usual procedure is to revert while investigating test failures instead of 
commenting out the tests)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG246b428fb3b5: [AST] Ignore implicit nodes in 
IgnoreUnlessSpelledInSource mode (authored by stephenkelly).

Changed prior to commit:
  https://reviews.llvm.org/D90982?vs=305767=305812#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,179 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_ConstructorDecl) {
+
+  std::string OtherStructPrefix = R"cpp(
+struct Other {
+)cpp";
+  std::string OtherStructSuffix = "};";
+
+  std::string CopyableStructName = "struct Copyable";
+  std::string BrokenStructName = "struct explicit Copyable";
+
+  std::string CodeSuffix = R"cpp(
+{
+Other m_i;
+Copyable();
+};
+)cpp";
+
+  std::string CopyCtor = "Other(const Other&) = default;";
+  std::string ExplicitCopyCtor = "explicit Other(const Other&) = default;";
+  std::string BrokenExplicitCopyCtor =
+  "explicit explicit explicit Other(const Other&) = default;";
+
+  std::string RewriteInput = OtherStructPrefix + CopyCtor + OtherStructSuffix +
+ CopyableStructName + CodeSuffix;
+  std::string ExpectedRewriteOutput = OtherStructPrefix + ExplicitCopyCtor +
+  OtherStructSuffix + CopyableStructName +
+  CodeSuffix;
+  std::string BrokenRewriteOutput = OtherStructPrefix + BrokenExplicitCopyCtor +
+OtherStructSuffix + BrokenStructName +
+CodeSuffix;
+
+  auto MatchedRecord =
+  cxxConstructorDecl(isCopyConstructor()).bind("copyConstructor");
+
+  auto RewriteRule =
+  changeTo(before(node("copyConstructor")), cat("explicit "));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return 

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you!




Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:164
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode && !Finder->isTraversalAsIs())
+  if (LambdaNode && Finder->isTraversalIgnoringImplicitNodes())
 StmtToTraverse = LambdaNode;

Good catch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 305767.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,179 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_ConstructorDecl) {
+
+  std::string OtherStructPrefix = R"cpp(
+struct Other {
+)cpp";
+  std::string OtherStructSuffix = "};";
+
+  std::string CopyableStructName = "struct Copyable";
+  std::string BrokenStructName = "struct explicit Copyable";
+
+  std::string CodeSuffix = R"cpp(
+{
+Other m_i;
+Copyable();
+};
+)cpp";
+
+  std::string CopyCtor = "Other(const Other&) = default;";
+  std::string ExplicitCopyCtor = "explicit Other(const Other&) = default;";
+  std::string BrokenExplicitCopyCtor =
+  "explicit explicit explicit Other(const Other&) = default;";
+
+  std::string RewriteInput = OtherStructPrefix + CopyCtor + OtherStructSuffix +
+ CopyableStructName + CodeSuffix;
+  std::string ExpectedRewriteOutput = OtherStructPrefix + ExplicitCopyCtor +
+  OtherStructSuffix + CopyableStructName +
+  CodeSuffix;
+  std::string BrokenRewriteOutput = OtherStructPrefix + BrokenExplicitCopyCtor +
+OtherStructSuffix + BrokenStructName +
+CodeSuffix;
+
+  auto MatchedRecord =
+  cxxConstructorDecl(isCopyConstructor()).bind("copyConstructor");
+
+  auto RewriteRule =
+  changeTo(before(node("copyConstructor")), cat("explicit "));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray const&) = default;
+};

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this, I think it's mostly looking good!




Comment at: clang/include/clang/AST/ASTNodeTraverser.h:88
   void Visit(const Decl *D) {
+if (Traversal != TK_AsIs && D->isImplicit())
+  return;

Similar to the feedback on D90984, I think this will do the wrong thing for the 
weird traversal mode for ignoring parens and implicit casts. It should probably 
be checking for `Traversal == TK_IgnoreUnlessSpelledInSource` here.



Comment at: clang/include/clang/AST/ASTNodeTraverser.h:192
   void Visit(const CXXCtorInitializer *Init) {
+if (Traversal != TK_AsIs && !Init->isWritten())
+  return;

Same issue here.



Comment at: clang/include/clang/AST/ASTNodeTraverser.h:410
 
+if (Traversal != TK_AsIs && D->isDefaulted())
+  return;

Same here. I'll stop calling these out -- can you double check all the uses of 
`Traversal != TK_AsIs`?



Comment at: clang/include/clang/AST/ASTNodeTraverser.h:733
+  void VisitCXXForRangeStmt(const CXXForRangeStmt *Node) {
+if (Traversal != TK_AsIs) {
+  Visit(Node->getInit());

If we're in AsIs mode, don't we still want to match on some parts of the 
range-based for loop because they are spelled in source (like the body of the 
loop or the range-expression, etc)? 

The test behavior looks reasonable around this, so I suspect I'm just 
misunderstanding this change.



Comment at: clang/include/clang/AST/ASTNodeTraverser.h:742
+  void VisitCallExpr(const CallExpr *Node) {
+for (auto Child :
+ make_filter_range(Node->children(), [this](const Stmt *Child) {

This be `const auto *` or at least `auto *`.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:152
+
+if (DeclNode && DeclNode->isImplicit() && !Finder->isTraversalAsIs())
+  return baseTraverse(*DeclNode);

Similar issue here about traversal mode.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1083
+
+  auto ScopedTraversal =
+  TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();

Spelling this as `bool` would not be amiss (same below, given how trivial it is 
to spell the type out).



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1108
   }
+  auto ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
+ TraversingASTChildrenNotSpelledInSource;

`bool` here as well, please.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1157
 
+  auto ScopedTraversal = TraversingASTNodeNotSpelledInSource ||
+ TraversingASTChildrenNotSpelledInSource;

Here too.



Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2503
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+

Can you add one more test for the body like 
`cxxForRangeStmt(hasBody(compoundStmt()))` which should match in both modes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 304103.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,179 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_ConstructorDecl) {
+
+  std::string OtherStructPrefix = R"cpp(
+struct Other {
+)cpp";
+  std::string OtherStructSuffix = "};";
+
+  std::string CopyableStructName = "struct Copyable";
+  std::string BrokenStructName = "struct explicit Copyable";
+
+  std::string CodeSuffix = R"cpp(
+{
+Other m_i;
+Copyable();
+};
+)cpp";
+
+  std::string CopyCtor = "Other(const Other&) = default;";
+  std::string ExplicitCopyCtor = "explicit Other(const Other&) = default;";
+  std::string BrokenExplicitCopyCtor =
+  "explicit explicit explicit Other(const Other&) = default;";
+
+  std::string RewriteInput = OtherStructPrefix + CopyCtor + OtherStructSuffix +
+ CopyableStructName + CodeSuffix;
+  std::string ExpectedRewriteOutput = OtherStructPrefix + ExplicitCopyCtor +
+  OtherStructSuffix + CopyableStructName +
+  CodeSuffix;
+  std::string BrokenRewriteOutput = OtherStructPrefix + BrokenExplicitCopyCtor +
+OtherStructSuffix + BrokenStructName +
+CodeSuffix;
+
+  auto MatchedRecord =
+  cxxConstructorDecl(isCopyConstructor()).bind("copyConstructor");
+
+  auto RewriteRule =
+  changeTo(before(node("copyConstructor")), cat("explicit "));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray const&) = default;
+};

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 303728.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,179 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_ConstructorDecl) {
+
+  std::string OtherStructPrefix = R"cpp(
+struct Other {
+)cpp";
+  std::string OtherStructSuffix = "};";
+
+  std::string CopyableStructName = "struct Copyable";
+  std::string BrokenStructName = "struct explicit Copyable";
+
+  std::string CodeSuffix = R"cpp(
+{
+Other m_i;
+Copyable();
+};
+)cpp";
+
+  std::string CopyCtor = "Other(const Other&) = default;";
+  std::string ExplicitCopyCtor = "explicit Other(const Other&) = default;";
+  std::string BrokenExplicitCopyCtor =
+  "explicit explicit explicit Other(const Other&) = default;";
+
+  std::string RewriteInput = OtherStructPrefix + CopyCtor + OtherStructSuffix +
+ CopyableStructName + CodeSuffix;
+  std::string ExpectedRewriteOutput = OtherStructPrefix + ExplicitCopyCtor +
+  OtherStructSuffix + CopyableStructName +
+  CodeSuffix;
+  std::string BrokenRewriteOutput = OtherStructPrefix + BrokenExplicitCopyCtor +
+OtherStructSuffix + BrokenStructName +
+CodeSuffix;
+
+  auto MatchedRecord =
+  cxxConstructorDecl(isCopyConstructor()).bind("copyConstructor");
+
+  auto RewriteRule =
+  changeTo(before(node("copyConstructor")), cat("explicit "));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray const&) = default;
+};
+
+void testIt()
+{
+

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 303682.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,179 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_ConstructorDecl) {
+
+  std::string OtherStructPrefix = R"cpp(
+struct Other {
+)cpp";
+  std::string OtherStructSuffix = "};";
+
+  std::string CopyableStructName = "struct Copyable";
+  std::string BrokenStructName = "struct explicit Copyable";
+
+  std::string CodeSuffix = R"cpp(
+{
+Other m_i;
+Copyable();
+};
+)cpp";
+
+  std::string CopyCtor = "Other(const Other&) = default;";
+  std::string ExplicitCopyCtor = "explicit Other(const Other&) = default;";
+  std::string BrokenExplicitCopyCtor =
+  "explicit explicit explicit Other(const Other&) = default;";
+
+  std::string RewriteInput = OtherStructPrefix + CopyCtor + OtherStructSuffix +
+ CopyableStructName + CodeSuffix;
+  std::string ExpectedRewriteOutput = OtherStructPrefix + ExplicitCopyCtor +
+  OtherStructSuffix + CopyableStructName +
+  CodeSuffix;
+  std::string BrokenRewriteOutput = OtherStructPrefix + BrokenExplicitCopyCtor +
+OtherStructSuffix + BrokenStructName +
+CodeSuffix;
+
+  auto MatchedRecord =
+  cxxConstructorDecl(isCopyConstructor()).bind("copyConstructor");
+
+  auto RewriteRule =
+  changeTo(before(node("copyConstructor")), cat("explicit "));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray const&) = default;
+};
+
+void testIt()
+{
+

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 303679.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90982/new/

https://reviews.llvm.org/D90982

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -132,6 +132,13 @@
 compareSnippets(Expected, rewrite(Input));
   }
 
+  template  void testRuleFailure(R Rule, StringRef Input) {
+Transformers.push_back(
+std::make_unique(std::move(Rule), consumer()));
+Transformers.back()->registerMatchers();
+ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+  }
+
   // Transformers are referenced by MatchFinder.
   std::vector> Transformers;
   clang::ast_matchers::MatchFinder MatchFinder;
@@ -1067,6 +1074,134 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, ImplicitNodes_RangeFor) {
+
+  std::string CodePrefix = R"cpp(
+struct Container
+{
+int* begin() const;
+int* end() const;
+int* cbegin() const;
+int* cend() const;
+};
+
+void foo()
+{
+  const Container c;
+)cpp";
+
+  std::string BeginCallBefore = "  c.begin();";
+  std::string BeginCallAfter = "  c.cbegin();";
+
+  std::string ForLoop = "for (auto i : c)";
+  std::string BrokenForLoop = "for (auto i :.cbegin() c)";
+
+  std::string CodeSuffix = R"cpp(
+  {
+  }
+}
+)cpp";
+
+  std::string RewriteInput =
+  CodePrefix + BeginCallBefore + ForLoop + CodeSuffix;
+  std::string ExpectedRewriteOutput =
+  CodePrefix + BeginCallAfter + ForLoop + CodeSuffix;
+  std::string BrokenRewriteOutput =
+  CodePrefix + BeginCallAfter + BrokenForLoop + CodeSuffix;
+
+  auto MatchedRecord =
+  cxxMemberCallExpr(on(expr(hasType(qualType(isConstQualified(),
+ hasDeclaration(cxxRecordDecl(
+ hasName("Container"))
+   .bind("callTarget")),
+callee(cxxMethodDecl(hasName("begin"
+  .bind("constBeginCall");
+
+  auto RewriteRule =
+  changeTo(node("constBeginCall"), cat(name("callTarget"), ".cbegin()"));
+
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedRecord),
+RewriteRule),
+   RewriteInput, ExpectedRewriteOutput);
+
+  testRule(makeRule(traverse(TK_AsIs, MatchedRecord), RewriteRule),
+   RewriteInput, BrokenRewriteOutput);
+}
+
+TEST_F(TransformerTest, ImplicitNodes_ForStmt) {
+
+  std::string CodePrefix = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial const&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray const&) = default;
+};
+
+void testIt()
+{
+ContainsArray ca1;
+ContainsArray ca2;
+ca2 = ca1;
+)cpp";
+
+  auto CodeSuffix = "}";
+
+  auto LoopBody = R"cpp(
+{
+
+}
+)cpp";
+
+  auto RawLoop = "for (auto i = 0; i != 5; ++i)";
+
+  auto RangeLoop = "for (auto i : boost::irange(5))";
+
+  // Expect to rewrite the raw loop to the ranged loop.
+  // This works in TK_IgnoreUnlessSpelledInSource mode, but TK_AsIs
+  // mode also matches the hidden for loop generated in the copy assignment
+  // operator of ContainsArray. Transformer then fails to transform the code at
+  // all.
+
+  auto RewriteInput =
+  CodePrefix + RawLoop + LoopBody + RawLoop + LoopBody + CodeSuffix;
+
+  auto RewriteOutput =
+  CodePrefix + RangeLoop + LoopBody + RangeLoop + LoopBody + CodeSuffix;
+  {
+auto MatchedLoop = forStmt(
+has(declStmt(
+hasSingleDecl(varDecl(hasInitializer(integerLiteral(equals(0
+  .bind("loopVar",
+has(binaryOperator(hasOperatorName("!="),
+   hasLHS(ignoringImplicit(declRefExpr(
+   to(varDecl(equalsBoundNode("loopVar")),
+   hasRHS(expr().bind("upperBoundExpr",
+has(unaryOperator(hasOperatorName("++"),
+  hasUnaryOperand(declRefExpr(
+  to(varDecl(equalsBoundNode("loopVar"))
+.bind("incrementOp")));
+
+auto RewriteRule =
+changeTo(transformer::enclose(node("loopVar"), node("incrementOp")),
+ cat("auto ", name("loopVar"), " : boost::irange(",
+ node("upperBoundExpr"), ")"));
+
+

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
steveire requested review of this revision.

Update the ASTNodeTraverser to dump only nodes spelled in source.  There
are only a few which need to be handled, but Decl nodes for which
isImplicit() is true are handled together.

Update the RAV instances used in ASTMatchFinder to ignore the nodes too.
As with handling of template instantiations, it is necessary to allow
the RAV to process the implicit nodes because they need to be visitable
before the first traverse() matcher is encountered.  An exception to
this is in the MatchChildASTVisitor, because we sometimes wish to make a
node matchable but make its children not-matchable.  This is the case
for defaulted CXXMethodDecls for example.

This change accounts for handling nodes not spelled in source when using
direct matching of nodes, and when using the has() and hasDescendant()
matchers.  Other matchers such as
cxxRecordDecl(hasMethod(cxxMethodDecl())) still succeed for
compiler-generated methods for example after this change.  Updating the
implementations of hasMethod() and other matchers is for a follow-up
patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90982

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2331,6 +2331,250 @@
   }
 }
 
+TEST(Traversal, traverseNoImplicit) {
+  StringRef Code = R"cpp(
+struct NonTrivial {
+NonTrivial() {}
+NonTrivial(NonTrivial&) {}
+NonTrivial& operator=(NonTrivial&) { return *this; }
+
+~NonTrivial() {}
+};
+
+struct NoSpecialMethods {
+NonTrivial nt;
+};
+
+struct ContainsArray {
+NonTrivial arr[2];
+ContainsArray& operator=(ContainsArray ) = default;
+};
+
+void copyIt()
+{
+NoSpecialMethods nc1;
+NoSpecialMethods nc2(nc1);
+nc2 = nc1;
+
+ContainsArray ca;
+ContainsArray ca2;
+ca2 = ca;
+}
+
+struct HasCtorInits : NoSpecialMethods, NonTrivial
+{
+  int m_i;
+  NonTrivial m_nt;
+  HasCtorInits() : NoSpecialMethods(), m_i(42) {}
+};
+
+)cpp";
+  {
+auto M = cxxRecordDecl(hasName("NoSpecialMethods"),
+   has(cxxRecordDecl(hasName("NoSpecialMethods";
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  has(cxxConstructorDecl(isCopyConstructor(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  has(cxxMethodDecl(isCopyAssignmentOperator(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  has(cxxConstructorDecl(isDefaultConstructor(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"), has(cxxDestructorDecl()));
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+// Compiler generates a forStmt to copy the array
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, forStmt(;
+EXPECT_FALSE(
+matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, forStmt(;
+  }
+  {
+// The defaulted method declaration can be matched, but not its
+// definition, in IgnoreUnlessSpelledInSource mode
+auto MDecl = cxxMethodDecl(ofClass(cxxRecordDecl(hasName("ContainsArray"))),
+   isCopyAssignmentOperator(), isDefaulted());
+
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, MDecl)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, MDecl)));
+
+auto MDef = cxxMethodDecl(MDecl, has(compoundStmt()));
+
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, MDef)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, MDef)));
+
+// The parameter of the defaulted method can still be matched.
+auto MParm =
+cxxMethodDecl(MDecl, hasParameter(0, parmVarDecl(hasName("other";
+
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, MParm)));
+EXPECT_TRUE(matches(Code,