Page MenuHomePhabricator

[Sema][Typo] Fix assertion failure for expressions with multiple typos
ClosedPublic

Authored by dgoldman on May 30 2019, 1:13 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dgoldman created this revision.May 30 2019, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 1:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith added a subscriber: rsmith.May 30 2019, 12:11 PM
rsmith added inline comments.
lib/Sema/SemaExprCXX.cpp
7713–7714 ↗(On Diff #202132)

What prevents diagnostics from being emitted for TypoExprs 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 TypoExprs, and will presumably diagnose it (and take action below if it turned out to be ambiguous etc).

7762–7787 ↗(On Diff #202132)

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.

dgoldman updated this revision to Diff 202772.Jun 3 2019, 12:07 PM
  • Fix diagnostics for ignored TypoExprs
dgoldman marked 2 inline comments as done.Jun 3 2019, 12:18 PM
dgoldman added inline comments.
lib/Sema/SemaExprCXX.cpp
7713–7714 ↗(On Diff #202132)

Fixed by discarding the failed TypoExprs

7762–7787 ↗(On Diff #202132)

So what do you recommend here? Checking for the non-ambiguity in RecursiveTransformLoop itself?

dgoldman updated this revision to Diff 202775.Jun 3 2019, 12:26 PM
dgoldman marked an inline comment as done.
  • Fix discarded correction test
rsmith added inline comments.Jun 3 2019, 1:29 PM
lib/Sema/SemaExprCXX.cpp
7762–7787 ↗(On Diff #202132)

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.

dgoldman marked an inline comment as done.Jun 3 2019, 4:31 PM
dgoldman added inline comments.
lib/Sema/SemaExprCXX.cpp
7762–7787 ↗(On Diff #202132)

Will submit a follow up patch to handle this, I think I'll likely rewrite the ambiguity handling to remove the AmbiguousTypoExprs bit.

dgoldman updated this revision to Diff 202938.Jun 4 2019, 7:36 AM
  • Remove unused State variable
dgoldman updated this revision to Diff 203011.Jun 4 2019, 1:33 PM
  • Move if empty check back
dgoldman updated this revision to Diff 206009.Jun 21 2019, 8:38 AM
  • Fix ambiguity handling and add more tests
dgoldman updated this revision to Diff 206011.Jun 21 2019, 8:54 AM
  • Add another test and fix up comment

Thanks, this looks good; just some nits.

lib/Sema/SemaExprCXX.cpp
7616 ↗(On Diff #206011)

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

7620–7621 ↗(On Diff #206011)

Reflow to 80 columns.

7686 ↗(On Diff #206011)

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

7694–7695 ↗(On Diff #206011)

Use std::move here.

7696–7699 ↗(On Diff #206011)

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

7723–7724 ↗(On Diff #206011)

And std::move here too.

7778–7779 ↗(On Diff #206011)

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 =[...]).

7781–7785 ↗(On Diff #206011)

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

7819 ↗(On Diff #206011)

isAmbiguous -> IsAmbiguous.

dgoldman updated this revision to Diff 206870.Jun 27 2019, 8:24 AM
dgoldman marked 9 inline comments as done.
  • Minor fixes
dgoldman added inline comments.Jun 27 2019, 8:26 AM
lib/Sema/SemaExprCXX.cpp
7762–7764 ↗(On Diff #206870)

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

rsmith accepted this revision.Jun 27 2019, 3:17 PM
rsmith added inline comments.
lib/Sema/SemaExprCXX.cpp
7762–7764 ↗(On Diff #206870)

Yes, please.

This revision is now accepted and ready to land.Jun 27 2019, 3:17 PM
dgoldman updated this revision to Diff 210862.Jul 19 2019, 10:59 AM
  • Minor fixes
dgoldman updated this revision to Diff 211184.Jul 22 2019, 1:53 PM
  • Bug fixes

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

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.

dgoldman updated this revision to Diff 211757.Jul 25 2019, 7:56 AM
  • 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).

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.

lib/Sema/TreeTransform.h
3485–3489 ↗(On Diff #211757)

It's not safe to assume that all created TypoExprs 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.)

dgoldman updated this revision to Diff 213437.Aug 5 2019, 12:46 PM

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)
dgoldman updated this revision to Diff 213438.Aug 5 2019, 12:47 PM
  • remove extra newline
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 12:03 PM