[ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression

[arphaman]

Summary

This patch ensures that the typo fixit for the @try/@finally/@autoreleasepool { } directive is shown only when we're parsing an actual statement where such directives can actually be present.

[bruno]

Hi Alex, thanks for working on this

lib/Parse/ParseObjc.cpp

2877 ↗

(On Diff #78728)

Does this only triggers when Res.isInvalid() returns true in the first part of the patch? I wonder if it's also safe to allow ExprStatementTokLoc = AtLoc; for every path or only when it fails.

[arphaman]

lib/Parse/ParseObjc.cpp

2877 ↗

(On Diff #78728)

Yes, this code will always flow to the body of the if with Res.isInvalid, but only after this code is executed, so we need to set ExprStatementTokLoc before the check for Res.isInvalid. As well as that, all users of ExprStatementTokLoc currently care only about the current location where the current statement is being parsed (they check to see if some expression location matches it), so it should be set before a method call like ParseExpressionWithLeadingAt, and it doesn't have to be cleared as the next statement will have a different location anyway. So it seems safe to set ExprStatementTokLoc = AtLoc as it follows the intended convention.

[bruno]

Thanks for the explanation, LGTM

cfe/trunk/lib/Parse/ParseObjc.cpp

if (Tok.isObjCAtKeyword(tok::objc_autoreleasepool))
     return ParseObjCAutoreleasePoolStmt(AtLoc);
 
   if (Tok.isObjCAtKeyword(tok::objc_import) &&
       getLangOpts().DebuggerSupport) {
     SkipUntil(tok::semi);
     return Actions.ActOnNullStmt(Tok.getLocation());
   }
 
+  ExprStatementTokLoc = AtLoc;
   ExprResult Res(ParseExpressionWithLeadingAt(AtLoc));
   if (Res.isInvalid()) {
     // If the expression is invalid, skip ahead to the next semicolon. Not
     // doing this opens us up to the possibility of infinite loops if
     // ParseExpression does not consume any tokens.
     SkipUntil(tok::semi);
     return StmtError();
   }
default:
     case tok::objc_protocol:
       return ParsePostfixExpressionSuffix(ParseObjCProtocolExpression(AtLoc));
     case tok::objc_selector:
       return ParsePostfixExpressionSuffix(ParseObjCSelectorExpression(AtLoc));
     case tok::objc_available:
       return ParseAvailabilityCheckExpr(AtLoc);
       default: {
         const char *str = nullptr;
-        if (GetLookAheadToken(1).is(tok::l_brace)) {
+        // Only provide the @try/@finally/@autoreleasepool fixit when we're sure
+        // that this is a proper statement where such directives could actually
+        // occur.
+        if (GetLookAheadToken(1).is(tok::l_brace) &&
+            ExprStatementTokLoc == AtLoc) {
           char ch = Tok.getIdentifierInfo()->getNameStart()[0];
           str =  
             ch == 't' ? "try" 
                       : (ch == 'f' ? "finally" 
                                    : (ch == 'a' ? "autoreleasepool" : nullptr));
         }
         if (str) {
           SourceLocation kwLoc = Tok.getLocation();

cfe/trunk/test/Parser/objc-at-directive-fixit.m

+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -verify -fobjc-exceptions %s
+// RUN: not %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -fdiagnostics-parseable-fixits -fobjc-exceptions %s 2>&1 | FileCheck %s
+
+// rdar://19669565
+
+void bar(int x);
+
+void f() {
+  @try { }
+  @finally { }
+  @autoreleasepool { }
+
+  // Provide a fixit when we are parsing a standalone statement
+  @tr { }; // expected-error {{unexpected '@' in program}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"try"
+  @finaly { }; // expected-error {{unexpected '@' in program}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:10}:"finally"
+  @autorelpool { }; // expected-error {{unexpected '@' in program}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:15}:"autoreleasepool"
+
+  // Ensure that no fixit is given when parsing expressions
+  // CHECK-NOT: fix-it
+  id thing = @autoreleasepool { }; // expected-error {{unexpected '@' in program}}
+  (void)@tr { }; // expected-error {{unexpected '@' in program}}
+  bar(@final { }); // expected-error {{unexpected '@' in program}}
+  for(@auto;;) { } // expected-error {{unexpected '@' in program}}
+  [@try]; // expected-error {{unexpected '@' in program}}
+}