[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359713: [Parser] Avoid correcting delayed typos in array 
subscript multiple times. (authored by vsapsai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60848?vs=197421=197608#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60848/new/

https://reviews.llvm.org/D60848

Files:
  cfe/trunk/lib/Parse/ParseExpr.cpp
  cfe/trunk/test/SemaCXX/typo-correction.cpp
  cfe/trunk/test/SemaObjC/typo-correction-subscript.m


Index: cfe/trunk/lib/Parse/ParseExpr.cpp
===
--- cfe/trunk/lib/Parse/ParseExpr.cpp
+++ cfe/trunk/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 
Index: cfe/trunk/test/SemaCXX/typo-correction.cpp
===
--- cfe/trunk/test/SemaCXX/typo-correction.cpp
+++ cfe/trunk/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: cfe/trunk/test/SemaObjC/typo-correction-subscript.m
===
--- cfe/trunk/test/SemaObjC/typo-correction-subscript.m
+++ cfe/trunk/test/SemaObjC/typo-correction-subscript.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple i386-apple-macosx10.10 -fobjc-arc -fsyntax-only 
-Wno-objc-root-class %s -verify -disable-free
+
+@class Dictionary;
+
+@interface Test
+@end
+@implementation Test
+// rdar://problem/47403222
+- (void)rdar47403222:(Dictionary *)opts {
+  [self undeclaredMethod:undeclaredArg];
+  // expected-error@-1{{no visible @interface for 'Test' declares the selector 
'undeclaredMethod:'}}
+  opts[(__bridge id)undeclaredKey] = 0;
+  // expected-error@-1{{use of undeclared identifier 'undeclaredKey'}}
+}
+@end


Index: cfe/trunk/lib/Parse/ParseExpr.cpp
===
--- cfe/trunk/lib/Parse/ParseExpr.cpp
+++ cfe/trunk/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 
Index: cfe/trunk/test/SemaCXX/typo-correction.cpp
===
--- cfe/trunk/test/SemaCXX/typo-correction.cpp
+++ cfe/trunk/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: cfe/trunk/test/SemaObjC/typo-correction-subscript.m
===
--- cfe/trunk/test/SemaObjC/typo-correction-subscript.m
+++ cfe/trunk/test/SemaObjC/typo-correction-subscript.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple i386-apple-macosx10.10 -fobjc-arc -fsyntax-only -Wno-objc-root-class %s -verify -disable-free
+
+@class Dictionary;
+
+@interface Test
+@end
+@implementation Test
+// rdar://problem/47403222
+- (void)rdar47403222:(Dictionary *)opts {
+  [self undeclaredMethod:undeclaredArg];
+  // expected-error@-1{{no visible @interface for 'Test' declares the selector 'undeclaredMethod:'}}
+  opts[(__bridge id)undeclaredKey] = 0;
+  // expected-error@-1{{use of undeclared identifier 

[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 197421.
vsapsai added a comment.

- Add a test case with `-disable-free`.

Thanks for the suggestion, Erik.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60848/new/

https://reviews.llvm.org/D60848

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/SemaCXX/typo-correction.cpp
  clang/test/SemaObjC/typo-correction-subscript.m


Index: clang/test/SemaObjC/typo-correction-subscript.m
===
--- /dev/null
+++ clang/test/SemaObjC/typo-correction-subscript.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple i386-apple-macosx10.10 -fobjc-arc -fsyntax-only 
-Wno-objc-root-class %s -verify -disable-free
+
+@class Dictionary;
+
+@interface Test
+@end
+@implementation Test
+// rdar://problem/47403222
+- (void)rdar47403222:(Dictionary *)opts {
+  [self undeclaredMethod:undeclaredArg];
+  // expected-error@-1{{no visible @interface for 'Test' declares the selector 
'undeclaredMethod:'}}
+  opts[(__bridge id)undeclaredKey] = 0;
+  // expected-error@-1{{use of undeclared identifier 'undeclaredKey'}}
+}
+@end
Index: clang/test/SemaCXX/typo-correction.cpp
===
--- clang/test/SemaCXX/typo-correction.cpp
+++ clang/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 


Index: clang/test/SemaObjC/typo-correction-subscript.m
===
--- /dev/null
+++ clang/test/SemaObjC/typo-correction-subscript.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple i386-apple-macosx10.10 -fobjc-arc -fsyntax-only -Wno-objc-root-class %s -verify -disable-free
+
+@class Dictionary;
+
+@interface Test
+@end
+@implementation Test
+// rdar://problem/47403222
+- (void)rdar47403222:(Dictionary *)opts {
+  [self undeclaredMethod:undeclaredArg];
+  // expected-error@-1{{no visible @interface for 'Test' declares the selector 'undeclaredMethod:'}}
+  opts[(__bridge id)undeclaredKey] = 0;
+  // expected-error@-1{{use of undeclared identifier 'undeclaredKey'}}
+}
+@end
Index: clang/test/SemaCXX/typo-correction.cpp
===
--- clang/test/SemaCXX/typo-correction.cpp
+++ clang/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington accepted this revision.
erik.pilkington added a comment.
This revision is now accepted and ready to land.

Huh, okay. I guess there are two separate bugs that are conspiring to crash the 
compiler here: we shouldn't correct a TypoExpr more than once, and we shouldn't 
correct a TypoExpr less than once. I think fixing one of the two is fine. FYI I 
think you should be able to write a testcase if you RUN it with `-disable-free`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60848/new/

https://reviews.llvm.org/D60848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D60848#1472799 , @erik.pilkington 
wrote:

> Wait, why is NumTypos incorrect here? I think its because we don't handle the 
> typo on the line: `[self undeclaredMethod:undeclaredArg];`, even the 
> following asserts now. Seems like the right fix would be to track down why we 
> aren't handling the typo in the message expr.
>
>   // RUN: clang -cc1 %s -fobjc-arc
>   @implementation X
>   -x { [self undeclaredMethod:undeclaredArg]; }
>   @end
>


The reason why we aren't handling the typo in this case is because TypoExpr is 
created for `undeclaredArg` and when we emit `err_arc_may_not_respond` 

 we just return `ExprError` and drop `MultiExprArg` without checking if it 
contains TypoExpr or not. It is possible to fix this case but we have many 
more. Some of them are

  @class Test;
  void test(void) {
Test *obj;
[obj test:undeclaredArg];
  }



  struct S {
int x;
  } S;
  void test(void) {
[S test:undeclaredArg];
  }

I think that with the current design for delayed typos it's not feasible to fix 
all of these lingering delayed typos. Given that with disabled assertions  
non-empty `DelayedTypos` in `~Sema` isn't causing crashes, I've decided to 
improve handling the subscripts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60848/new/

https://reviews.llvm.org/D60848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Wait, why is NumTypos incorrect here? I think its because we don't handle the 
typo on the line: `[self undeclaredMethod:undeclaredArg];`, even the following 
asserts now. Seems like the right fix would be to track down why we aren't 
handling the typo in the message expr.

  // RUN: clang -cc1 %s -fobjc-arc
  @implementation X
  -x { [self undeclaredMethod:undeclaredArg]; }
  @end


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60848/new/

https://reviews.llvm.org/D60848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60848: [Parser] Avoid correcting delayed typos in array subscript multiple times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, erik.pilkington, majnemer.
Herald added subscribers: dexonsmith, jkorous.

We correct some typos in `ActOnArraySubscriptExpr` and
`ActOnOMPArraySectionExpr`, so when their result is `ExprError`, we can
end up correcting delayed typos in the same expressions again. In
general it is OK but when `NumTypos` is incorrect, we can hit the
assertion

> Assertion failed: (Entry != DelayedTypos.end() && "Failed to get the state 
> for a TypoExpr!"), function getTypoExprState, file 
> clang/lib/Sema/SemaLookup.cpp, line 5219.

This assertion is reproducible with Objective-C method

- (void)test { [self undeclaredMethod:undeclaredArg]; NSMutableDictionary *opts 
= [NSMutableDictionary new]; opts[(__bridge id)undeclaredKey] = @0; }

There is no test for the fix because `NumTypos` is still incorrect and
we hit the assertion

> Assertion failed: (DelayedTypos.empty() && "Uncorrected typos!"), function 
> ~Sema, file clang/lib/Sema/Sema.cpp, line 382.

Another option is to fix tracking delayed typos but it is non-trivial as
in many cases we drop erroneous expressions without cleaning up
`TypoExpr` they contain. Also the assertion in `~Sema` isn't causing
problems with assertions disabled, while a missing `TypoExprState` can
cause a segmentation fault.

rdar://problem/47403222


https://reviews.llvm.org/D60848

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/SemaCXX/typo-correction.cpp


Index: clang/test/SemaCXX/typo-correction.cpp
===
--- clang/test/SemaCXX/typo-correction.cpp
+++ clang/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 


Index: clang/test/SemaCXX/typo-correction.cpp
===
--- clang/test/SemaCXX/typo-correction.cpp
+++ clang/test/SemaCXX/typo-correction.cpp
@@ -678,7 +678,7 @@
 struct a0is0 {};
 struct b0is0 {};
 int g() {
-  0 [ // expected-error {{subscripted value is not an array}}
+  0 [
   sizeof(c0is0)]; // expected-error {{use of undeclared identifier}}
 };
 }
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -1582,7 +1582,9 @@
 
   SourceLocation RLoc = Tok.getLocation();
 
-  ExprResult OrigLHS = LHS;
+  LHS = Actions.CorrectDelayedTyposInExpr(LHS);
+  Idx = Actions.CorrectDelayedTyposInExpr(Idx);
+  Length = Actions.CorrectDelayedTyposInExpr(Length);
   if (!LHS.isInvalid() && !Idx.isInvalid() && !Length.isInvalid() &&
   Tok.is(tok::r_square)) {
 if (ColonLoc.isValid()) {
@@ -1594,12 +1596,6 @@
 }
   } else {
 LHS = ExprError();
-  }
-  if (LHS.isInvalid()) {
-(void)Actions.CorrectDelayedTyposInExpr(OrigLHS);
-(void)Actions.CorrectDelayedTyposInExpr(Idx);
-(void)Actions.CorrectDelayedTyposInExpr(Length);
-LHS = ExprError();
 Idx = ExprError();
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits