Page MenuHomePhabricator

Tracking exception specification source locations
ClosedPublic

Authored by aaron.ballman on May 19 2016, 6:00 AM.

Details

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.

Event Timeline

aaron.ballman retitled this revision from to Tracking exception specification source locations.
aaron.ballman updated this object.
aaron.ballman added a reviewer: rsmith.

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?

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!

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

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

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 added a comment.EditedMay 19 2016, 8:45 AM

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.

Added test cases for parameter types that have exception specifications.

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.

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.

Richard, ping.

rsmith edited edge metadata.May 26 2016, 5:43 PM

Seems reasonable to me.

lib/Parse/ParseDeclCXX.cpp
3403–3427 ↗(On Diff #83816)

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 added inline comments.May 26 2016, 5:44 PM
include/clang/AST/TypeLoc.h
1251 ↗(On Diff #83816)

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 edited edge metadata.
aaron.ballman marked an inline comment as done.

Updated based on review comments.

lib/Parse/ParseDeclCXX.cpp
3403–3427 ↗(On Diff #83816)

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 added inline comments.May 27 2016, 9:19 AM
lib/Parse/ParseDeclCXX.cpp
3403–3428 ↗(On Diff #83816)

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 added inline comments.May 27 2016, 9:33 AM
lib/Parse/ParseDeclCXX.cpp
3403–3428 ↗(On Diff #83816)

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 marked 3 inline comments as done.

Updating based on further review comments.

lib/Parse/ParseDeclCXX.cpp
3403–3428 ↗(On Diff #83816)

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 added inline comments.May 27 2016, 10:13 AM
lib/Parse/ParseDeclCXX.cpp
3403–3428 ↗(On Diff #83816)

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

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

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?

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 accepted this revision.Jun 8 2016, 1:41 PM
rsmith edited edge metadata.
rsmith added inline comments.
lib/AST/Decl.cpp
2938–2948 ↗(On Diff #83816)

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).

This revision is now accepted and ready to land.Jun 8 2016, 1:41 PM

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 ↗(On Diff #83816)

Yup, I can do that.

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.

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.

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?

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).

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 updated this revision to Diff 83346.Jan 5 2017, 11:39 PM
hintonda edited edge metadata.
  • Fix compile errors.
  • When noexcept expr is invalid, set NoexceptType to EST_BasicNoexcept

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.

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

Richard, is it okay to commit this?

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.

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 added inline comments.
include/clang/AST/TypeLoc.h
1355 ↗(On Diff #83816)

auto *

1443 ↗(On Diff #83816)

SourceRange's constructor from SourceLocation is implicit.

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

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 updated this revision to Diff 83816.Jan 10 2017, 9:09 AM
  • Address comments.

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

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

Continues to LGTM.

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?

Sorry, but I do not have commit access.

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.

This revision was automatically updated to reflect the committed changes.