[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369427: [Sema][Typo] Fix assertion failure for expressions 
with multiple typos (authored by dgoldman, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62648

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/lib/Sema/SemaLookup.cpp
  cfe/trunk/test/Sema/typo-correction-recursive.cpp

Index: cfe/trunk/test/Sema/typo-correction-recursive.cpp
===
--- cfe/trunk/test/Sema/typo-correction-recursive.cpp
+++ cfe/trunk/test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: cfe/trunk/lib/Sema/SemaLookup.cpp
===
--- cfe/trunk/lib/Sema/SemaLookup.cpp
+++ cfe/trunk/lib/Sema/SemaLookup.cpp
@@ -5434,6 +5434,8 @@
   State.Consumer = std::move(TCC);
   State.DiagHandler = std::move(TDG);
   State.RecoveryHandler = std::move(TRC);
+  if (TE)
+TypoExprs.push_back(TE);
   return TE;
 }
 
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -7580,15 +7580,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that fail

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 213438.
dgoldman added a comment.

- remove extra newline


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLookup.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaLookup.cpp
===
--- lib/Sema/SemaLookup.cpp
+++ lib/Sema/SemaLookup.cpp
@@ -5361,6 +5361,8 @@
   State.Consumer = std::move(TCC);
   State.DiagHandler = std::move(TDG);
   State.RecoveryHandler = std::move(TRC);
+  if (TE)
+TypoExprs.push_back(TE);
   return TE;
 }
 
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7584,15 +7584,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that failed to ambiguity
+  /// and we don't want to report those diagnostics.
+  void EmitAllDiagnostics(bool IsAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {
   auto &State = SemaRef.getTypoExprState(TE);
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = Tra

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 213437.
dgoldman added a comment.

Fix test failure via `TransformTypos`

- Add a new property on Sema to track newly created Typos and use this from 
within TransformTypos in order to delete any Typos that are unreachable (tested 
in typo-correction-cxx11.cpp)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLookup.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaLookup.cpp
===
--- lib/Sema/SemaLookup.cpp
+++ lib/Sema/SemaLookup.cpp
@@ -5361,6 +5361,8 @@
   State.Consumer = std::move(TCC);
   State.DiagHandler = std::move(TDG);
   State.RecoveryHandler = std::move(TRC);
+  if (TE)
+TypoExprs.push_back(TE);
   return TE;
 }
 
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7584,15 +7584,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that failed to ambiguity
+  /// and we don't want to report those diagnostics.
+  void EmitAllDiagnostics(bool IsAmbiguous) {
 for (TypoExpr *TE 

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I don't think you need to change the `TreeTranform` base class to support this; 
`TreeTransform::TransformExpr` is an extension point which you can override 
from `TransformTypos` to inject the custom logic you need. But I also don't 
think this `TreeTransform::TransformExpr` approach works in general anyway, as 
noted below.




Comment at: lib/Sema/TreeTransform.h:3485-3489
+  // If the transformed Expr is valid, check if it's a TypoExpr so we can keep
+  // track of them. Otherwise, if the transform result is invalid, clear any
+  // TypoExprs that might have been created recursively (TODO: verify that
+  // this can happen in practice here instead of via an error trap).
+  if (Res.isUsable()) {

It's not safe to assume that all created `TypoExpr`s will be produced by 
`TransformExpr`. We might (for example) produce a new `TypoExpr` for the callee 
expression while transforming a `CallExpr`, and you won't catch that here.

It would seem cleaner and more robust to me to collect the typos produced 
during typo correction from within `Sema::createDelayedTypo`. (Eg, set some 
state on `Sema` for the duration of typo correction, and use that state to 
communicate any created typos back from `createDelayedTypo` to typo correction.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 211757.
dgoldman added a comment.

- Fix test failure: typo-correction-cxx11.cpp

Make sure that `TryTransform` clears any TypoExprs that are created
if it returns an invalid ExprResult (as the new TypoExprs are unreachable
from the result).


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/TreeTransform.h
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -118,6 +118,26 @@
   /// rather than in the subclass (e.g., lambda closure types).
   llvm::DenseMap TransformedLocalDecls;
 
+  /// Holds TypoExprs that are created during the transform. `TransformExpr`
+  /// and `TryTransform` will check for changes to this set and delete any
+  /// TypoExprs created from a failed transform. Otherwise, it is possible
+  /// that TypoExprs are created but are not reachable from the returned
+  /// ExprResult, meaning we will never correct the typos.
+  llvm::SmallSetVector TypoExprs;
+
+  /// Transform the given expression.
+  ///
+  /// By default, this routine transforms an expression by delegating to the
+  /// appropriate TransformXXXExpr function to build a new expression.
+  /// Subclasses may override this function to transform expressions using some
+  /// other mechanism.
+  ///
+  /// This method should does not handle clearing any TypoExprs and therefore
+  /// should not be called directly. Use `TransformExpr` instead.
+  ///
+  /// \returns the transformed expression.
+  ExprResult TransformExprWrapped(Expr *E);
+
 public:
   /// Initializes a new tree transformer.
   TreeTransfor

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D62648#1596535 , @rsmith wrote:

> Thanks, LGTM. Do you need someone to commit this for you?


Nope, I can commit it. There's still one outstanding issue though: 
typo-correction-cxx11.cpp is still failing with the `DelayedTypos.empty() && 
"Uncorrected typos!"` error, looking into how I can handle it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, LGTM. Do you need someone to commit this for you?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 211184.
dgoldman added a comment.

- Bug fixes


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7584,15 +7584,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that failed to ambiguity
+  /// and we don't want to report those diagnostics.
+  void EmitAllDiagnostics(bool IsAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {
   auto &State = SemaRef.getTypoExprState(TE);
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = IsAmbiguous
+? TypoCorrection() : State.Consumer->getCurrentCorrection();
+ExprResult Replacement = IsAmbiguous ? ExprError() : TransformCache[TE];
 
 // Extract the NamedDecl from the transformed TypoExpr and add it to the
 // TypoCorrection, replacing the existing decls. This ensures the right
@@ -7654,6 +7661,115 @@
 return ExprF

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 210862.
dgoldman added a comment.

- Minor fixes


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp
  test/SemaCXX/typo-correction-cxx11.cpp

Index: test/SemaCXX/typo-correction-cxx11.cpp
===
--- test/SemaCXX/typo-correction-cxx11.cpp
+++ test/SemaCXX/typo-correction-cxx11.cpp
@@ -50,10 +50,10 @@
 };
 
 void run(A *annotations) {
-  map new_annotations;
+  map annotation_map;
 
   auto &annotation = *annotations;
-  auto new_it = new_annotations.find(5);
+  auto new_it = annotation_map.find(5);
   auto &new_anotation = new_it.second;  // expected-note {{'new_anotation' declared here}}
   new_annotation->Swap(&annotation);  // expected-error {{use of undeclared identifier 'new_annotation'; did you mean 'new_anotation'?}}
 }
Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7610,15 +7610,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that failed to ambiguity
+  /// and we don't want to report those diagnosti

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7764
+  llvm::SmallDenseMap NewTransformCache;
+  auto SavedTransformCache = TransformCache;
+  TransformCache = NewTransformCache;

dgoldman wrote:
> Should I do the same `std::move` and `clear` here as well?
Yes, please.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7764
+  llvm::SmallDenseMap NewTransformCache;
+  auto SavedTransformCache = TransformCache;
+  TransformCache = NewTransformCache;

Should I do the same `std::move` and `clear` here as well?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 206870.
dgoldman marked 9 inline comments as done.
dgoldman added a comment.

- Minor fixes


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7610,15 +7610,22 @@
   llvm::SmallDenseMap OverloadResolution;
 
   /// Emit diagnostics for all of the TypoExprs encountered.
+  ///
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  ///
+  /// If we've failed to correct due to ambiguous corrections, we need to
+  /// be sure to pass empty corrections and replacements. Otherwise it's
+  /// possible that the Consumer has a TypoCorrection that failed to ambiguity
+  /// and we don't want to report those diagnostics.
+  void EmitAllDiagnostics(bool IsAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {
   auto &State = SemaRef.getTypoExprState(TE);
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = IsAmbiguous
+? TypoCorrection() : State.Consumer->getCurrentCorrection();
+ExprResult Replacement = IsAmbiguous ? ExprError() : TransformCache[TE];
 
 // Extract the NamedDecl from the transformed TypoExpr and add it to the
 // TypoCorrection, replacing the existing decls. This ensures the ri

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this looks good; just some nits.




Comment at: lib/Sema/SemaExprCXX.cpp:7616
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  void EmitAllDiagnostics(bool isAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {

Please capitalize this parameter name to match the surrounding code style.



Comment at: lib/Sema/SemaExprCXX.cpp:7620-7621
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = isAmbiguous ? TypoCorrection() : 
State.Consumer->getCurrentCorrection();
+ExprResult Replacement = isAmbiguous ? ExprError() : 
TransformCache[TE];
 

Reflow to 80 columns.



Comment at: lib/Sema/SemaExprCXX.cpp:7686
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult CheckForRecursiveTypos(ExprResult Res, bool &outIsAmbiguous) {
+if (Res.isInvalid()) {

`outIsAmbiguous` -> `IsAmbiguous` (we don't use an `out` prefix for output 
parameters).



Comment at: lib/Sema/SemaExprCXX.cpp:7694-7695
+
+auto SavedTypoExprs = TypoExprs;
+auto SavedAmbiguousTypoExprs = AmbiguousTypoExprs;
+llvm::SmallSetVector

Use `std::move` here.



Comment at: lib/Sema/SemaExprCXX.cpp:7696-7699
+llvm::SmallSetVector
+RecursiveTypoExprs, RecursiveAmbiguousTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+AmbiguousTypoExprs = RecursiveAmbiguousTypoExprs;

Call `TypoExprs.clear()` rather than copying from a default-constructed object.



Comment at: lib/Sema/SemaExprCXX.cpp:7723-7724
+
+TypoExprs = SavedTypoExprs;
+AmbiguousTypoExprs = SavedAmbiguousTypoExprs;
+

And `std::move` here too.



Comment at: lib/Sema/SemaExprCXX.cpp:7778-7779
+  }
+} while((Next = State.Consumer->peekNextCorrection()) &&
+Next.getEditDistance(false) == TC.getEditDistance(false));
+

Apply clang-format here. (Or: add space after `while` on the first line, and 
indent the second line so it begins in the same column as `(Next =[...]`).



Comment at: lib/Sema/SemaExprCXX.cpp:7781-7785
+if (!outIsAmbiguous) {
+  AmbiguousTypoExprs.remove(TE);
+  State.Consumer->restoreSavedPosition();
+} else
+  break;

Reverse the condition of this `if` so you can early-exit from the loop and 
remove the `else`.



Comment at: lib/Sema/SemaExprCXX.cpp:7819
   ExprResult Transform(Expr *E) {
-ExprResult Res;
-while (true) {
-  Res = TryTransform(E);
-
-  // Exit if either the transform was valid or if there were no TypoExprs
-  // to transform that still have any untried correction candidates..
-  if (!Res.isInvalid() ||
-  !CheckAndAdvanceTypoExprCorrectionStreams())
-break;
-}
-
-// Ensure none of the TypoExprs have multiple typo correction candidates
-// with the same edit length that pass all the checks and filters.
-// TODO: Properly handle various permutations of possible corrections when
-// there is more than one potentially ambiguous typo correction.
-// Also, disable typo correction while attempting the transform when
-// handling potentially ambiguous typo corrections as any new TypoExprs 
will
-// have been introduced by the application of one of the correction
-// candidates and add little to no value if corrected.
-SemaRef.DisableTypoCorrection = true;
-while (!AmbiguousTypoExprs.empty()) {
-  auto TE  = AmbiguousTypoExprs.back();
-  auto Cached = TransformCache[TE];
-  auto &State = SemaRef.getTypoExprState(TE);
-  State.Consumer->saveCurrentPosition();
-  TransformCache.erase(TE);
-  if (!TryTransform(E).isInvalid()) {
-State.Consumer->resetCorrectionStream();
-TransformCache.erase(TE);
-Res = ExprError();
-break;
-  }
-  AmbiguousTypoExprs.remove(TE);
-  State.Consumer->restoreSavedPosition();
-  TransformCache[TE] = Cached;
-}
-SemaRef.DisableTypoCorrection = false;
+bool isAmbiguous = false;
+ExprResult Res = RecursiveTransformLoop(E, isAmbiguous);

`isAmbiguous` -> `IsAmbiguous`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 206011.
dgoldman added a comment.

- Add another test and fix up comment


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().// expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
+
+class DeepAmbiguityHelper {
+public:
+  DeepAmbiguityHelper& help1();
+  DeepAmbiguityHelper& help2();
+
+  DeepAmbiguityHelper& methodA();
+  DeepAmbiguityHelper& somethingMethodB();
+  DeepAmbiguityHelper& functionC();
+  DeepAmbiguityHelper& deepMethodD();
+  DeepAmbiguityHelper& asDeepAsItGets();
+};
+
+DeepAmbiguityHelper deep_obj;
+
+int testDeepAmbiguity() {
+  deep_obj.
+  methodB(). // expected-error {{no member named 'methodB' in 'DeepAmbiguityHelper'}}
+  somethingMethodC().
+  functionD().
+  deepMethodD().
+  help3().
+  asDeepASItGet().
+  functionE();
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7613,12 +7613,12 @@
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  void EmitAllDiagnostics(bool isAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {
   auto &State = SemaRef.getTypoExprState(TE);
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = isAmbiguous ? TypoCorrection() : State.Consumer->getCurrentCorrection();
+ExprResult Replacement = isAmbiguous ? ExprError() : TransformCache[TE];
 
 // Extract the NamedDecl from the transformed TypoExpr and add it to the
 // TypoCorrection, replacing the existing decls. This ensures the right
@@ -7680,6 +7680,115 @@
 return ExprFilter(Res.get());
   }
 
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult CheckForRecursiveTypos(ExprResult Res, bool &outIsAmbiguous) {
+if (Res.isInvalid()) {
+  return Res;
+}
+/

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 206009.
dgoldman added a comment.

- Fix ambiguity handling and add more tests


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; }  // expected-note {{'getX' declared here}}
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; }  // expected-note {{'getY0' declared here}}
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+  getM(). // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+  triggee();  // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2().  // expected-error {{no member named 'getY2' in 'Z'}}
+  getM().
+  thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();  // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B();  // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E().  // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+  get_me_a_Z().value;// expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
+
+class AmbiguousHelper {
+public:
+  int helpMe();
+  int helpBe();
+};
+class Ambiguous {
+public:
+  int calculateA();
+  int calculateB();
+
+  AmbiguousHelper getHelp1();
+  AmbiguousHelper getHelp2();
+};
+
+Ambiguous ambiguous_obj;
+
+int testDirectAmbiguousCorrection() {
+  return ambiguous_obj.calculateZ();  // expected-error {{no member named 'calculateZ' in 'Ambiguous'}}
+}
+
+int testRecursiveAmbiguousCorrection() {
+  return ambiguous_obj.getHelp3().  // expected-error {{no member named 'getHelp3' in 'Ambiguous'}}
+  helpCe();
+}
+
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7613,12 +7613,12 @@
   /// If the TypoExprs were successfully corrected, then the diagnostics should
   /// suggest the corrections. Otherwise the diagnostics will not suggest
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  void EmitAllDiagnostics(bool isAmbiguous) {
 for (TypoExpr *TE : TypoExprs) {
   auto &State = SemaRef.getTypoExprState(TE);
   if (State.DiagHandler) {
-TypoCorrection TC = State.Consumer->getCurrentCorrection();
-ExprResult Replacement = TransformCache[TE];
+TypoCorrection TC = isAmbiguous ? TypoCorrection() : State.Consumer->getCurrentCorrection();
+ExprResult Replacement = isAmbiguous ? ExprError() : TransformCache[TE];
 
 // Extract the NamedDecl from the transformed TypoExpr and add it to the
 // TypoCorrection, replacing the existing decls. This ensures the right
@@ -7680,6 +7680,115 @@
 return ExprFilter(Res.get());
   }
 
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult CheckForRecursiveTypos(ExprResult Res, bool &outIsAmbiguous) {
+if (Res.isInvalid()) {
+  return Res;
+}
+// Check to see if any new TypoExprs were created. If so, we need to recurse
+// to check their validity.
+Expr *FixedExpr = Res.get();
+
+auto SavedTypoExprs = TypoExprs;
+auto SavedAmbiguousTypoExprs = AmbiguousTypoExprs;
+llvm::SmallSetVector
+RecursiveTypoExprs, RecursiveAmbiguousTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+AmbiguousTypoExprs = RecursiveAmbiguousTypoExprs;
+
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+if (!TypoExprs.empty()) {
+  // Recurse to handle newly created TypoExprs. If we're not able to
+  // handle them, disc

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 203011.
dgoldman added a comment.

- Move if empty check back


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; } // expected-note {{'getX' declared here}}
+  int getN() const { return m_n; }
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; } // expected-note {{'getY0' declared here}}
+  const Y& getY1() const { return m_y1; }
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+getM().  // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+triggee();   // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'}}
+getM().
+thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D(); // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B(); // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E(). // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+get_me_a_Z().value; // expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7680,6 +7680,61 @@
 return ExprFilter(Res.get());
   }
 
+  // Try to transform the given expression, looping through the correction
+  // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`.
+  //
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult RecursiveTransformLoop(Expr *E) {
+ExprResult Res;
+while (true) {
+  Res = TryTransform(E);
+
+  // The transform was valid: check if there any new TypoExprs were created.
+  // If so, we need to recurse to check their validity.
+  if (!Res.isInvalid()) {
+Expr *FixedExpr = Res.get();
+auto SavedTypoExprs = TypoExprs;
+llvm::SmallSetVector RecursiveTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+
+if (!TypoExprs.empty()) {
+  // Recurse to handle newly created TypoExprs. If we're not able to
+  // handle them, discard these TypoExprs.
+  ExprResult RecurResult = RecursiveTransformLoop(FixedExpr);
+  if (RecurResult.isInvalid()) {
+Res = ExprError();
+// Recursive corrections didn't work, wipe them away and don't add
+// them to the TypoExprs set.
+for (auto TE : TypoExprs) {
+  TransformCache.erase(TE);
+  SemaRef.clearDelayedTypo(TE);
+}
+  } else {
+// TypoExpr is valid: add newly created TypoExprs since we were
+// able to correct them.
+Res = RecurResult;
+SavedTypoExprs.set_union(TypoExprs);
+  }
+}
+
+TypoExprs = SavedTypoExprs;
+  }
+  // If the transform is still valid after checking for any new typos,
+  // it's good to go.
+  if (!Res.isInvalid())
+break;
+
+  // The transform was invalid, see if we have any TypoExprs with untried
+  // correction candidates.
+  if (!CheckAndAdvanceTypoExprCorrectionStreams())
+break;
+}
+return Res;
+  }
+
 public:
   TransformTypos(Sema &SemaRef, VarDecl *InitDecl, llvm::function_ref Filter)
   : BaseTransform(SemaRef), InitDecl(InitDecl), ExprFilter(Filter) {}
@@ -7707,16 +7762,7 @@
   ExprResult TransformBlockExpr(BlockExpr *E) { return Owned(E); }
 
   

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 202938.
dgoldman added a comment.

- Remove unused State variable


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; } // expected-note {{'getX' declared here}}
+  int getN() const { return m_n; }
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; } // expected-note {{'getY0' declared here}}
+  const Y& getY1() const { return m_y1; }
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+getM().  // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+triggee();   // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'}}
+getM().
+thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D(); // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B(); // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E(). // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+get_me_a_Z().value; // expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7680,6 +7680,59 @@
 return ExprFilter(Res.get());
   }
 
+  // Try to transform the given expression, looping through the correction
+  // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`.
+  //
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult RecursiveTransformLoop(Expr *E) {
+ExprResult Res;
+while (true) {
+  Res = TryTransform(E);
+
+  // The transform was valid: check if there any new TypoExprs were created.
+  // If so, we need to recurse to check their validity.
+  if (!Res.isInvalid() && !TypoExprs.empty()) {
+Expr *FixedExpr = Res.get();
+auto SavedTypoExprs = TypoExprs;
+llvm::SmallSetVector RecursiveTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+
+// Recurse to handle newly created TypoExprs. If we're not able to
+// handle them, discard these TypoExprs.
+ExprResult RecurResult = RecursiveTransformLoop(FixedExpr);
+if (RecurResult.isInvalid()) {
+  Res = ExprError();
+  // Recursive corrections didn't work, wipe them away and don't add
+  // them to the TypoExprs set.
+  for (auto TE : TypoExprs) {
+TransformCache.erase(TE);
+SemaRef.clearDelayedTypo(TE);
+  }
+} else {
+  // TypoExpr is valid: add newly created TypoExprs since we were
+  // able to correct them.
+  Res = RecurResult;
+  SavedTypoExprs.set_union(TypoExprs);
+}
+
+TypoExprs = SavedTypoExprs;
+  }
+  // If the transform is still valid after checking for any new typos,
+  // it's good to go.
+  if (!Res.isInvalid())
+break;
+
+  // The transform was invalid, see if we have any TypoExprs with untried
+  // correction candidates.
+  if (!CheckAndAdvanceTypoExprCorrectionStreams())
+break;
+}
+return Res;
+  }
+
 public:
   TransformTypos(Sema &SemaRef, VarDecl *InitDecl, llvm::function_ref Filter)
   : BaseTransform(SemaRef), InitDecl(InitDecl), ExprFilter(Filter) {}
@@ -7707,16 +7760,7 @@
   ExprResult TransformBlockExpr(BlockExpr *E) { return Owned(E); }
 
   ExprResult Transform(Expr *E) {
-ExprResult Res;
-

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman marked an inline comment as done.
dgoldman added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7787
 // Ensure none of the TypoExprs have multiple typo correction candidates
 // with the same edit length that pass all the checks and filters.
 // TODO: Properly handle various permutations of possible corrections when
 // there is more than one potentially ambiguous typo correction.
 // Also, disable typo correction while attempting the transform when
 // handling potentially ambiguous typo corrections as any new TypoExprs 
will
 // have been introduced by the application of one of the correction

rsmith wrote:
> dgoldman wrote:
> > rsmith wrote:
> > > What happens if an ambiguous `TypoExpr` is created as a result of one of 
> > > the outer level transformations?
> > > 
> > > In that case, I think that we will try alternatives for that `TypoExpr` 
> > > here, but that `TypoExpr` is not in the expression we're transforming (it 
> > > was created within `RecursiveTransformLoop` and isn't part of `E`), so 
> > > we're just redoing the same transformation we already did but with 
> > > typo-correction disabled. This means that the transform will fail 
> > > (because we hit a typo and can't correct it), so we'll accept the 
> > > original set of corrections despite them being ambiguous.
> > So what do you recommend here? Checking for the non-ambiguity in 
> > `RecursiveTransformLoop` itself?
> Suggestion: in the recursive transform, if we find a potential ambiguity, 
> check whether it's actually ambiguous before returning. If it is ambiguous, 
> fail out of the entire typo-correction process: one of our tied-for-best 
> candidates was ambiguous, so we deem the overall typo-correction process to 
> be ambiguous. And if not, then carry on as in your current patch.
> 
Will submit a follow up patch to handle this, I think I'll likely rewrite the 
ambiguity handling to remove the `AmbiguousTypoExprs` bit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7787
 // Ensure none of the TypoExprs have multiple typo correction candidates
 // with the same edit length that pass all the checks and filters.
 // TODO: Properly handle various permutations of possible corrections when
 // there is more than one potentially ambiguous typo correction.
 // Also, disable typo correction while attempting the transform when
 // handling potentially ambiguous typo corrections as any new TypoExprs 
will
 // have been introduced by the application of one of the correction

dgoldman wrote:
> rsmith wrote:
> > What happens if an ambiguous `TypoExpr` is created as a result of one of 
> > the outer level transformations?
> > 
> > In that case, I think that we will try alternatives for that `TypoExpr` 
> > here, but that `TypoExpr` is not in the expression we're transforming (it 
> > was created within `RecursiveTransformLoop` and isn't part of `E`), so 
> > we're just redoing the same transformation we already did but with 
> > typo-correction disabled. This means that the transform will fail (because 
> > we hit a typo and can't correct it), so we'll accept the original set of 
> > corrections despite them being ambiguous.
> So what do you recommend here? Checking for the non-ambiguity in 
> `RecursiveTransformLoop` itself?
Suggestion: in the recursive transform, if we find a potential ambiguity, check 
whether it's actually ambiguous before returning. If it is ambiguous, fail out 
of the entire typo-correction process: one of our tied-for-best candidates was 
ambiguous, so we deem the overall typo-correction process to be ambiguous. And 
if not, then carry on as in your current patch.



Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 202775.
dgoldman marked an inline comment as done.
dgoldman added a comment.

- Fix discarded correction test


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; } // expected-note {{'getX' declared here}}
+  int getN() const { return m_n; }
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; } // expected-note {{'getY0' declared here}}
+  const Y& getY1() const { return m_y1; }
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+getM().  // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+triggee();   // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'}}
+getM().
+thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D(); // expected-note {{'get_me_a_D' declared here}}
+};
+class Scope {
+public:
+  A make_an_A();
+  B make_a_B(); // expected-note {{'make_a_B' declared here}}
+};
+
+Scope scope_obj;
+
+int testDiscardedCorrections() {
+  return scope_obj.make_an_E(). // expected-error {{no member named 'make_an_E' in 'Scope'; did you mean 'make_a_B'}}
+get_me_a_Z().value; // expected-error {{no member named 'get_me_a_Z' in 'B'; did you mean 'get_me_a_D'}}
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7680,6 +7680,60 @@
 return ExprFilter(Res.get());
   }
 
+  // Try to transform the given expression, looping through the correction
+  // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`.
+  //
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult RecursiveTransformLoop(Expr *E) {
+ExprResult Res;
+while (true) {
+  Res = TryTransform(E);
+
+  // The transform was valid: check if there any new TypoExprs were created.
+  // If so, we need to recurse to check their validity.
+  if (!Res.isInvalid() && !TypoExprs.empty()) {
+Expr *FixedExpr = Res.get();
+auto SavedTypoExprs = TypoExprs;
+llvm::SmallSetVector RecursiveTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+
+// Recurse to handle newly created TypoExprs. If we're not able to
+// handle them, discard these TypoExprs.
+ExprResult RecurResult = RecursiveTransformLoop(FixedExpr);
+if (RecurResult.isInvalid()) {
+  Res = ExprError();
+  // Recursive corrections didn't work, wipe them away and don't add
+  // them to the TypoExprs set.
+  for (auto TE : TypoExprs) {
+auto &State = SemaRef.getTypoExprState(TE);
+TransformCache.erase(TE);
+SemaRef.clearDelayedTypo(TE);
+  }
+} else {
+  // TypoExpr is valid: add newly created TypoExprs since we were
+  // able to correct them.
+  Res = RecurResult;
+  SavedTypoExprs.set_union(TypoExprs);
+}
+
+TypoExprs = SavedTypoExprs;
+  }
+  // If the transform is still valid after checking for any new typos,
+  // it's good to go.
+  if (!Res.isInvalid())
+break;
+
+  // The transform was invalid, see if we have any TypoExprs with untried
+  // correction candidates.
+  if (!CheckAndAdvanceTypoExprCorrectionStreams())
+break;
+}
+return Res;
+  }
+
 public:
   TransformTypos(Sema &SemaRef, VarDecl *InitDecl, llvm::function_ref Filter)
   : BaseTransform(SemaRef), InitDecl(InitDecl), ExprFilter(Filter) {}
@@ -7707,16 +7761,7 @@
   ExprResult TransformBlo

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman marked 2 inline comments as done.
dgoldman added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:7713-7714
+  // Add the newly created typos to the TypoExprs list, even if they
+  // failed to apply. This allows them to be reaped although they won't
+  // emit any diagnostic.
+  SavedTypoExprs.set_union(TypoExprs);

rsmith wrote:
> What prevents diagnostics from being emitted for `TypoExpr`s we create for an 
> outer transform that we end up discarding? Eg, in:
> 
> ```
> struct Y {};
> struct Z { int value; };
> struct A {
>   Y get_me_a_Y();
> };
> struct B {
>   Z get_me_a_Z();
> };
> A make_an_A();
> B make_a_B();
> int f() {
>   return make_an_E().get_me_a_Z().value;
> }
> ```
> 
> I'm concerned that we will:
>  * try correcting `make_me_an_E` (`TypoExpr` `T0`) to `make_me_an_A` (because 
> that correction has the minimum edit distance) and discover that there is no 
> `get_me_a_Z` member, creating a new `TypoExpr` `T1`, and recurse:
>* try correcting `get_me_a_Z` (`TypoExpr` `T1`) to `get_me_a_Y` and 
> discover that there is no `value` member
>* no correction works: bail out of recursive step
>  * try correcting `make_me_an_E` (`TypoExpr` `T0`) to `make_me_a_B` and 
> succeed
> 
> But now we still have `T1` in our list of `TypoExpr`s, and will presumably 
> diagnose it (and take action below if it turned out to be ambiguous etc).
> 
Fixed by discarding the failed `TypoExpr`s



Comment at: lib/Sema/SemaExprCXX.cpp:7762-7787
 // Ensure none of the TypoExprs have multiple typo correction candidates
 // with the same edit length that pass all the checks and filters.
 // TODO: Properly handle various permutations of possible corrections when
 // there is more than one potentially ambiguous typo correction.
 // Also, disable typo correction while attempting the transform when
 // handling potentially ambiguous typo corrections as any new TypoExprs 
will
 // have been introduced by the application of one of the correction

rsmith wrote:
> What happens if an ambiguous `TypoExpr` is created as a result of one of the 
> outer level transformations?
> 
> In that case, I think that we will try alternatives for that `TypoExpr` here, 
> but that `TypoExpr` is not in the expression we're transforming (it was 
> created within `RecursiveTransformLoop` and isn't part of `E`), so we're just 
> redoing the same transformation we already did but with typo-correction 
> disabled. This means that the transform will fail (because we hit a typo and 
> can't correct it), so we'll accept the original set of corrections despite 
> them being ambiguous.
So what do you recommend here? Checking for the non-ambiguity in 
`RecursiveTransformLoop` itself?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 202772.
dgoldman added a comment.

- Fix diagnostics for ignored TypoExprs


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; } // expected-note {{'getX' declared here}}
+  int getN() const { return m_n; }
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; } // expected-note {{'getY0' declared here}}
+  const Y& getY1() const { return m_y1; }
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+getM().  // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+triggee();   // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'}}
+getM().
+thisDoesntSeemToMakeSense();
+}
+
+struct C {};
+struct D { int value; };
+struct A {
+  C get_me_a_C();
+};
+struct B {
+  D get_me_a_D();
+};
+A make_an_A();
+B make_an_B();
+int testDiscardedCorrections() {
+  return make_an_E().  // expected-error {{use of undeclared identifier 'make_an_E'}}
+get_me_a_Z().value;
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7680,6 +7680,60 @@
 return ExprFilter(Res.get());
   }
 
+  // Try to transform the given expression, looping through the correction
+  // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`.
+  //
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult RecursiveTransformLoop(Expr *E) {
+ExprResult Res;
+while (true) {
+  Res = TryTransform(E);
+
+  // The transform was valid: check if there any new TypoExprs were created.
+  // If so, we need to recurse to check their validity.
+  if (!Res.isInvalid() && !TypoExprs.empty()) {
+Expr *FixedExpr = Res.get();
+auto SavedTypoExprs = TypoExprs;
+llvm::SmallSetVector RecursiveTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+
+// Recurse to handle newly created TypoExprs. If we're not able to
+// handle them, discard these TypoExprs.
+ExprResult RecurResult = RecursiveTransformLoop(FixedExpr);
+if (RecurResult.isInvalid()) {
+  Res = ExprError();
+  // Recursive corrections didn't work, wipe them away and don't add
+  // them to the TypoExprs set.
+  for (auto TE : TypoExprs) {
+auto &State = SemaRef.getTypoExprState(TE);
+TransformCache.erase(TE);
+SemaRef.clearDelayedTypo(TE);
+  }
+} else {
+  // TypoExpr is valid: add newly created TypoExprs since we were
+  // able to correct them.
+  Res = RecurResult;
+  SavedTypoExprs.set_union(TypoExprs);
+}
+
+TypoExprs = SavedTypoExprs;
+  }
+  // If the transform is still valid after checking for any new typos,
+  // it's good to go.
+  if (!Res.isInvalid())
+break;
+
+  // The transform was invalid, see if we have any TypoExprs with untried
+  // correction candidates.
+  if (!CheckAndAdvanceTypoExprCorrectionStreams())
+break;
+}
+return Res;
+  }
+
 public:
   TransformTypos(Sema &SemaRef, VarDecl *InitDecl, llvm::function_ref Filter)
   : BaseTransform(SemaRef), InitDecl(InitDecl), ExprFilter(Filter) {}
@@ -7707,16 +7761,7 @@
   ExprResult TransformBlockExpr(BlockExpr *E) { return Owned(E); }
 
   ExprResult Transform(Expr *E) {
-ExprResult Res;
-while (true) {
-  Res = TryTransform(E);
-
-  // Exit if either the transform was valid or if there were no TypoExprs
-  // to transform that still have any untried correction candidates..
-

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:7713-7714
+  // Add the newly created typos to the TypoExprs list, even if they
+  // failed to apply. This allows them to be reaped although they won't
+  // emit any diagnostic.
+  SavedTypoExprs.set_union(TypoExprs);

What prevents diagnostics from being emitted for `TypoExpr`s we create for an 
outer transform that we end up discarding? Eg, in:

```
struct Y {};
struct Z { int value; };
struct A {
  Y get_me_a_Y();
};
struct B {
  Z get_me_a_Z();
};
A make_an_A();
B make_a_B();
int f() {
  return make_an_E().get_me_a_Z().value;
}
```

I'm concerned that we will:
 * try correcting `make_me_an_E` (`TypoExpr` `T0`) to `make_me_an_A` (because 
that correction has the minimum edit distance) and discover that there is no 
`get_me_a_Z` member, creating a new `TypoExpr` `T1`, and recurse:
   * try correcting `get_me_a_Z` (`TypoExpr` `T1`) to `get_me_a_Y` and discover 
that there is no `value` member
   * no correction works: bail out of recursive step
 * try correcting `make_me_an_E` (`TypoExpr` `T0`) to `make_me_a_B` and succeed

But now we still have `T1` in our list of `TypoExpr`s, and will presumably 
diagnose it (and take action below if it turned out to be ambiguous etc).




Comment at: lib/Sema/SemaExprCXX.cpp:7762-7787
 // Ensure none of the TypoExprs have multiple typo correction candidates
 // with the same edit length that pass all the checks and filters.
 // TODO: Properly handle various permutations of possible corrections when
 // there is more than one potentially ambiguous typo correction.
 // Also, disable typo correction while attempting the transform when
 // handling potentially ambiguous typo corrections as any new TypoExprs 
will
 // have been introduced by the application of one of the correction

What happens if an ambiguous `TypoExpr` is created as a result of one of the 
outer level transformations?

In that case, I think that we will try alternatives for that `TypoExpr` here, 
but that `TypoExpr` is not in the expression we're transforming (it was created 
within `RecursiveTransformLoop` and isn't part of `E`), so we're just redoing 
the same transformation we already did but with typo-correction disabled. This 
means that the transform will fail (because we hit a typo and can't correct 
it), so we'll accept the original set of corrections despite them being 
ambiguous.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62648



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

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[David Goldman via Phabricator via cfe-commits]

dgoldman created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As Typo Resolution can create new TypoExprs while resolving typos,
it is necessary to recurse through the expression to search for more
typos.

This should fix the assertion failure in `clang::Sema::~Sema()`:

  `DelayedTypos.empty() && "Uncorrected typos!"`

Notes:

- For expressions with multiple typos, we only give suggestions if we are able 
to resolve all typos in the expression
- This patch is similar to D37521  except that 
it does not eagerly commit to a correction for the first typo in the 
expression. Instead, it will search for corrections which fix all of the typos 
in the expression.


Repository:
  rC Clang

https://reviews.llvm.org/D62648

Files:
  lib/Sema/SemaExprCXX.cpp
  test/Sema/typo-correction-recursive.cpp

Index: test/Sema/typo-correction-recursive.cpp
===
--- /dev/null
+++ test/Sema/typo-correction-recursive.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check the following typo correction behavior:
+// - multiple typos in a single member call chain are all diagnosed
+// - no typos are diagnosed for multiple typos in an expression when not all
+//   typos can be corrected
+
+class DeepClass
+{
+public:
+  void trigger() const;  // expected-note {{'trigger' declared here}}
+};
+
+class Y
+{
+public:
+  const DeepClass& getX() const { return m_deepInstance; } // expected-note {{'getX' declared here}}
+  int getN() const { return m_n; }
+private:
+  DeepClass m_deepInstance;
+  int m_n;
+};
+
+class Z
+{
+public:
+  const Y& getY0() const { return m_y0; } // expected-note {{'getY0' declared here}}
+  const Y& getY1() const { return m_y1; }
+  const Y& getActiveY() const { return m_y0; }
+
+private:
+  Y m_y0;
+  Y m_y1;
+};
+
+Z z_obj;
+
+void testMultipleCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'; did you mean 'getY0'}}
+getM().  // expected-error {{no member named 'getM' in 'Y'; did you mean 'getX'}}
+triggee();   // expected-error {{no member named 'triggee' in 'DeepClass'; did you mean 'trigger'}}
+}
+
+void testNoCorrections()
+{
+  z_obj.getY2(). // expected-error {{no member named 'getY2' in 'Z'}}
+getM().
+thisDoesntSeemToMakeSense();
+}
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7680,6 +7680,56 @@
 return ExprFilter(Res.get());
   }
 
+  // Try to transform the given expression, looping through the correction
+  // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`.
+  //
+  // Since correcting typos may intoduce new TypoExprs, this function
+  // checks for new TypoExprs and recurses if it finds any. Note that it will
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult RecursiveTransformLoop(Expr *E) {
+ExprResult Res;
+while (true) {
+  Res = TryTransform(E);
+
+  // The transform was valid: check if there any new TypoExprs were created.
+  // If so, we need to recurse to check their validity.
+  if (!Res.isInvalid()) {
+Expr *FixedExpr = Res.get();
+auto SavedTypoExprs = TypoExprs;
+llvm::SmallSetVector RecursiveTypoExprs;
+TypoExprs = RecursiveTypoExprs;
+FindTypoExprs(TypoExprs).TraverseStmt(FixedExpr);
+
+// Check to see if any new TypoExprs were created. If they were, we need
+// to recurse in order to handle them. If we're not able to handle them,
+// discard this correction.
+if (!TypoExprs.empty()) {
+  ExprResult RecurResult = RecursiveTransformLoop(FixedExpr);
+  if (RecurResult.isInvalid())
+Res = ExprError();
+  else
+Res = RecurResult;
+  // Add the newly created typos to the TypoExprs list, even if they
+  // failed to apply. This allows them to be reaped although they won't
+  // emit any diagnostic.
+  SavedTypoExprs.set_union(TypoExprs);
+}
+
+TypoExprs = SavedTypoExprs;
+  }
+  // If the transform is still valid after checking for any new typos,
+  // it's good to go.
+  if (!Res.isInvalid())
+break;
+
+  // The transform was invalid, see if we have any TypoExprs with untried
+  // correction candidates.
+  if (!CheckAndAdvanceTypoExprCorrectionStreams())
+break;
+}
+return Res;
+  }
+
 public:
   TransformTypos(Sema &SemaRef, VarDecl *InitDecl, llvm::function_ref Filter)
   : BaseTransform(SemaRef), InitDecl(InitDecl), ExprFilter(Filter) {}
@@ -7707,16 +7757,7 @@
   ExprResult TransformBlockExpr(BlockExpr *E) { return Owned(E); }
 
   ExprResult Transform(Expr *E) {
-ExprResult Res;
-while (true) {
-  Res = TryTransform(E);
-
-  // Exit if eith