Tracking exception specification source locations

[aaron.ballman]

Summary

We do not currently track the source locations for exception specifications such that their source range can be queried through the AST. This leads to trying to write more complex code to determine the source range for uses like FixItHints (see D18575 for an example). In addition to use within tools like clang-tidy, I think this information may become more important to track as exception specifications become more integrated into the type system.

One thing this patch is missing is a test case. I'm not certain of the best way to test this functionality in isolation; suggestions welcome.

[alexfh]

Full context diff, please.

I'm not certain of the best way to test this functionality in isolation;

Same way other locations/ranges are tested in unittests/AST/SourceLocationTest.cpp?

[aaron.ballman]

In D20428#434238, @alexfh wrote:

Full context diff, please.

Pardon my complete ignorance, but how? I generated the diff from svn the usual way, so I assume I've missed some step.

I'm not certain of the best way to test this functionality in isolation;

Same way other locations/ranges are tested in unittests/AST/SourceLocationTest.cpp?

Ah, good point, we have unit tests for this sort of thing. I'll add some unit tests and generate a new patch. Thanks!

[alexfh]

In D20428#434242, @aaron.ballman wrote:

In D20428#434238, @alexfh wrote:

Full context diff, please.

Pardon my complete ignorance, but how? I generated the diff from svn the usual way, so I assume I've missed some step.

There are at least two ways to do this:

1. both git diff and svn diff can be convinced to produce diffs with full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
2. I find arcanist a very useful alternative to using web-interface for posting diffs: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line

[aaron.ballman]

For noexcept specifications without an expression, now tracking the full source range. Also, added tests.

[aaron.ballman]

In D20428#434270, @alexfh wrote:

In D20428#434242, @aaron.ballman wrote:

In D20428#434238, @alexfh wrote:

Full context diff, please.

Pardon my complete ignorance, but how? I generated the diff from svn the usual way, so I assume I've missed some step.

There are at least two ways to do this:

1. both git diff and svn diff can be convinced to produce diffs with full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
2. I find arcanist a very useful alternative to using web-interface for posting diffs: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line

Hmm, so (1) it's strange that I've yet to run into this before today, and (2) I'm on Windows using TortoiseSVN, so the command line help isn't overly helpful and it seems Tortoise doesn't have this functionality with its Create Patch menu item or related options. Sorry for the lack of context in my latest upload, but it does have two important changes:

1. noexcept now tracks its full source range, not just the start location of the noexcept token,
2. now, with actual tests.

[hintonda]

I can already catch all of these cases, but I can't catch this one, will this catch it too?

void g(void (*fp)(void) throw()) throw();
                        ^^^^^^^

Actually, I can catch it, but I haven't been able to create an ASTMatcher that will only include FunctionDecl's -- even using functionDecl() still returns all Decl's, not just FunctionDecl's.

[aaron.ballman]

Added test cases for parameter types that have exception specifications.

[aaron.ballman]

In D20428#434420, @hintonda wrote:

I can already catch all of these cases, but I can't catch this one, will this catch it too?

void g(void (*fp)(void) throw()) throw();
                        ^^^^^^^

Actually, I can catch it, but I haven't been able to create an ASTMatcher that will only include FunctionDecl's -- even using functionDecl() still returns all Decl's, not just FunctionDecl's.

Yes, this information is also tracked as part of my patch. I've added a few more test cases for it, thank you!

Assuming that @rsmith thinks my direction is reasonable for tracking source information, I strongly prefer to gate your patch on this one. Your custom parser may work fine, but I don't think it should be required since we already have this information from the real parser. Might as well thread the information through so that it can be used by tooling.

[hintonda]

Sure that sounds good to me. However, I would like to learn how to write better ASTMatchers.

In any case, this has been a great learning experience.

[alexfh]

Richard, ping.

[rsmith]

Seems reasonable to me.

lib/Parse/ParseDeclCXX.cpp

3403–3427

This change seems strange:

1. We should be using normal token-based ranges here; the getEndLoc().getLocWithOffset(-1) doesn't make sense.
2. This specifies a range for the error case, where we produce an exception specification of EST_None. We should not have a NoexceptRange if we're recovering as if there were no noexcept.

Can you revert the changes to this file?

[rsmith]

include/clang/AST/TypeLoc.h

1251

Can you arrange things so we only store this if there is an exception specification? Or, failing that, at least only store it for a FunctionProtoTypeLoc.

[aaron.ballman]

Updated based on review comments.

lib/Parse/ParseDeclCXX.cpp

3403–3427

I agree that this code is strange, what I found was that anything other than getLocWithOffset would produce the wrong range (the resulting range would be off-by-one). I found other code that does this, but it may be equally incorrect if there's a better way to get a range for a token.

As for #2, I don't think that's actually the case. This is the code path that parses a noexcept specifier with no arguments. (Result starts as EST_None, we aren't delayed, there is no dynamic exception spec, the next token is noexcept, there is no l_paren, and so we set SpecificationRange and Result before returning). Without tracking the source range, the following test case fails because the token range has no length:

Verifier.expectRange(1, 10, 1, 17);
EXPECT_TRUE(Verifier.match("void f() noexcept;\n", loc(functionType()),
                           Language::Lang_CXX11));

Is there a better way for me to get the full source range of the token? I would have assumed that getEndLoc() would work, but it appears to give an inconsistent result with other ranges that we track. e.g., noexcept(false) the caret points at ), throw(...) the caret points at ), but without the -1 offset above for void f() noexcept; the caret points at ; instead of t.

[hintonda]

lib/Parse/ParseDeclCXX.cpp

3403–3428

The range for a single token is a single location. The problem is your test. The range for "throw()" ends at the start of the ')' token. For "noexcept", the beginning and end are the same location, e.g., here's how "int" is tested:

TEST(MatchVerifier, ParseError) {
  LocationVerifier<VarDecl> Verifier;
  Verifier.expectLocation(1, 1);
  EXPECT_FALSE(Verifier.match("int i", varDecl()));
}

I think you should use this instead:

SourceRange NoexceptRange(Tok.getLocation());

[hintonda]

lib/Parse/ParseDeclCXX.cpp

3403–3428

Pasted the wrong test -- this one does the range:

TEST(RangeVerifier, WrongRange) {
  RangeVerifier<VarDecl> Verifier;
  Verifier.expectRange(1, 1, 1, 1);
  EXPECT_FALSE(Verifier.match("int i;", varDecl()));
}

[aaron.ballman]

Updating based on further review comments.

lib/Parse/ParseDeclCXX.cpp

3403–3428

Ah, how interesting; I would have assumed the range would be the full range of the source involved, but I forgot, that's what char ranges are for.

[hintonda]

lib/Parse/ParseDeclCXX.cpp

3403–3428

Sorry, looked at wrong tests -- should have looked at multi-character ending tokens.

NoexceptRange = SourceRange(
      Tok.getLocation(), Tok.getLocation().getLocWithOffset(Tok.getLength()));

[aaron.ballman]

@rsmith: Ping

[aaron.ballman]

One thing this patch does not do but needs to is fix ASTContext::adjustExceptionSpec() (Thanks to Don Hinton for pointing this out off-list!), however, I am at a bit of a loss for how best to rebuild the type location.

Would it be correct to call CreateTypeSourceInfo(Updated) to get a new TypeSourceInfo object of the proper size, then initializeFullCopy() the new type location object from the old one, and call setExceptionSpecRange() to update the source range information on the new type loc object? (The range would have to be passed to adjustExceptionSpec().) Or is there a better way to perform this rebuilding?

[hintonda]

The comment says to rebuild TypeSourceInfo, but isn't that what this does?

if (TSInfo->getType() != FD->getType())
  Updated = getFunctionTypeWithExceptionSpec(*this, TSInfo->getType(), ESI);
TSInfo->overrideType(Updated);

If so, could you fix this by either removing the assert or moving it below

TSInfo->overrideType(Updated);

[rsmith]

lib/AST/Decl.cpp

2938–2948

Can you factor out a function to get the FunctionTypeLoc from a FunctionDecl, when there is one (preferably as a separate change)? This is duplicated in a few places now (you can find some more by searching for getAs<FunctionProtoTypeLoc> in Sema), and looks slightly wrong here (we should skip calling convention attributes as well as parens).

[aaron.ballman]

Richard, with the following test case, my patch currently fails an assertion in ASTContext::adjustExceptionSpec() that I want to solve before committing:

template<typename T> void f7() {
  struct S { void g() noexcept(undefined_val); };  // expected-error{{use of undeclared identifier 'undefined_val'}}
}
template void f7<int>();

What is the correct way to rebuild the type source information? Would it be correct to call CreateTypeSourceInfo(Updated) to get a new TypeSourceInfo object of the proper size, then initializeFullCopy() the new type location object from the old one, and call setExceptionSpecRange() to update the source range information on the new type loc object? (The range would have to be passed to adjustExceptionSpec().) Or is there a better way to perform this rebuilding?

lib/AST/Decl.cpp

2938–2948

Yup, I can do that.

[aaron.ballman]

In D20428#452680, @hintonda wrote:

The comment says to rebuild TypeSourceInfo, but isn't that what this does?

if (TSInfo->getType() != FD->getType())
  Updated = getFunctionTypeWithExceptionSpec(*this, TSInfo->getType(), ESI);
TSInfo->overrideType(Updated);

That is only overriding the QualType stored in the TypeSourceInfo, but it does not rebuild the type source information as a whole (which includes the FunctionTypeLoc). The problem is that we need a new TSI that has sufficient space to store the exception specification source range in the case the existing source info does not have it. What's more, this function doesn't currently have that information (I believe it needs to be passed in as a parameter) to store in the adjusted type info.

[rsmith]

Ah right, we were (intentionally, but unfortunately) making an assumption that the TypeLoc data layout doesn't change when the exception spec of a function is updated. You'd need to make yourself a TypeLocBuilder, copy the relevant data, and update the exception spec loc.

Where are we adjusting the exception specification on the type source info? That (changing the type-info-as-written) seems like a somewhat unusual thing to do implicitly.

[sbarzowski]

What's happening here? It's accepted, but not merged and the last activity is 3 months old while my change is waiting for it. Can I help somehow?

[aaron.ballman]

In D20428#564511, @sbarzowski wrote:

What's happening here? It's accepted, but not merged and the last activity is 3 months old while my change is waiting for it. Can I help somehow?

It was accepted, but then caused build failures. I started correcting it, but ran into problems properly getting TypeLocBuilder to generate the proper type location information. Since then, I've ran out of funding to work on the problem and haven't had the spare time to pick it back up again. If someone would like to work on this patch in the meantime, I certainly would not be offended (and I'd be happy to help review).

[hintonda]

The problem is that when a noexcept(bool expr) specification is invalid, e.g., bad bool expr, the NoexceptType in Parser::tryParseExceptionSpecification is set to EST_None and returned. This will mean the FunctionDecl type won't have an exception TypeLoc, but the TypeSourceInfo type in ASTContext::adjustExceptionSpec will, which will trigger the assert in due to different sizes.

The easy fix is to set NoexceptType = EST_BasicNoexcept which will cause all tests to pass. The other option would be to leave it as EST_ComputedNoexcept, but that would cause a crash later during template instantiation.

Here's a patch (not sure I can uploade it):

diff --git a/lib/Parse/ParseDeclCXX.cpp b/lib/Parse/ParseDeclCXX.cpp
index 4002b09..7cfb8d5 100644
--- a/lib/Parse/ParseDeclCXX.cpp
+++ b/lib/Parse/ParseDeclCXX.cpp
@@ -3545,7 +3545,7 @@ Parser::tryParseExceptionSpecification(bool Delayed,
           Actions.CheckBooleanCondition(KeywordLoc, NoexceptExpr.get());
       NoexceptRange = SourceRange(KeywordLoc, T.getCloseLocation());
     } else {
-      NoexceptType = EST_None;
+      NoexceptType = EST_BasicNoexcept;
     }
   } else {
     // There is no argument.

[hintonda]

• Fix compile errors.
• When noexcept expr is invalid, set NoexceptType to EST_BasicNoexcept

[aaron.ballman]

I think that these changes look good to me, and I noticed Richard signed off on the other review for the exception spec changes (thank you for working on that!), so I think this is ready to commit.

[hintonda]

Great, thanks. Spoke to Richard about it last night and believe he's comfortable with this change, but I'll let him weigh in...

[hintonda]

Richard, is it okay to commit this?

[aaron.ballman]

I think that these changes look good to me, and I noticed Richard signed off on the other review for the exception spec changes (thank you for working on that!), so I think this is ready to commit.

In D20428#641068, @hintonda wrote:

Richard, is it okay to commit this?

If Richard does not respond today, then yes, it's fine to commit (it's had relatively extensive review already, and we can correct any outstanding issues post-commit).

[malcolm.parsons]

include/clang/AST/TypeLoc.h

1355

auto *

1443

SourceRange's constructor from SourceLocation is implicit.

[hintonda]

Aaron, I've got this version in a branch, so if you like, I'd be happy to apply Malcolm's suggestions.

[aaron.ballman]

In D20428#641416, @hintonda wrote:

Aaron, I've got this version in a branch, so if you like, I'd be happy to apply Malcolm's suggestions.

Please do; they're good suggestions.

[hintonda]

• Address comments.

[hintonda]

If you want this included in the 4.0 release, it needs to get in before they branch tomorrow.

[aaron.ballman]

In D20428#643339, @hintonda wrote:

If you want this included in the 4.0 release, it needs to get in before they branch tomorrow.

Continues to LGTM.

[malcolm.parsons]

In D20428#643973, @aaron.ballman wrote:

In D20428#643339, @hintonda wrote:

If you want this included in the 4.0 release, it needs to get in before they branch tomorrow.

Continues to LGTM.

Does Don have commit access?

[hintonda]

Sorry, but I do not have commit access.

[aaron.ballman]

In D20428#644007, @hintonda wrote:

Sorry, but I do not have commit access.

Ah! My apologies, I thought you did.

@malcolm.parsons, can you perform the commit? I'm traveling currently and can't do it myself right now.

include/clang/AST/Decl.h

   /// limited representation in the AST.
   SourceRange getReturnTypeSourceRange() const;
 
+  /// \brief Attempt to compute an informative source range covering the
+  /// function exception specification, if any.
+  SourceRange getExceptionSpecSourceRange() const;
+
   /// \brief Determine the type of an expression that calls this function.
   QualType getCallResultType() const {
     assert(getType()->getAs<FunctionType>() && "Expected a FunctionType!");

include/clang/AST/TypeLoc.h

   SourceLocation LocalRangeBegin;
   SourceLocation LParenLoc;
   SourceLocation RParenLoc;
+  SourceRange ExceptionSpecRange;
   SourceLocation LocalRangeEnd;
 };
 

     return SourceRange(getLParenLoc(), getRParenLoc());
   }
 
+  SourceRange getExceptionSpecRange() const {
+    return this->getLocalData()->ExceptionSpecRange;
+  }
+  void setExceptionSpecRange(SourceRange R) {
+    this->getLocalData()->ExceptionSpecRange = R;
+  }
+
   ArrayRef<ParmVarDecl *> getParams() const {
     return llvm::makeArrayRef(getParmArray(), getNumParams());
   }

     setLocalRangeBegin(Loc);
     setLParenLoc(Loc);
     setRParenLoc(Loc);
+    setExceptionSpecRange(SourceRange(Loc));
     setLocalRangeEnd(Loc);
     for (unsigned i = 0, e = getNumParams(); i != e; ++i)
       setParam(i, nullptr);

lib/AST/Decl.cpp

   return RTRange;
 }
 
+SourceRange FunctionDecl::getExceptionSpecSourceRange() const {
+  const TypeSourceInfo *TSI = getTypeSourceInfo();
+  if (!TSI)
+    return SourceRange();
+  FunctionTypeLoc FTL =
+    TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
+  if (!FTL)
+    return SourceRange();
+
+  return FTL.getExceptionSpecRange();
+}
+
 const Attr *FunctionDecl::getUnusedResultAttr() const {
   QualType RetType = getReturnType();
   if (RetType->isRecordType()) {

lib/Parse/ParseDeclCXX.cpp

 
   // If we already had a dynamic specification, parse the noexcept for,
   // recovery, but emit a diagnostic and don't store the results.
-  SourceRange NoexceptRange;
+  SourceRange NoexceptRange(Tok.getLocation(),
+                            Tok.getEndLoc().getLocWithOffset(-1));
   ExceptionSpecificationType NoexceptType = EST_None;
 
   SourceLocation KeywordLoc = ConsumeToken();

   } else {
     // There is no argument.
     NoexceptType = EST_BasicNoexcept;
-    NoexceptRange = SourceRange(KeywordLoc, KeywordLoc);
   }
 
   if (Result == EST_None) {

lib/Sema/SemaType.cpp

         ParmVarDecl *Param = cast<ParmVarDecl>(FTI.Params[i].Param);
         TL.setParam(tpi++, Param);
       }
-      // FIXME: exception specs
+      TL.setExceptionSpecRange(FTI.getExceptionSpecRange());
     }
     void VisitParenTypeLoc(ParenTypeLoc TL) {
       assert(Chunk.Kind == DeclaratorChunk::Paren);

lib/Sema/TreeTransform.h

   NewTL.setLocalRangeBegin(TL.getLocalRangeBegin());
   NewTL.setLParenLoc(TL.getLParenLoc());
   NewTL.setRParenLoc(TL.getRParenLoc());
+  NewTL.setExceptionSpecRange(TL.getExceptionSpecRange());
   NewTL.setLocalRangeEnd(TL.getLocalRangeEnd());
   for (unsigned i = 0, e = NewTL.getNumParams(); i != e; ++i)
     NewTL.setParam(i, ParamDecls[i]);

lib/Serialization/ASTReader.cpp

   TL.setLocalRangeBegin(ReadSourceLocation(Record, Idx));
   TL.setLParenLoc(ReadSourceLocation(Record, Idx));
   TL.setRParenLoc(ReadSourceLocation(Record, Idx));
+  TL.setExceptionSpecRange(SourceRange(ReadSourceLocation(Record, Idx),
+                                       ReadSourceLocation(Record, Idx)));
   TL.setLocalRangeEnd(ReadSourceLocation(Record, Idx));
   for (unsigned i = 0, e = TL.getNumParams(); i != e; ++i) {
     TL.setParam(i, ReadDeclAs<ParmVarDecl>(Record, Idx));

lib/Serialization/ASTWriter.cpp

   Record.AddSourceLocation(TL.getLocalRangeBegin());
   Record.AddSourceLocation(TL.getLParenLoc());
   Record.AddSourceLocation(TL.getRParenLoc());
+  Record.AddSourceRange(TL.getExceptionSpecRange());
   Record.AddSourceLocation(TL.getLocalRangeEnd());
   for (unsigned i = 0, e = TL.getNumParams(); i != e; ++i)
     Record.AddDeclRef(TL.getParam(i));

unittests/AST/SourceLocationTest.cpp

       Language::Lang_CXX11));
 }
 
+class ExceptionSpecRangeVerifier : public RangeVerifier<TypeLoc> {
+protected:
+  SourceRange getRange(const TypeLoc &Node) override {
+    auto T =
+      Node.getUnqualifiedLoc().castAs<FunctionProtoTypeLoc>();
+    assert(!T.isNull());
+    return T.getExceptionSpecRange();
+  }
+};
+
+TEST(FunctionDecl, ExceptionSpecifications) {
+  ExceptionSpecRangeVerifier Verifier;
+
+  Verifier.expectRange(1, 10, 1, 16);
+  EXPECT_TRUE(Verifier.match("void f() throw();\n", loc(functionType())));
+
+  Verifier.expectRange(1, 10, 1, 34);
+  EXPECT_TRUE(Verifier.match("void f() throw(void(void) throw());\n",
+                             loc(functionType())));
+
+  Verifier.expectRange(1, 10, 1, 19);
+  std::vector<std::string> Args;
+  Args.push_back("-fms-extensions");
+  EXPECT_TRUE(Verifier.match("void f() throw(...);\n", loc(functionType()),
+                             Args, Language::Lang_CXX));
+
+  Verifier.expectRange(1, 10, 1, 17);
+  EXPECT_TRUE(Verifier.match("void f() noexcept;\n", loc(functionType()),
+                             Language::Lang_CXX11));
+
+  Verifier.expectRange(1, 10, 1, 24);
+  EXPECT_TRUE(Verifier.match("void f() noexcept(false);\n", loc(functionType()),
+                             Language::Lang_CXX11));
+
+  Verifier.expectRange(1, 10, 1, 32);
+  EXPECT_TRUE(Verifier.match("void f() noexcept(noexcept(1+1));\n",
+                             loc(functionType()), Language::Lang_CXX11));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang