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 @@ -7646,12 +7653,117 @@ } ExprResult TryTransform(Expr *E) { - Sema::SFINAETrap Trap(SemaRef); - ExprResult Res = TransformExpr(E); - if (Trap.hasErrorOccurred() || Res.isInvalid()) - return ExprError(); + return TryTransformExpr(E, ExprFilter); + } + + // 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 &IsAmbiguous) { + 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 = std::move(TypoExprs); + auto SavedAmbiguousTypoExprs = std::move(AmbiguousTypoExprs); + TypoExprs.clear(); + AmbiguousTypoExprs.clear(); + + 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, IsAmbiguous); + 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 = std::move(SavedTypoExprs); + AmbiguousTypoExprs = std::move(SavedAmbiguousTypoExprs); - return ExprFilter(Res.get()); + return Res; + } + + // Try to transform the given expression, looping through the correction + // candidates with `CheckAndAdvanceTypoExprCorrectionStreams`. + // + // If valid ambiguous typo corrections are seen, `IsAmbiguous` is set to + // true and this method immediately will return an `ExprError`. + ExprResult RecursiveTransformLoop(Expr *E, bool &IsAmbiguous) { + ExprResult Res; + while (true) { + Res = CheckForRecursiveTypos(TryTransform(E), IsAmbiguous); + + // Recursion encountered an ambiguous correction. This means that our + // correction itself is ambiguous, so stop now. + if (IsAmbiguous) + break; + + // 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; + } + + // If we found a valid result, double check to make sure it's not ambiguous. + if (!IsAmbiguous && !Res.isInvalid() && !AmbiguousTypoExprs.empty()) { + auto SavedTransformCache = std::move(TransformCache); + TransformCache.clear(); + // Ensure none of the TypoExprs have multiple typo correction candidates + // with the same edit length that pass all the checks and filters. + + while (!AmbiguousTypoExprs.empty()) { + auto TE = AmbiguousTypoExprs.back(); + + // TryTransform itself can create new Typos, adding them to the TypoExpr map + // and invalidating our TypoExprState, so always fetch it instead of storing. + SemaRef.getTypoExprState(TE).Consumer->saveCurrentPosition(); + + TypoCorrection TC = SemaRef.getTypoExprState(TE).Consumer->peekNextCorrection(); + TypoCorrection Next; + do { + ExprResult AmbigRes = CheckForRecursiveTypos(TryTransform(E), IsAmbiguous); + + if (!AmbigRes.isInvalid() || IsAmbiguous) { + SemaRef.getTypoExprState(TE).Consumer->resetCorrectionStream(); + SavedTransformCache.erase(TE); + Res = ExprError(); + IsAmbiguous = true; + break; + } + } while ((Next = SemaRef.getTypoExprState(TE).Consumer->peekNextCorrection()) && + Next.getEditDistance(false) == TC.getEditDistance(false)); + + if (IsAmbiguous) + break; + + AmbiguousTypoExprs.remove(TE); + SemaRef.getTypoExprState(TE).Consumer->restoreSavedPosition(); + } + TransformCache = std::move(SavedTransformCache); + } + return Res; } public: @@ -7681,49 +7793,13 @@ 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.. - 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); - // Ensure that all of the TypoExprs within the current Expr have been found. if (!Res.isUsable()) FindTypoExprs(TypoExprs).TraverseStmt(E); - EmitAllDiagnostics(); + EmitAllDiagnostics(IsAmbiguous); return Res; } 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. TreeTransform(Sema &SemaRef) : SemaRef(SemaRef) { } @@ -372,6 +392,14 @@ const X##Attr *Transform##X##Attr(const X##Attr *R) { return R; } #include "clang/Basic/AttrList.inc" + /// Try to transform the given expression, catching any traps. After + /// successfully applying the transformation, apply the filter. + /// + /// Any TypoExprs created during an invalid transformation will automatically + /// be cleared. + ExprResult TryTransformExpr(Expr *E, + llvm::function_ref Filter); + /// Transform the given expression. /// /// By default, this routine transforms an expression by delegating to the @@ -379,6 +407,9 @@ /// Subclasses may override this function to transform expressions using some /// other mechanism. /// + /// This method should also handle clearing any TypoExprs that are created + /// from an invalid transformation (see `TypoExprs`). + /// /// \returns the transformed expression. ExprResult TransformExpr(Expr *E); @@ -3394,9 +3425,8 @@ return S; } - template -ExprResult TreeTransform::TransformExpr(Expr *E) { +ExprResult TreeTransform::TransformExprWrapped(Expr *E) { if (!E) return E; @@ -3412,6 +3442,66 @@ return E; } +template +ExprResult TreeTransform::TryTransformExpr(Expr *E, + llvm::function_ref Filter) { + Sema::SFINAETrap Trap(SemaRef); + + auto StartSize = TypoExprs.size(); + ExprResult Res = getDerived().TransformExpr(E); + if (Trap.hasErrorOccurred() || Res.isInvalid()) + Res = ExprError(); + else + Res = Filter(Res.get()); + + // Check if the final result is invalid; if so, clear any TypoExprs that have + // been created recursively. + // + // Note that there's no check for a `TypoExpr` + // result here as `TransformExpr` should have handled that case assuming our + // filter doesn't do anything wonky (return a different `Expr`). + // + // This is needed in addition to the checks in `TransformExpr` since an error + // may have occurred or the filter might have returned an invalid result. + if (Res.isInvalid()) { + for (auto Iterator = TypoExprs.begin() + StartSize; + Iterator != TypoExprs.end();) { + SemaRef.clearDelayedTypo(*Iterator); + Iterator = TypoExprs.erase(Iterator); + } + } + + return Res; +} + +template +ExprResult TreeTransform::TransformExpr(Expr *E) { + if (!E) + return E; + + auto StartSize = TypoExprs.size(); + ExprResult Res = getDerived().TransformExprWrapped(E); + + // 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()) { + Expr *TransformedExpr = Res.get(); + if (TypoExpr::classof(TransformedExpr)) { + TypoExprs.insert(reinterpret_cast(TransformedExpr)); + } + } else { + for (auto Iterator = TypoExprs.begin() + StartSize; + Iterator != TypoExprs.end();) { + SemaRef.clearDelayedTypo(*Iterator); + Iterator = TypoExprs.erase(Iterator); + } + } + + return Res; +} + template ExprResult TreeTransform::TransformInitializer(Expr *Init, bool NotCopyInit) { 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(); +}