[SemObjC] Fix TypoExpr handling in TransformObjCDictionaryLiteral

[bruno]

Summary

Calls to TransformExpr for NSDictionary elements (keys and values) in
TransformObjCDictionaryLiteral might fail to obtain TypoCorrections. This is OK,
but the early exits with ExprError() don't allow TransformExpr to run for other
elements.

This avoids typo correction suggestions to kick in and has a major
drawback to compile time; it forces Transform to call TryTransform more
times than it should. Before this patch, the testcase added used to take
5s to compile!!! A bit more elaborate NSDictionary literal with some
undeclared enums would make the compiler take 22min to run, followed by
a crash.

[manmanren]

Before this patch, the testcase added used to take
5s to compile!!! A bit more elaborate NSDictionary literal with some
undeclared enums would make the compiler take 22min to run, followed by a crash.

--> this is a big improvement!

A few notes from discussions with Bruno:
This patch changes the base class TreeTransform<Derived>::TransformObjCDictionaryLiteral, which is not specific to TransformTypos.

Here we have multiple TypoExprs in a single DictionaryLiteral, and the search space for each TypoExpr can be pretty big. That is why it takes a long time to go through all the possible corrections.

I applied your patch, looks like it returns an ObjCDictionaryLiteral that contains TypoExpr as valid and that is why we break out of the while loop in TransformTypos::Transform. That does not look right.

We can probably prune the search space if one of the TypoExprs has zero candidate (in the testing case, the 2nd TypoExpr has zero candidate).

Manman

[bruno]

Thanks for the review Manman, I found a much better approach. Updated the patch to reflect that!

[bruno]

Ping!

[manmanren]

LGTM.

Manman

lib/Parse/ParseObjc.cpp

3489

Add a period at the end of the comment.

[bruno]

Thanks Manman, Committed r276020

lib/Parse/ParseObjc.cpp

Parser::ParseObjCBoxedExpr(SourceLocation AtLoc) {
   return Actions.BuildObjCBoxedExpr(SourceRange(AtLoc, RPLoc),
                                     ValueExpr.get());
 }
 
 ExprResult Parser::ParseObjCArrayLiteral(SourceLocation AtLoc) {
   ExprVector ElementExprs;                   // array elements.
   ConsumeBracket(); // consume the l_square.
 
+  bool HasInvalidEltExpr = false;
   while (Tok.isNot(tok::r_square)) {
     // Parse list of array element expressions (all must be id types).
     ExprResult Res(ParseAssignmentExpression());
     if (Res.isInvalid()) {
       // We must manually skip to a ']', otherwise the expression skipper will
       // stop at the ']' when it skips to the ';'.  We want it to skip beyond
       // the enclosing expression.
       SkipUntil(tok::r_square, StopAtSemi);
       return Res;
     }    
     
+    Res = Actions.CorrectDelayedTyposInExpr(Res.get());
+    if (Res.isInvalid())
+      HasInvalidEltExpr = true;
+
     // Parse the ellipsis that indicates a pack expansion.
     if (Tok.is(tok::ellipsis))
       Res = Actions.ActOnPackExpansion(Res.get(), ConsumeToken());    
     if (Res.isInvalid())
-      return true;
+      HasInvalidEltExpr = true;
 
     ElementExprs.push_back(Res.get());
 
     if (Tok.is(tok::comma))
       ConsumeToken(); // Eat the ','.
     else if (Tok.isNot(tok::r_square))
       return ExprError(Diag(Tok, diag::err_expected_either) << tok::r_square
                                                             << tok::comma);
   }
   SourceLocation EndLoc = ConsumeBracket(); // location of ']'
+
+  if (HasInvalidEltExpr)
+    return ExprError();
+
   MultiExprArg Args(ElementExprs);
   return Actions.BuildObjCArrayLiteral(SourceRange(AtLoc, EndLoc), Args);
 }
 
 ExprResult Parser::ParseObjCDictionaryLiteral(SourceLocation AtLoc) {
   SmallVector<ObjCDictionaryElement, 4> Elements; // dictionary elements.
   ConsumeBrace(); // consume the l_square.
+  bool HasInvalidEltExpr = false;
   while (Tok.isNot(tok::r_brace)) {
     // Parse the comma separated key : value expressions.
     ExprResult KeyExpr;
     {
       ColonProtectionRAIIObject X(*this);
       KeyExpr = ParseAssignmentExpression();
       if (KeyExpr.isInvalid()) {
         // We must manually skip to a '}', otherwise the expression skipper will
while (Tok.isNot(tok::r_brace)) {
     if (ValueExpr.isInvalid()) {
       // We must manually skip to a '}', otherwise the expression skipper will
       // stop at the '}' when it skips to the ';'.  We want it to skip beyond
       // the enclosing expression.
       SkipUntil(tok::r_brace, StopAtSemi);
       return ValueExpr;
     }
     
-    // Parse the ellipsis that designates this as a pack expansion.
-    SourceLocation EllipsisLoc;
+    // Check the key and value for possible typos
+    KeyExpr = Actions.CorrectDelayedTyposInExpr(KeyExpr.get());
+    ValueExpr = Actions.CorrectDelayedTyposInExpr(ValueExpr.get());
+    if (KeyExpr.isInvalid() || ValueExpr.isInvalid())
+      HasInvalidEltExpr = true;
+
+    // Parse the ellipsis that designates this as a pack expansion. Do not
+    // ActOnPackExpansion here, leave it to template instantiation time where
+    // we can get better diagnostics.
     if (getLangOpts().CPlusPlus)
       TryConsumeToken(tok::ellipsis, EllipsisLoc);
 
     // We have a valid expression. Collect it in a vector so we can
     // build the argument list.
     ObjCDictionaryElement Element = { 
       KeyExpr.get(), ValueExpr.get(), EllipsisLoc, None 
     };
     Elements.push_back(Element);
 
     if (!TryConsumeToken(tok::comma) && Tok.isNot(tok::r_brace))
       return ExprError(Diag(Tok, diag::err_expected_either) << tok::r_brace
                                                             << tok::comma);
   }
   SourceLocation EndLoc = ConsumeBrace();
-  
+  if (HasInvalidEltExpr)
+    return ExprError();
+  
   // Create the ObjCDictionaryLiteral.
   return Actions.BuildObjCDictionaryLiteral(SourceRange(AtLoc, EndLoc),
                                             Elements);
 }
 
 ///    objc-encode-expression:
 ///      \@encode ( type-name )
 ExprResult

test/SemaObjC/objc-array-literal.m

 id radar15147688() {
 #define R15147688_A @"hello"
 #define R15147688_B "world"
 #define CONCATSTR R15147688_A R15147688_B
   id x = @[ @"stuff", CONCATSTR ]; // no-warning
   x = @[ @"stuff", @"hello" "world"]; // expected-warning {{concatenated NSString literal for an NSArray expression}}
   return x;
 }
+
+enum XXXYYYZZZType { XXXYYYZZZTypeAny };
+void foo() {
+  NSArray *array = @[
+    @(XXXYYYZZZTypeA),                 // expected-error {{use of undeclared identifier 'XXXYYYZZZTypeA'; did you mean 'XXXYYYZZZTypeAny'}}
+    @(XXXYYYZZZTypeSomethingSomething) // expected-error {{use of undeclared identifier 'XXXYYYZZZTypeSomethingSomething'}}
+  ];
+}

test/SemaObjC/objc-dictionary-literal.m

int main() {
   [@{@"foo" : @"bar"} setObject:nil forKeyedSubscript:@"gorf"];
         @{@"foo" : @"bar"}[nil] = @"gorf";
   [@{@"foo" : @"bar"} setObject:pvoid forKeyedSubscript:@"gorf"];
         @{@"foo" : @"bar"}[pvoid] = @"gorf";
 
   return 0;
 }
 
+enum XXXYYYZZZType { XXXYYYZZZTypeAny }; // expected-note {{'XXXYYYZZZTypeAny' declared here}}
+void foo() {
+  NSDictionary *d = @{
+    @"A" : @(XXXYYYZZZTypeA), // expected-error {{use of undeclared identifier 'XXXYYYZZZTypeA'; did you mean 'XXXYYYZZZTypeAny'}}
+    @"F" : @(XXXYYYZZZTypeSomethingSomething), // expected-error {{use of undeclared identifier 'XXXYYYZZZTypeSomethingSomething'}}
+  };
+}