[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 ↗

(On Diff #64162)

Add a period at the end of the comment.

[bruno]

Thanks Manman, Committed r276020

lib/Sema/TreeTransform.h

if (getDerived().TryExpandParameterPacks(OrigElement.EllipsisLoc,
       // FIXME: Retain a pack expansion if RetainExpansion is true.
 
       // We've finished with this pack expansion.
       continue;
     }
 
     // Transform and check key.
     ExprResult Key = getDerived().TransformExpr(OrigElement.Key);
-    if (Key.isInvalid())
-      return ExprError();
-
-    if (Key.get() != OrigElement.Key)
+    if (!Key.isInvalid() && Key.get() != OrigElement.Key)
       ArgChanged = true;
 
     // Transform and check value.
     ExprResult Value
       = getDerived().TransformExpr(OrigElement.Value);
-    if (Value.isInvalid())
-      return ExprError();
-
-    if (Value.get() != OrigElement.Value)
+    if (!Value.isInvalid() && Value.get() != OrigElement.Value)
       ArgChanged = true;
 
     ObjCDictionaryElement Element = {
-      Key.get(), Value.get(), SourceLocation(), None
+        Key.isInvalid() ? OrigElement.Key : Key.get(),
+        Value.isInvalid() ? OrigElement.Value : Value.get(), SourceLocation(),
+        None
     };
     Elements.push_back(Element);
   }
 
   if (!getDerived().AlwaysRebuild() && !ArgChanged)
     return SemaRef.MaybeBindToTemporary(E);
 
   return getDerived().RebuildObjCDictionaryLiteral(E->getSourceRange(),

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'}}
+  };
+}