Added a better diagnostic when using the delete operator with lambdas

[Rakete1111]

Summary

This adds a new error for missing parentheses around lambdas in delete operators.

int main() {
  delete []() { return new int(); }();
}

This will result in:

test.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
  delete []() { return new int(); }();
  ^~~~~~~~~
test.cpp:2:9: note: add parentheses around the lambda
  delete []() { return new int(); }();
        ^
        (                          )

[Rakete1111]

Rebased and friendly ping.

[Rakete1111]

Used better diagnostic id.

[Rakete1111]

Updated error message, added a FixItHint + a rebase and friendly ping :)

[Rakete1111]

Rebased + friendly 2018 ping :)

[rsmith]

lib/Parse/ParseExprCXX.cpp

2906–2912

This seems error-prone. Given:

for (item *p = first, *oldp = p; p; p = p->next, delete [] oldp, oldp = p) {
  /*...*/;
}

... we'll decide the delete-expression is followed by a lambda. Likewise for cases like:

delete [] f->getPtr([] {
  return blah;
});

Our goal here should be a fast heuristic (don't use lookahead except when you're already confident you have a lambda) and zero false positives. How about these heuristics instead:

Assume that the delete [] is actually delete followed by a lambda if either:

• The next token is { or <, or
• The next token is ( and either
  • the following token is ) or
  • the following tokens are a type specifier followed by an identifier

This should have no false positives, and only has false negatives if a lambda has an unnamed parameter or a parameter with a non-trivial parameter type. (For the last condition, we could try to tentatively parse an entire parameter and see if it has a name, which would handle all cases except an expression/declaration ambiguity in the parameter declaration, but that seems like overkill to me. This is already performing more lookahead than I'd like, but delete [] expressions are rare enough that using two lookahead tokens for an improved error message seems OK.)

[Rakete1111]

lib/Parse/ParseExprCXX.cpp

2906–2912

Ah, I see. I had no idea that lookaheads were that expensive. Thanks for finding a better algorithm than me :)

[Rakete1111]

Addressed review comments :)

[rsmith]

Did you forget to upload the updated patch? This looks unchanged compared to the prior version.

[Rakete1111]

Did you forget to upload the updated patch? This looks unchanged compared to the prior version.

Whoops, that's true. Sorry...

[Rakete1111]

@rsmith Which type specifiers should I test for? just T? Or also T*, T&, ...? Now I'm just checking for an identifier, but anything else would make the lookahead more complicated I think. Any ideas?

[Rakete1111]

@rsmith Should I only check for an identifier, like only "int" and nothing else? Because parsing a full type specifier might be a bit expensive, no?

[rsmith]

lib/Parse/ParseExprCXX.cpp

2907–2909

Please don't request the additional lookahead tokens until you've checked Next. (Maybe factor out some parts of the check into a lambda so you can return early?)

2919

TryParseLambdaExpression will always commit to parsing as a lambda-expression, because the first two tokens are []. Since you've already determined that we definitely have a *lambda-expression* here, you may as well just call ParseLambdaExpression directly.

2925

This offset of -1 isn't right: when we have a fix-it pointing at a character, insertions are inserted before that character. Given delete[]{}, this would insert the ( between delet and e[], rather than between delete and [].

2929

Instead of getLocWithOffset here, use Lexer::getLocForEndOfToken; getLocWIthOffset(1) will return the wrong location if the closing brace token's length is greater than 1 (which can happen in the presence of escaped newlines).

2935–2936

Our coding style puts the else on the same line as the }.

However, we also generally don't like using else when the if block returns. And in this case I'd reverse the sense of the earlier if:

if (Lambda.isInvalid())
  return ExprError();

[Rakete1111]

Addressed review comments :)

[Rakete1111]

Rebased + friendly ping :)

[rsmith]

Thanks!

lib/Parse/ParseExprCXX.cpp

2956

I don't think this lambda is useful: since you're not caching the result of GetLookAheadToken(3) between calls, you may as well call it directly below.

2961–2962

Use GetLookAheadToken(3).isOneOf(tok::r_paren, tok::identifier) here.

2971–2980

Because we can be very confident the fix is correct, and we're recovering as if the fix were applied, you can combine the err_ diagnostic and the note_ diagnostic into one. That way, tools like clang -fixit will apply your fixit.

2972

If you want the location of the ] token for the end of the range here, it'd be best to save NextToken().getLocation() prior to calling ParseLambdaExpression(). Adding 1 to the location of the [ token isn't necessarily the right location (for instance, if there's whitespace between the [ and ], or if the left square bracket is spelled as <:).

[Rakete1111]

Addressed review comments.

[Rakete1111]

Rebased + friendly ping

[rsmith]

This looks good, with some minor changes. Please add more test coverage, though, specifically:

• test all four forms of lambda that we recognize after delete
• add a test that the FixItHint is correct (maybe in test/FixIt/fixit-cxx0x.cpp, which checks that applying the fixes results in working code)

lib/Parse/ParseExprCXX.cpp

2956

This doesn't seem quite right now. []() is not recognized as a lambda unless it's followed by an identifier. I think we want (and sorry if this is reverting to what you had before switching to isOneOf):

if (Next.isOneOf(tok::l_brace, tok::less) ||
    (Next.is(tok::l_paren) &&
     (GetLookAheadToken(3).is(tok::r_paren) ||
      (GetLookAheadToken(3).is(tok::identifier) && GetLookAheadToken(4).is(tok::r_paren))))) {

That is, we're matching:

• []{
• []<
• []()
• [](identifier identifier

2969

The 1 here should be a 0. Given

delete []{return new int;}();

this will insert the ) after the final ; instead of before it.

2978

No need for an else here, since the if block terminates by return.

[Rakete1111]

Addressed review comments.

Note that clang doesn't support the fourth kind of lambda yet ([]<>), because D36527 hasn't been merged yet, so I didn't add a test case for that one.

[Rakete1111]

Rebase + friendly ping :)

[Rakete1111]

Rebase and friendly ping :)

[Rakete1111]

Rebase + friendly ping :).

[Rakete1111]

friendly ping :)

[rsmith]

Thanks! Some minor nits, please feel free to commit once they're addressed.

In D36357#1177852, @Rakete1111 wrote:

Note that clang doesn't support the fourth kind of lambda yet ([]<>), because D36527 hasn't been merged yet, so I didn't add a test case for that one.

We support that now, so please add a test for that :)

clang/include/clang/Basic/DiagnosticParseKinds.td

109 ↗

(On Diff #198142)

Please add to this: "; add parentheses to treat this as a lambda-expression" or similar.

clang/lib/Parse/ParseExprCXX.cpp

2996 ↗

(On Diff #198142)

RightBracketLock -> RSquareLoc

(Our convention is to use Loc for "location" and to avoid the word "bracket" because it means different things in different English dialects -- usually [] in US English and usually () in UK English.)

2997 ↗

(On Diff #198142)

if -> that
parenthesis -> parentheses

3014 ↗

(On Diff #198142)

Maybe we should do this before producing the diagnostic so that we can suggest inserting the ) after the complete expression? (But I don't have a strong preference either way.)

[Rakete1111]

@rsmith One last question: The fixit diagnostic seems to be inconsistent with the rest?

main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
  delete[] { return new int; }
  ^~~~~~~~
        (                     )

Shouldn't the ^~~~~~~ be starting at the []?

[Rakete1111]

Is this also okay?

main.cpp:2:9: warning: lambda expressions are incompatible with C++98 [-Wc++98-compat]
  delete[] { return new int; }
        ^
main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'; add parentheses to treat this as a lambda-expression
  delete[] { return new int; }
  ^~~~~~~~
        (                     )

[rsmith]

In D36357#1491081, @Rakete1111 wrote:

@rsmith One last question: The fixit diagnostic seems to be inconsistent with the rest?

main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
  delete[] { return new int; }
  ^~~~~~~~
        (                     )

Shouldn't the ^~~~~~~ be starting at the []?

Pointing the caret at the [ might be clearer, but I think that's fine too.

In D36357#1491082, @Rakete1111 wrote:

Is this also okay?

main.cpp:2:9: warning: lambda expressions are incompatible with C++98 [-Wc++98-compat]
  delete[] { return new int; }
        ^
main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'; add parentheses to treat this as a lambda-expression
  delete[] { return new int; }
  ^~~~~~~~
        (                     )

Hmm. It would be better to produce the error first, but that would make it more difficult to get the fixit hint right. I suppose in the case where we've already decided we're going to diagnose, we could perform a tentative parse and skip to the } of the lambda.

[Rakete1111]

we could perform a tentative parse and skip to the } of the lambda.

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

[Rakete1111]

• Addressed review comments

[rsmith]

In D36357#1500949, @Rakete1111 wrote:

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

It would be a little expensive, yes, but we'd only be doing it on codepaths where we're producing an error -- for an ill-formed program, it's OK to take more time in order to produce a better diagnostic. Skipping to the next } won't work, because SkipUntil will skip over pairs of brace-balanced tokens (so you'll skip past the } you're looking for), but skipping until the next { and then skipping to the } after it should work.

clang/lib/Parse/ParseExprCXX.cpp

2996 ↗

(On Diff #198142)

Lock -> Loc. There's no k in "location" =)

[Rakete1111]

In D36357#1501961, @rsmith wrote:

In D36357#1500949, @Rakete1111 wrote:

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

It would be a little expensive, yes, but we'd only be doing it on codepaths where we're producing an error -- for an ill-formed program, it's OK to take more time in order to produce a better diagnostic. Skipping to the next } won't work, because SkipUntil will skip over pairs of brace-balanced tokens (so you'll skip past the } you're looking for), but skipping until the next { and then skipping to the } after it should work.

Hmm wouldn't this interact badly with {} in initializers?

[]<int = {0}> {};
[](int = {0}) {};

clang/lib/Parse/ParseExprCXX.cpp

2996 ↗

(On Diff #198142)

No idea how that k managed to sneak in :)

[Rakete1111]

Nevermind, seems to be working fine even with.

[rsmith]

In D36357#1501999, @Rakete1111 wrote:

In D36357#1501961, @rsmith wrote:

In D36357#1500949, @Rakete1111 wrote:

How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive?

It would be a little expensive, yes, but we'd only be doing it on codepaths where we're producing an error -- for an ill-formed program, it's OK to take more time in order to produce a better diagnostic. Skipping to the next } won't work, because SkipUntil will skip over pairs of brace-balanced tokens (so you'll skip past the } you're looking for), but skipping until the next { and then skipping to the } after it should work.

Hmm wouldn't this interact badly with {} in initializers?

[]<int = {0}> {};
[](int = {0}) {};

Ouch. The first of these two seems like it won't work, since SkipUntil has no idea the angles are brackets. =( Getting this right is ... really hairy. Eg:

[]<bool = a<b>{} // end of lambda here?
                >(){} // or here?

(The answer depends on whether a is a template.) And the same problem shows up in the trailing-return-type and the requires-clause.

How about this: SkipUntil an { or <. If you find a < first, don't attach a FixItHint to the diagnostic since we can't easily tell where to suggest putting the ).

[Rakete1111]

• Fix crash with invalid postfix expr
• Don't emit FixIt when using <

[Rakete1111]

• Use TryConsumeToken

[rsmith]

LGTM, thank you! :)

include/clang/Basic/DiagnosticParseKinds.td

   InGroup<CXX98Compat>, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup<GNUAlignofExpression>;
+def err_lambda_after_array_delete : Error<
+  "missing parentheses around lambda expression">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 

lib/Parse/ParseExprCXX.cpp

     //   [Footnote: A lambda expression with a lambda-introducer that consists
     //              of empty square brackets can follow the delete keyword if
     //              the lambda expression is enclosed in parentheses.]
-    // FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-    //        lambda-introducer.
-    ArrayDelete = true;
-    BalancedDelimiterTracker T(*this, tok::l_square);
+    TentativeParsingAction TPA(*this);
 
-    T.consumeOpen();
-    T.consumeClose();
-    if (T.getCloseLocation().isInvalid())
-      return ExprError();
+    // Basic lookahead to check if we have a lambda expression. If we
+    // encounter two braces with a semicolon, we can be pretty sure
+    // that this is a lambda, not say a compound literal. 
+    if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+        (NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+        !SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+      TPA.Revert();
+      ArrayDelete = true;
+      BalancedDelimiterTracker T(*this, tok::l_square);
+
+      T.consumeOpen();
+      T.consumeClose();
+      if (T.getCloseLocation().isInvalid())
+        return ExprError();
+    } else {
+      TPA.Revert();
+
+      // Warn if the non-capturing lambda isn't surrounded by parenthesis
+      // to disambiguate it from 'delete[]'. 
+      ExprResult Lambda = TryParseLambdaExpression();
+      if (!Lambda.isInvalid()) {
+        SourceLocation StartLoc = Lambda.get()->getLocStart();
+        Diag(StartLoc, diag::err_lambda_after_array_delete)
+          << SourceRange(StartLoc, Lambda.get()->getLocEnd());
+
+        // Evaluate any postfix expressions used on the lambda.
+        Lambda = ParsePostfixExpressionSuffix(Lambda);
+        return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+                                      Lambda.get());
+      }
+    }
   }
 
   ExprResult Operand(ParseCastExpression(false));

test/Parser/cxx0x-lambda-expressions.cpp

   void delete_lambda(int *p) {
     delete [] p;
     delete [] (int*) { new int }; // ok, compound-literal, not lambda
-    delete [] { return new int; } (); // expected-error{{expected expression}}
+    delete [] { return new int; } (); // expected-error{{missing parentheses around lambda expression}}
     delete [&] { return new int; } (); // ok, lambda
   }
 

test/SemaCXX/new-delete-0x.cpp

 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses around lambda expression}}
 }