r222549 - Use the full-Expr filter to disambiguate equidistant correction

[Kaelyn Takata]

On Fri, Nov 21, 2014 at 11:17 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Nov 21, 2014 at 10:47 AM, Kaelyn Takata <rikka at google.com> wrote:
>
>> Author: rikka
>> Date: Fri Nov 21 12:47:58 2014
>> New Revision: 222549
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=222549&view=rev
>> Log:
>> Use the full-Expr filter to disambiguate equidistant correction
>> candidates.
>>
>> Modified:
>>     cfe/trunk/lib/Sema/SemaExprCXX.cpp
>>     cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=222549&r1=222548&r2=222549&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Fri Nov 21 12:47:58 2014
>> @@ -5992,7 +5992,7 @@ class TransformTypos : public TreeTransf
>>    typedef TreeTransform<TransformTypos> BaseTransform;
>>
>>    llvm::function_ref<ExprResult(Expr *)> ExprFilter;
>> -  llvm::SmallSetVector<TypoExpr *, 2> TypoExprs;
>> +  llvm::SmallSetVector<TypoExpr *, 2> TypoExprs, AmbiguousTypoExprs;
>>    llvm::SmallDenseMap<TypoExpr *, ExprResult, 2> TransformCache;
>>    llvm::SmallDenseMap<OverloadExpr *, Expr *, 4> OverloadResolution;
>>
>> @@ -6059,6 +6059,15 @@ class TransformTypos : public TreeTransf
>>      return nullptr;
>>    }
>>
>> +  ExprResult TryTransform(Expr *E) {
>> +    Sema::SFINAETrap Trap(SemaRef);
>> +    ExprResult Res = TransformExpr(E);
>> +    if (Trap.hasErrorOccurred() || Res.isInvalid())
>> +      return ExprError();
>> +
>> +    return ExprFilter(Res.get());
>> +  }
>> +
>>  public:
>>    TransformTypos(Sema &SemaRef, llvm::function_ref<ExprResult(Expr *)>
>> Filter)
>>        : BaseTransform(SemaRef), ExprFilter(Filter) {}
>> @@ -6079,30 +6088,42 @@ public:
>>    ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }
>>
>>    ExprResult Transform(Expr *E) {
>> -    ExprResult res;
>> -    bool error = false;
>> +    ExprResult Res;
>>      while (true) {
>> -      Sema::SFINAETrap Trap(SemaRef);
>> -      res = TransformExpr(E);
>> -      error = Trap.hasErrorOccurred();
>> -
>> -      if (!(error || res.isInvalid()))
>> -        res = ExprFilter(res.get());
>> +      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 (!(error || res.isInvalid()) ||
>> +      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.
>> +    while (!AmbiguousTypoExprs.empty()) {
>> +      auto TE  = AmbiguousTypoExprs.back();
>> +      auto Cached = TransformCache[TE];
>> +      AmbiguousTypoExprs.pop_back();
>> +      TransformCache.erase(TE);
>> +      if (!TryTransform(E).isInvalid()) {
>> +        SemaRef.getTypoExprState(TE).Consumer->resetCorrectionStream();
>> +        TransformCache.erase(TE);
>> +        Res = ExprError();
>> +        break;
>> +      } else
>> +        TransformCache[TE] = Cached;
>> +    }
>> +
>>      // Ensure that all of the TypoExprs within the current Expr have
>> been found.
>> -    if (!res.isUsable())
>> +    if (!Res.isUsable())
>>        FindTypoExprs(TypoExprs).TraverseStmt(E);
>>
>>      EmitAllDiagnostics();
>>
>> -    return res;
>> +    return Res;
>>    }
>>
>>    ExprResult TransformTypoExpr(TypoExpr *E) {
>> @@ -6124,21 +6145,15 @@ public:
>>            State.RecoveryHandler(SemaRef, E, TC) :
>>            attemptRecovery(SemaRef, *State.Consumer, TC);
>>        if (!NE.isInvalid()) {
>> -        // Check whether there is a second viable correction with the
>> same edit
>> -        // distance--in which case do not suggest anything since both are
>> -        // equally good candidates for correcting the typo.
>> -        Sema::SFINAETrap LocalTrap(SemaRef);
>> +        // Check whether there may be a second viable correction with
>> the same
>> +        // edit distance; if so, remember this TypoExpr may have an
>> ambiguous
>> +        // correction so it can be more thoroughly vetted later.
>>          TypoCorrection Next;
>> -        while ((Next = State.Consumer->peekNextCorrection()) &&
>> -               Next.getEditDistance(false) == TC.getEditDistance(false))
>> {
>> -          ExprResult Res =
>> -              State.RecoveryHandler
>> -                  ? State.RecoveryHandler(SemaRef, E, Next)
>> -                  : attemptRecovery(SemaRef, *State.Consumer, Next);
>> -          if (!Res.isInvalid()) {
>> -            NE = ExprError();
>> -            State.Consumer->getNextCorrection();
>> -          }
>> +        if ((Next = State.Consumer->peekNextCorrection()) &&
>> +            Next.getEditDistance(false) == TC.getEditDistance(false)) {
>> +          AmbiguousTypoExprs.insert(E);
>> +        } else {
>> +          AmbiguousTypoExprs.remove(E);
>>          }
>>          assert(!NE.isUnset() &&
>>                 "Typo was transformed into a valid-but-null ExprResult");
>>
>> Modified: cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=222549&r1=222548&r2=222549&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp Fri Nov 21
>> 12:47:58 2014
>> @@ -48,3 +48,14 @@ void testNoCandidates() {
>>    callee(xxxxxx,   // expected-error-re {{use of undeclared identifier
>> 'xxxxxx'{{$}}}}
>>           zzzzzz);  // expected-error-re {{use of undeclared identifier
>> 'zzzzzz'{{$}}}}
>>  }
>> +
>> +class string {};
>> +struct Item {
>> +  void Nest();
>> +  string text();
>> +  Item* next();  // expected-note {{'next' declared here}}
>> +};
>> +void testExprFilter(Item *i) {
>> +  Item *j;
>> +  j = i->Next();  // expected-error {{no member named 'Next' in 'Item';
>> did you mean 'next'?}}
>> +}
>>
>
> I imagine this test should pass by picking the best candidate (the edit
> distance from Next -> next is shorter than Next->text or Next-Nest), right?
> If that's the case, is that not addressed (are we not picking the best
> candidate?)? Once it is, we might need a different test case to verify this
> tie handling - presumably a test where the best candidate has a tie?
>
> (but I'm guessing your example had something else to test - since only the
> return type of 'next' would make this compile - the other two return types
> don't match - what was that about? Clearly I'm missing something here)
>

The edit distance from Next to all three of the candidates is the same--all
are 1 char off--so this explicitly tests tie handling. Prior to this patch,
no suggestion would ever be offered because, without considering the
surrounding assignment expression, all three candidates are equally likely
(all are a one character change, all are members of the same struct, all
are methods that take zero arguments, all are accessible in the current
context, etc) so no suggestion would be given. This patch allows the
candidates to be narrowed down (in this case) based on the return type
needed to make the assignment expression valid (since two of the three
suggestions would create an invalid assignment, while the third works just
fine). And the order of the methods is chosen so that the first candidate
isn't being chosen by virtue of being first, as would happen with the
delayed typo correction prior to r222462.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141121/3e0b80a4/attachment.html>