Add unit tests for source locations of AST nodes

[klimek]

unittests/AST/SourceLocationTest.cpp

25

I see overlap in the implementation with both TestVisitor.h and RecursiveASTVisitorTest.cpp (which already has location testing).

Do you think we can pull out something common?

[philipc]

unittests/AST/SourceLocationTest.cpp

25

There's bits of overlap between all of these unit tests, and I'm not sure yet which bits should be common.

I started using TestVisitor.h, but the structure of that code doesn't lend itself well to adding in range checking. I couldn't see a way to add it without either rewriting it all, or duplicating a lot of code.

The secondary issue is that TestVisitor.h doesn't use ASTMatchers, and I think they are quite useful for this sort of testing. I'm assuming that ASTMatchers shouldn't be used for testing RAV, since it's essentially another lay on top of RAV, so there may be things you can't test that way.

[klimek]

unittests/AST/SourceLocationTest.cpp

25

Yes, you're right. My main concern was that I wanted to make sure you know of the other instances, and didn't see a quick way to integrate them. I can poke at that later...

187

Both here and below having the extra expactLocation seems superfluous, if we do not also want to support multiple locations (which probably would require a slightly different interface again).

I would just put the stuff into the constructor. Unless you have plans for adding much more to the interface...

[philipc]

Add a topmost bind in the verifier. A different bind can still be specified if needed.

[philipc]

unittests/AST/SourceLocationTest.cpp

187

I didn't put it in the constructor because I expect LocationVerifier and RangeVerifier to be inherited from for testing different location members of nodes, and I didn't want the derived classes to need to define constructors and forward the arguments up. Is there a way to avoid that?

[klimek]

unittests/AST/SourceLocationTest.cpp

30

ExpectId seems completely unused now...

96

Can we instead use EXPECT(...) to fail the test? That way googletest would take care of keeping track of errors etc.

187

I would expect the constructors to then take different arguments. Also, I think the price of having a little more boilerplate is fine for the clearer use... I don't feel super-strongly about this, though, so feel free to make the call either way.

[philipc]

unittests/AST/SourceLocationTest.cpp

30

Yes, I left it there in case a test needs to specify its own bind within the matcher. I can delete it for now if you think that's better.

96

This result is returned at the end of match(), which is used in an EXPECT(), so it will keep track of the error.

187

I'll add some test cases that need to use different verifiers and see how it looks after that.

[klimek]

unittests/AST/SourceLocationTest.cpp

30

I would delete it. I dislike unused things that "might" be used in the future, as more often than not they end up staying unused.

96

I'm aware - my point is that all the result tracking could be deleted and replaced with one EXPECT here, which would lead to simpler code overall. Instead of EXPECT_TRUE(match(...)), the test could call Verifier.expectMatch(...).

[philipc]

unittests/AST/SourceLocationTest.cpp

30

Fixed.

96

Doing that means that failures no longer print the line number of the test case that is failing, but in most cases it will be the test case that needs fixing, not the run() function. You still have the test name, but it's easier to navigate to a line number. If that is acceptable, then this certainly simplifies things a lot.

[klimek]

unittests/AST/SourceLocationTest.cpp

96

Oh, I thought gtest prints the full stack trace at the point of the exception failure. .. If it doesn't, I'm happy with your solution.

96

And when I say "exception" I mean "expectation" :)

[philipc]

• deleted ExpectId
• small cleanup for VerifyResult
• added tests to ensure failures can be caught
• added LabelDecl range test, which is an example test that needs to derive from RangeVerifier
• renamed test case names - is this correct? Other unit tests are not consistent in this.

I think this is close to ready. Please review and commit if okay.

[philipc]

unittests/AST/SourceLocationTest.cpp

96

It doesn't have the full stack trace. And actually it's more than just the line number that is wrong; it doesn't include the code being tested either. So I have left it as is (with a small change to how the result is set).

[klimek]

Apart from my two nits looks good.

unittests/AST/SourceLocationTest.cpp

39

Can all methods after here be protected?

100

Are you already seeing a use case for this (as in: one of the next patches you do will use this) or is this purely speculative?

[philipc]

unittests/AST/SourceLocationTest.cpp

39

Yes.

100

It's needed to test any location that isn't an endpoint of the range of a node. A good example is BinaryOperator::getOperatorLoc().

[philipc]

Make some methods protected.

[philipc]

unittests/AST/SourceLocationTest.cpp

100

I should clarify that this patch is just a response to some other commits, and I have no plans to add more tests currently, but I do want to make it easy for others to add tests as they fix bugs.

[klimek]

Makes sense. I find it rather unfortunate that run() has to be public, but I think this is a fine first iteration, so LG.

[philipc]

Thanks for your review Manuel. Do I need to do anything more to get this committed? I don't have commit access.

[klimek]

Thanks for pinging.

Adding Chandler for general direction approval, and whether this is the right location...

[chandlerc]

Sure, here are my thoughts.

I'm generally fine with you committing this Manuel whenever, as long as someone does the follow-ups to restructure it. If you're not likely to have time to do the follow-ups, then we might want to wait to commit until it's a bit closer to the final shape of things.

unittests/AST/SourceLocationTest.cpp

26–28

Add a FIXME to hoist this into a general testing library that can be shared between different AST tests? Also, when that happens, we should really consider making these GoogleMock matchers, as controversial as that may be.

An important note is that I suspect that long term we will want to slice the AST unit tests differently. It seems better long-term to have a unit test file for an AST node (or group of related nodes) rather than a test for source locations for all AST nodes.

I'm fine with starting here though, as these are the types of things that are most pressing. I would just like to have some of the forward directions charted out in comments for future maintainers.

Some of this should probably go in the top-level file comment.

I'll try to chat w/ dgregor, efriedma and others about the exact long-term design of test for the AST.

119–121

It would be better in all of these printing contexts to re-use the existing printing facilities.

Specifically, FullSourceLoc should be avoided here. You're not carrying these things around or storing them for a long time. The only reason for FullSourceLoc to exist is when it is inconvenient to mention the source manager, and it isn't here.

Instead, use SourceLocation, and use SourceLocation::print to render them as text.

[philipc]

• added some FIXMEs
• removed use of FullSourceLoc

[philipc]

unittests/AST/SourceLocationTest.cpp

26–28

Added FIXMEs.

119–121

Done, although I couldn't see an equivalent way of printing ranges.

[klimek]

Submitted as r167470

unittests/AST/CMakeLists.txt

   CommentLexer.cpp
   CommentParser.cpp
   DeclPrinterTest.cpp
+  SourceLocationTest.cpp
   StmtPrinterTest.cpp
   )
 

unittests/AST/SourceLocationTest.cpp

+//===- unittest/AST/SourceLocationTest.cpp - AST source loc unit tests ----===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains tests for SourceLocation and SourceRange fields
+// in AST nodes.
+//
+// FIXME: In the long-term, when we test more than source locations, we may
+// want to have a unit test file for an AST node (or group of related nodes),
+// rather than a unit test file for source locations for all AST nodes.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ast_matchers {
+
+using clang::tooling::newFrontendActionFactory;
+using clang::tooling::runToolOnCodeWithArgs;
+using clang::tooling::FrontendActionFactory;
+
+enum Language { Lang_C, Lang_CXX };
+
+/// \brief Base class for verifying some property of nodes found by a matcher.
+///
+/// FIXME: This class should be shared with other AST tests.
+template <typename NodeType>
+class MatchVerifier : public MatchFinder::MatchCallback {
+public:
+  template <typename MatcherType>
+  testing::AssertionResult match(const std::string &Code,
+                                 const MatcherType &AMatcher) {
+    return match(Code, AMatcher, Lang_CXX);
+  }
+
+  template <typename MatcherType>
+  testing::AssertionResult match(const std::string &Code,
+                                 const MatcherType &AMatcher, Language L);
+
+protected:
+  virtual void run(const MatchFinder::MatchResult &Result);
+  virtual void verify(const MatchFinder::MatchResult &Result,
+                      const NodeType &Node) = 0;
+
+  void setFailure(const Twine &Result) {
+    Verified = false;
+    VerifyResult = Result.str();
+  }
+
+private:
+  bool Verified;
+  std::string VerifyResult;
+};
+
+/// \brief Runs a matcher over some code, and returns the result of the
+/// verifier for the matched node.
+template <typename NodeType> template <typename MatcherType>
+testing::AssertionResult MatchVerifier<NodeType>::match(
+    const std::string &Code, const MatcherType &AMatcher, Language L) {
+  MatchFinder Finder;
+  Finder.addMatcher(AMatcher.bind(""), this);
+  OwningPtr<FrontendActionFactory> Factory(newFrontendActionFactory(&Finder));
+
+  std::vector<std::string> Args;
+  StringRef FileName;
+  switch (L) {
+  case Lang_C:
+    Args.push_back("-std=c99");
+    FileName = "input.c";
+    break;
+  case Lang_CXX:
+    Args.push_back("-std=c++98");
+    FileName = "input.cc";
+    break;
+  }
+
+  // Default to failure in case callback is never called
+  setFailure("Could not find match");
+  if (!runToolOnCodeWithArgs(Factory->create(), Code, Args, FileName))
+    return testing::AssertionFailure() << "Parsing error";
+  if (!Verified)
+    return testing::AssertionFailure() << VerifyResult;
+  return testing::AssertionSuccess();
+}
+
+template <typename NodeType>
+void MatchVerifier<NodeType>::run(const MatchFinder::MatchResult &Result) {
+  const NodeType *Node = Result.Nodes.getNodeAs<NodeType>("");
+  if (!Node) {
+    setFailure("Matched node has wrong type");
+  } else {
+    // Callback has been called, default to success
+    Verified = true;
+    verify(Result, *Node);
+  }
+}
+
+/// \brief Verify whether a node has the correct source location.
+///
+/// By default, Node.getSourceLocation() is checked. This can be changed
+/// by overriding getLocation().
+template <typename NodeType>
+class LocationVerifier : public MatchVerifier<NodeType> {
+public:
+  void expectLocation(unsigned Line, unsigned Column) {
+    ExpectLine = Line;
+    ExpectColumn = Column;
+  }
+
+protected:
+  void verify(const MatchFinder::MatchResult &Result, const NodeType &Node) {
+    SourceLocation Loc = getLocation(Node);
+    unsigned Line = Result.SourceManager->getSpellingLineNumber(Loc);
+    unsigned Column = Result.SourceManager->getSpellingColumnNumber(Loc);
+    if (Line != ExpectLine || Column != ExpectColumn) {
+      std::string MsgStr;
+      llvm::raw_string_ostream Msg(MsgStr);
+      Msg << "Expected location <" << ExpectLine << ":" << ExpectColumn
+          << ">, found <";
+      Loc.print(Msg, *Result.SourceManager);
+      Msg << '>';
+      this->setFailure(Msg.str());
+    }
+  }
+
+  virtual SourceLocation getLocation(const NodeType &Node) {
+    return Node.getLocation();
+  }
+
+private:
+  unsigned ExpectLine, ExpectColumn;
+};
+
+/// \brief Verify whether a node has the correct source range.
+///
+/// By default, Node.getSourceRange() is checked. This can be changed
+/// by overriding getRange().
+template <typename NodeType>
+class RangeVerifier : public MatchVerifier<NodeType> {
+public:
+  void expectRange(unsigned BeginLine, unsigned BeginColumn,
+                   unsigned EndLine, unsigned EndColumn) {
+    ExpectBeginLine = BeginLine;
+    ExpectBeginColumn = BeginColumn;
+    ExpectEndLine = EndLine;
+    ExpectEndColumn = EndColumn;
+  }
+
+protected:
+  void verify(const MatchFinder::MatchResult &Result, const NodeType &Node) {
+    SourceRange R = getRange(Node);
+    SourceLocation Begin = R.getBegin();
+    SourceLocation End = R.getEnd();
+    unsigned BeginLine = Result.SourceManager->getSpellingLineNumber(Begin);
+    unsigned BeginColumn = Result.SourceManager->getSpellingColumnNumber(Begin);
+    unsigned EndLine = Result.SourceManager->getSpellingLineNumber(End);
+    unsigned EndColumn = Result.SourceManager->getSpellingColumnNumber(End);
+    if (BeginLine != ExpectBeginLine || BeginColumn != ExpectBeginColumn ||
+        EndLine != ExpectEndLine || EndColumn != ExpectEndColumn) {
+      std::string MsgStr;
+      llvm::raw_string_ostream Msg(MsgStr);
+      Msg << "Expected range <" << ExpectBeginLine << ":" << ExpectBeginColumn
+          << '-' << ExpectEndLine << ":" << ExpectEndColumn << ">, found <";
+      Begin.print(Msg, *Result.SourceManager);
+      Msg << '-';
+      End.print(Msg, *Result.SourceManager);
+      Msg << '>';
+      this->setFailure(Msg.str());
+    }
+  }
+
+  virtual SourceRange getRange(const NodeType &Node) {
+    return Node.getSourceRange();
+  }
+
+private:
+  unsigned ExpectBeginLine, ExpectBeginColumn, ExpectEndLine, ExpectEndColumn;
+};
+
+TEST(MatchVerifier, ParseError) {
+  LocationVerifier<VarDecl> Verifier;
+  Verifier.expectLocation(1, 1);
+  EXPECT_FALSE(Verifier.match("int i", varDecl()));
+}
+
+TEST(MatchVerifier, NoMatch) {
+  LocationVerifier<VarDecl> Verifier;
+  Verifier.expectLocation(1, 1);
+  EXPECT_FALSE(Verifier.match("int i;", recordDecl()));
+}
+
+TEST(MatchVerifier, WrongType) {
+  LocationVerifier<RecordDecl> Verifier;
+  Verifier.expectLocation(1, 1);
+  EXPECT_FALSE(Verifier.match("int i;", varDecl()));
+}
+
+TEST(LocationVerifier, WrongLocation) {
+  LocationVerifier<VarDecl> Verifier;
+  Verifier.expectLocation(1, 1);
+  EXPECT_FALSE(Verifier.match("int i;", varDecl()));
+}
+
+TEST(RangeVerifier, WrongRange) {
+  RangeVerifier<VarDecl> Verifier;
+  Verifier.expectRange(1, 1, 1, 1);
+  EXPECT_FALSE(Verifier.match("int i;", varDecl()));
+}
+
+class LabelDeclRangeVerifier : public RangeVerifier<LabelStmt> {
+protected:
+  virtual SourceRange getRange(const LabelStmt &Node) {
+    return Node.getDecl()->getSourceRange();
+  }
+};
+
+TEST(LabelDecl, Range) {
+  LabelDeclRangeVerifier Verifier;
+  Verifier.expectRange(1, 12, 1, 12);
+  EXPECT_TRUE(Verifier.match("void f() { l: return; }", labelStmt()));
+}
+
+TEST(LabelStmt, Range) {
+  RangeVerifier<LabelStmt> Verifier;
+  Verifier.expectRange(1, 12, 1, 15);
+  EXPECT_TRUE(Verifier.match("void f() { l: return; }", labelStmt()));
+}
+
+TEST(ParmVarDecl, KNRLocation) {
+  LocationVerifier<ParmVarDecl> Verifier;
+  Verifier.expectLocation(1, 8);
+  EXPECT_TRUE(Verifier.match("void f(i) {}", varDecl(), Lang_C));
+}
+
+TEST(ParmVarDecl, KNRRange) {
+  RangeVerifier<ParmVarDecl> Verifier;
+  Verifier.expectRange(1, 8, 1, 8);
+  EXPECT_TRUE(Verifier.match("void f(i) {}", varDecl(), Lang_C));
+}
+
+} // end namespace ast_matchers
+} // end namespace clang