Remove delayed typo expressions #143423
Merged
Remove delayed typo expressions #143423
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This corrects non-delayed typos more aggressively which improves diagnostic output for the most part, but did expose a few more crashes
There are some tests failing still, that's expected
It's possible these tests could be repaired but AST dump tests are incredibly fragile to begin with. I think removing these tests is not actively harmful.
This test requires a RecoveryExpr and typos no longer cause a recovery expression to be created. So the expression was changed to one which does have a RecoveryExpr.
AaronBallman
requested review from
rnk,
cor3ntin,
shafik,
erichkeane and
efriedma-quic
AaronBallman
added
clang
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesThis removes the delayed typo correction functionality from Clang (regular typo correction still remains) due to fragility of the solution. An RFC was posted here: https://discourse.llvm.org/t/rfc-removing-support-for-delayed-typo-correction/86631 and while that RFC was asking for folks to consider stepping up to be maintainers, and we did have a few new contributors show some interest, experiments show that it's likely worth it to remove this functionality entirely and focus efforts on improving regular typo correction. This removal fixes ~20 open issues (quite possibly more), improves compile time performance by roughly .3-.4% (https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=AaronBallman&sortBy=date), and does not appear to regress diagnostic behavior in a way we wouldn't find acceptable. Fixes #142457 Patch is 232.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143423.diff 114 Files Affected:
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 43c28c8bf649f..9fc23d30b733f 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -240,8 +240,7 @@ class Expr : public ValueStmt {
return static_cast<bool>(getDependence() & ExprDependence::UnexpandedPack);
}
- /// Whether this expression contains subexpressions which had errors, e.g. a
- /// TypoExpr.
+ /// Whether this expression contains subexpressions which had errors.
bool containsErrors() const {
return static_cast<bool>(getDependence() & ExprDependence::Error);
}
@@ -6965,36 +6964,6 @@ class AtomicExpr : public Expr {
}
};
-/// TypoExpr - Internal placeholder for expressions where typo correction
-/// still needs to be performed and/or an error diagnostic emitted.
-class TypoExpr : public Expr {
- // The location for the typo name.
- SourceLocation TypoLoc;
-
-public:
- TypoExpr(QualType T, SourceLocation TypoLoc)
- : Expr(TypoExprClass, T, VK_LValue, OK_Ordinary), TypoLoc(TypoLoc) {
- assert(T->isDependentType() && "TypoExpr given a non-dependent type");
- setDependence(ExprDependence::TypeValueInstantiation |
- ExprDependence::Error);
- }
-
- child_range children() {
- return child_range(child_iterator(), child_iterator());
- }
- const_child_range children() const {
- return const_child_range(const_child_iterator(), const_child_iterator());
- }
-
- SourceLocation getBeginLoc() const LLVM_READONLY { return TypoLoc; }
- SourceLocation getEndLoc() const LLVM_READONLY { return TypoLoc; }
-
- static bool classof(const Stmt *T) {
- return T->getStmtClass() == TypoExprClass;
- }
-
-};
-
/// This class represents BOTH the OpenMP Array Section and OpenACC 'subarray',
/// with a boolean differentiator.
/// OpenMP 5.0 [2.1.5, Array Sections].
diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index b0f8ae621cf6d..5cb2f57edffe4 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2956,7 +2956,6 @@ DEF_TRAVERSE_STMT(CXXRewrittenBinaryOperator, {
}
})
DEF_TRAVERSE_STMT(OpaqueValueExpr, {})
-DEF_TRAVERSE_STMT(TypoExpr, {})
DEF_TRAVERSE_STMT(RecoveryExpr, {})
DEF_TRAVERSE_STMT(CUDAKernelCallExpr, {})
diff --git a/clang/include/clang/Basic/StmtNodes.td b/clang/include/clang/Basic/StmtNodes.td
index 9526fa5808aa5..c9c173f5c7469 100644
--- a/clang/include/clang/Basic/StmtNodes.td
+++ b/clang/include/clang/Basic/StmtNodes.td
@@ -202,7 +202,6 @@ def ShuffleVectorExpr : StmtNode<Expr>;
def ConvertVectorExpr : StmtNode<Expr>;
def BlockExpr : StmtNode<Expr>;
def OpaqueValueExpr : StmtNode<Expr>;
-def TypoExpr : StmtNode<Expr>;
def RecoveryExpr : StmtNode<Expr>;
def BuiltinBitCastExpr : StmtNode<ExplicitCastExpr>;
def EmbedExpr : StmtNode<Expr>;
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 0b2fab4a45c96..5a3941556d9d0 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -4171,8 +4171,7 @@ class Parser : public CodeCompletionHandler {
bool ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
llvm::function_ref<void()> ExpressionStarts =
llvm::function_ref<void()>(),
- bool FailImmediatelyOnInvalidExpr = false,
- bool EarlyTypoCorrection = false);
+ bool FailImmediatelyOnInvalidExpr = false);
/// ParseSimpleExpressionList - A simple comma-separated list of expressions,
/// used for misc language extensions.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f9a086b6966d9..15be7620f9ff4 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -6713,10 +6713,6 @@ class Sema final : public SemaBase {
/// this expression evaluation context.
unsigned NumCleanupObjects;
- /// The number of typos encountered during this expression evaluation
- /// context (i.e. the number of TypoExprs created).
- unsigned NumTypos;
-
MaybeODRUseExprSet SavedMaybeODRUseExprs;
/// The lambdas that are present within this context, if it
@@ -6813,7 +6809,7 @@ class Sema final : public SemaBase {
Decl *ManglingContextDecl,
ExpressionKind ExprContext)
: Context(Context), ParentCleanup(ParentCleanup),
- NumCleanupObjects(NumCleanupObjects), NumTypos(0),
+ NumCleanupObjects(NumCleanupObjects),
ManglingContextDecl(ManglingContextDecl), ExprContext(ExprContext),
InDiscardedStatement(false), InImmediateFunctionContext(false),
InImmediateEscalatingFunctionContext(false) {}
@@ -7146,8 +7142,7 @@ class Sema final : public SemaBase {
CorrectionCandidateCallback &CCC,
TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr,
ArrayRef<Expr *> Args = {},
- DeclContext *LookupCtx = nullptr,
- TypoExpr **Out = nullptr);
+ DeclContext *LookupCtx = nullptr);
/// If \p D cannot be odr-used in the current expression evaluation context,
/// return a reason explaining why. Otherwise, return NOUR_None.
@@ -8747,40 +8742,6 @@ class Sema final : public SemaBase {
ExprResult CheckUnevaluatedOperand(Expr *E);
- /// Process any TypoExprs in the given Expr and its children,
- /// generating diagnostics as appropriate and returning a new Expr if there
- /// were typos that were all successfully corrected and ExprError if one or
- /// more typos could not be corrected.
- ///
- /// \param E The Expr to check for TypoExprs.
- ///
- /// \param InitDecl A VarDecl to avoid because the Expr being corrected is its
- /// initializer.
- ///
- /// \param RecoverUncorrectedTypos If true, when typo correction fails, it
- /// will rebuild the given Expr with all TypoExprs degraded to RecoveryExprs.
- ///
- /// \param Filter A function applied to a newly rebuilt Expr to determine if
- /// it is an acceptable/usable result from a single combination of typo
- /// corrections. As long as the filter returns ExprError, different
- /// combinations of corrections will be tried until all are exhausted.
- ExprResult CorrectDelayedTyposInExpr(
- Expr *E, VarDecl *InitDecl = nullptr,
- bool RecoverUncorrectedTypos = false,
- llvm::function_ref<ExprResult(Expr *)> Filter =
- [](Expr *E) -> ExprResult { return E; });
-
- ExprResult CorrectDelayedTyposInExpr(
- ExprResult ER, VarDecl *InitDecl = nullptr,
- bool RecoverUncorrectedTypos = false,
- llvm::function_ref<ExprResult(Expr *)> Filter =
- [](Expr *E) -> ExprResult { return E; }) {
- return ER.isInvalid()
- ? ER
- : CorrectDelayedTyposInExpr(ER.get(), InitDecl,
- RecoverUncorrectedTypos, Filter);
- }
-
IfExistsResult
CheckMicrosoftIfExistsSymbol(Scope *S, CXXScopeSpec &SS,
const DeclarationNameInfo &TargetNameInfo);
@@ -9282,12 +9243,6 @@ class Sema final : public SemaBase {
/// for C++ records.
llvm::FoldingSet<SpecialMemberOverloadResultEntry> SpecialMemberCache;
- /// Holds TypoExprs that are created from `createDelayedTypo`. This is used by
- /// `TransformTypos` in order to keep track of any TypoExprs that are created
- /// recursively during typo correction and wipe them away if the correction
- /// fails.
- llvm::SmallVector<TypoExpr *, 2> TypoExprs;
-
enum class AcceptableKind { Visible, Reachable };
// Members have to be NamespaceDecl* or TranslationUnitDecl*.
@@ -9375,10 +9330,6 @@ class Sema final : public SemaBase {
bool VolatileArg, bool RValueThis, bool ConstThis,
bool VolatileThis);
- typedef std::function<void(const TypoCorrection &)> TypoDiagnosticGenerator;
- typedef std::function<ExprResult(Sema &, TypoExpr *, TypoCorrection)>
- TypoRecoveryCallback;
-
RedeclarationKind forRedeclarationInCurContext() const;
/// Look up a name, looking for a single declaration. Return
@@ -9732,51 +9683,6 @@ class Sema final : public SemaBase {
const ObjCObjectPointerType *OPT = nullptr,
bool RecordFailure = true);
- /// Try to "correct" a typo in the source code by finding
- /// visible declarations whose names are similar to the name that was
- /// present in the source code.
- ///
- /// \param TypoName the \c DeclarationNameInfo structure that contains
- /// the name that was present in the source code along with its location.
- ///
- /// \param LookupKind the name-lookup criteria used to search for the name.
- ///
- /// \param S the scope in which name lookup occurs.
- ///
- /// \param SS the nested-name-specifier that precedes the name we're
- /// looking for, if present.
- ///
- /// \param CCC A CorrectionCandidateCallback object that provides further
- /// validation of typo correction candidates. It also provides flags for
- /// determining the set of keywords permitted.
- ///
- /// \param TDG A TypoDiagnosticGenerator functor that will be used to print
- /// diagnostics when the actual typo correction is attempted.
- ///
- /// \param TRC A TypoRecoveryCallback functor that will be used to build an
- /// Expr from a typo correction candidate.
- ///
- /// \param MemberContext if non-NULL, the context in which to look for
- /// a member access expression.
- ///
- /// \param EnteringContext whether we're entering the context described by
- /// the nested-name-specifier SS.
- ///
- /// \param OPT when non-NULL, the search for visible declarations will
- /// also walk the protocols in the qualified interfaces of \p OPT.
- ///
- /// \returns a new \c TypoExpr that will later be replaced in the AST with an
- /// Expr representing the result of performing typo correction, or nullptr if
- /// typo correction is not possible. If nullptr is returned, no diagnostics
- /// will be emitted and it is the responsibility of the caller to emit any
- /// that are needed.
- TypoExpr *CorrectTypoDelayed(
- const DeclarationNameInfo &Typo, Sema::LookupNameKind LookupKind,
- Scope *S, CXXScopeSpec *SS, CorrectionCandidateCallback &CCC,
- TypoDiagnosticGenerator TDG, TypoRecoveryCallback TRC,
- CorrectTypoKind Mode, DeclContext *MemberContext = nullptr,
- bool EnteringContext = false, const ObjCObjectPointerType *OPT = nullptr);
-
/// Kinds of missing import. Note, the values of these enumerators correspond
/// to %select values in diagnostics.
enum class MissingImportKind {
@@ -9795,20 +9701,6 @@ class Sema final : public SemaBase {
SourceLocation DeclLoc, ArrayRef<Module *> Modules,
MissingImportKind MIK, bool Recover);
- struct TypoExprState {
- std::unique_ptr<TypoCorrectionConsumer> Consumer;
- TypoDiagnosticGenerator DiagHandler;
- TypoRecoveryCallback RecoveryHandler;
- TypoExprState();
- TypoExprState(TypoExprState &&other) noexcept;
- TypoExprState &operator=(TypoExprState &&other) noexcept;
- };
-
- const TypoExprState &getTypoExprState(TypoExpr *TE) const;
-
- /// Clears the state of the given TypoExpr.
- void clearDelayedTypo(TypoExpr *TE);
-
/// Called on #pragma clang __debug dump II
void ActOnPragmaDump(Scope *S, SourceLocation Loc, IdentifierInfo *II);
@@ -9831,23 +9723,15 @@ class Sema final : public SemaBase {
/// Determine if we could use all the declarations in the module.
bool isUsableModule(const Module *M);
- /// Helper for CorrectTypo and CorrectTypoDelayed used to create and
- /// populate a new TypoCorrectionConsumer. Returns nullptr if typo correction
- /// should be skipped entirely.
+ /// Helper for CorrectTypo used to create and populate a new
+ /// TypoCorrectionConsumer. Returns nullptr if typo correction should be
+ /// skipped entirely.
std::unique_ptr<TypoCorrectionConsumer> makeTypoCorrectionConsumer(
const DeclarationNameInfo &Typo, Sema::LookupNameKind LookupKind,
Scope *S, CXXScopeSpec *SS, CorrectionCandidateCallback &CCC,
DeclContext *MemberContext, bool EnteringContext,
const ObjCObjectPointerType *OPT, bool ErrorRecovery);
- /// The set of unhandled TypoExprs and their associated state.
- llvm::MapVector<TypoExpr *, TypoExprState> DelayedTypos;
-
- /// Creates a new TypoExpr AST node.
- TypoExpr *createDelayedTypo(std::unique_ptr<TypoCorrectionConsumer> TCC,
- TypoDiagnosticGenerator TDG,
- TypoRecoveryCallback TRC, SourceLocation TypoLoc);
-
/// Cache for module units which is usable for current module.
llvm::DenseSet<const Module *> UsableModuleUnitsCache;
diff --git a/clang/include/clang/Sema/SemaInternal.h b/clang/include/clang/Sema/SemaInternal.h
index 95874077050a9..4d0da1102bb59 100644
--- a/clang/include/clang/Sema/SemaInternal.h
+++ b/clang/include/clang/Sema/SemaInternal.h
@@ -314,20 +314,6 @@ class TypoCorrectionConsumer : public VisibleDeclConsumer {
bool SearchNamespaces;
};
-inline Sema::TypoExprState::TypoExprState() {}
-
-inline Sema::TypoExprState::TypoExprState(TypoExprState &&other) noexcept {
- *this = std::move(other);
-}
-
-inline Sema::TypoExprState &Sema::TypoExprState::
-operator=(Sema::TypoExprState &&other) noexcept {
- Consumer = std::move(other.Consumer);
- DiagHandler = std::move(other.DiagHandler);
- RecoveryHandler = std::move(other.RecoveryHandler);
- return *this;
-}
-
} // end namespace clang
#endif
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 17d2cb4a30f30..c3722c65abf6e 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3611,7 +3611,6 @@ bool Expr::HasSideEffects(const ASTContext &Ctx,
case PackExpansionExprClass:
case SubstNonTypeTemplateParmPackExprClass:
case FunctionParmPackExprClass:
- case TypoExprClass:
case RecoveryExprClass:
case CXXFoldExprClass:
// Make a conservative assumption for dependent nodes.
diff --git a/clang/lib/AST/ExprClassification.cpp b/clang/lib/AST/ExprClassification.cpp
index 3f37d06cc8f3a..ad66335138a42 100644
--- a/clang/lib/AST/ExprClassification.cpp
+++ b/clang/lib/AST/ExprClassification.cpp
@@ -129,7 +129,6 @@ static Cl::Kinds ClassifyInternal(ASTContext &Ctx, const Expr *E) {
// FIXME: Is this wise? Should they get their own kind?
case Expr::UnresolvedLookupExprClass:
case Expr::UnresolvedMemberExprClass:
- case Expr::TypoExprClass:
case Expr::DependentCoawaitExprClass:
case Expr::CXXDependentScopeMemberExprClass:
case Expr::DependentScopeDeclRefExprClass:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index fa4e10e84de05..5ff2a7c1b9b38 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -17326,7 +17326,6 @@ static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
case Expr::CXXDeleteExprClass:
case Expr::CXXPseudoDestructorExprClass:
case Expr::UnresolvedLookupExprClass:
- case Expr::TypoExprClass:
case Expr::RecoveryExprClass:
case Expr::DependentScopeDeclRefExprClass:
case Expr::CXXConstructExprClass:
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index ecf5be220439b..487933a748ab8 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -4994,7 +4994,6 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
case Expr::ParenListExprClass:
case Expr::MSPropertyRefExprClass:
case Expr::MSPropertySubscriptExprClass:
- case Expr::TypoExprClass: // This should no longer exist in the AST by now.
case Expr::RecoveryExprClass:
case Expr::ArraySectionExprClass:
case Expr::OMPArrayShapingExprClass:
diff --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp
index 13c3bc0387890..28317911d825b 100644
--- a/clang/lib/AST/StmtPrinter.cpp
+++ b/clang/lib/AST/StmtPrinter.cpp
@@ -2914,11 +2914,6 @@ void StmtPrinter::VisitOpaqueValueExpr(OpaqueValueExpr *Node) {
PrintExpr(Node->getSourceExpr());
}
-void StmtPrinter::VisitTypoExpr(TypoExpr *Node) {
- // TODO: Print something reasonable for a TypoExpr, if necessary.
- llvm_unreachable("Cannot print TypoExpr nodes");
-}
-
void StmtPrinter::VisitRecoveryExpr(RecoveryExpr *Node) {
OS << "<recovery-expr>(";
const char *Sep = "";
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index f7d1655f67ed1..c666d966a6e58 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2361,10 +2361,6 @@ void StmtProfiler::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
VisitExpr(E);
}
-void StmtProfiler::VisitTypoExpr(const TypoExpr *E) {
- VisitExpr(E);
-}
-
void StmtProfiler::VisitSourceLocExpr(const SourceLocExpr *E) {
VisitExpr(E);
}
diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp
index e215c64cccd11..9a010fb5f3427 100644
--- a/clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -422,7 +422,6 @@ void Parser::ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM) {
DefArgResult = ParseBraceInitializer();
} else
DefArgResult = ParseAssignmentExpression();
- DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult, Param);
if (DefArgResult.isInvalid()) {
Actions.ActOnParamDefaultArgumentError(Param, EqualLoc,
/*DefaultArg=*/nullptr);
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index f469e466e4634..647ee34efcabc 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -436,7 +436,6 @@ bool Parser::ParseAttributeArgumentList(
} else {
Expr = ParseAssignmentExpression();
}
- Expr = Actions.CorrectDelayedTyposInExpr(Expr);
if (Tok.is(tok::ellipsis))
Expr = Actions.ActOnPackExpansion(Expr.get(), ConsumeToken());
@@ -472,15 +471,6 @@ bool Parser::ParseAttributeArgumentList(
Arg++;
}
- if (SawError) {
- // Ensure typos get diagnosed when errors were encountered while parsing the
- // expression list.
- for (auto &E : Exprs) {
- ExprResult Expr = Actions.CorrectDelayedTyposInExpr(E);
- if (Expr.isUsable())
- E = Expr.get();
- }
- }
return SawError;
}
@@ -565,9 +555,7 @@ unsigned Parser::ParseAttributeArgsCommon(
nullptr,
Sema::ExpressionEvaluationContextRecord::EK_AttrArgument);
- ExprResult ArgExpr(
- Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
-
+ ExprResult ArgExpr = ParseAssignmentExpression();
if (ArgExpr.isInvalid()) {
SkipUntil(tok::r_paren, StopAtSemi);
return 0;
@@ -3212,9 +3200,7 @@ void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName,
Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, nullptr,
ExpressionKind::EK_AttrArgument);
- ExprResult ArgExpr(
- Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
-
+ ExprResult ArgExpr = ParseAssignmentExpression();
if (ArgExpr.isInvalid()) {
Parens.skipToEnd();
return;
@@ -6890,8 +6876,8 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
// void (f()) requires true;
Diag(Tok, diag::err_requires_clause_inside_parens);
ConsumeToken();
- ExprResult TrailingRequiresClause = Actions.CorrectDelayedTyposInExpr(
- ParseConstraintLogicalOrExpression(/*IsTrailingRequiresClause=*/true));
+ ExprResult TrailingRequiresClause =
+ ParseConstraintLogicalOrExpression(/*IsTrailingRequiresClause=*/true);
if (TrailingRequiresClause.isUsable() && D.isFunctionDeclarator() &&
!D.hasTrailingRequiresClause())
// We're already ill-formed if we got here but we'll accept it anyway.
@@ -7538,8 +7524,7 @@ void Parser::ParseParameterDeclarationClause(
Diag(Tok,
diag::err_requires_clause_on_declarator_not_de...
[truncated]
|
|
Posting the PR for review, but also because I'd like to see what precommit CI thinks of the changes on platforms other than the one I'm on (Windows). |
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cppm,c,cpp,cl,h -- clang/test/Sema/c23-delayed-typo-correction-crashes.c clang/test/Sema/delayed-typo-correction-crashes.c clang/test/SemaCXX/cxx-delayed-typo-correction-crashes.cpp clang/test/SemaCXX/cxx20-delayed-typo-correction-crashes.cpp clang-tools-extra/clangd/unittests/HoverTests.cpp clang/include/clang/AST/Expr.h clang/include/clang/AST/RecursiveASTVisitor.h clang/include/clang/Parse/Parser.h clang/include/clang/Sema/Sema.h clang/include/clang/Sema/SemaInternal.h clang/lib/AST/Expr.cpp clang/lib/AST/ExprClassification.cpp clang/lib/AST/ExprConstant.cpp clang/lib/AST/ItaniumMangle.cpp clang/lib/AST/StmtPrinter.cpp clang/lib/AST/StmtProfile.cpp clang/lib/Parse/ParseCXXInlineMethods.cpp clang/lib/Parse/ParseDecl.cpp clang/lib/Parse/ParseDeclCXX.cpp clang/lib/Parse/ParseExpr.cpp clang/lib/Parse/ParseExprCXX.cpp clang/lib/Parse/ParseInit.cpp clang/lib/Parse/ParseObjc.cpp clang/lib/Parse/ParseOpenACC.cpp clang/lib/Parse/ParseOpenMP.cpp clang/lib/Parse/ParseStmt.cpp clang/lib/Parse/ParseStmtAsm.cpp clang/lib/Parse/ParseTemplate.cpp clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaChecking.cpp clang/lib/Sema/SemaCoroutine.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaDeclCXX.cpp clang/lib/Sema/SemaExceptionSpec.cpp clang/lib/Sema/SemaExpr.cpp clang/lib/Sema/SemaExprCXX.cpp clang/lib/Sema/SemaExprMember.cpp clang/lib/Sema/SemaLookup.cpp clang/lib/Sema/SemaObjC.cpp clang/lib/Sema/SemaOverload.cpp clang/lib/Sema/SemaStmt.cpp clang/lib/Sema/SemaStmtAttr.cpp clang/lib/Sema/SemaTemplateVariadic.cpp clang/lib/Sema/TreeTransform.h clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/lib/StaticAnalyzer/Core/ExprEngine.cpp clang/test/AST/ByteCode/literals.cpp clang/test/AST/ast-dump-recovery.c clang/test/AST/ast-dump-recovery.cpp clang/test/CXX/drs/cwg1xx.cpp clang/test/CXX/drs/cwg26xx.cpp clang/test/CXX/module/basic/basic.link/p2.cppm clang/test/Index/fix-its.c clang/test/Lexer/raw-string-ext.c clang/test/OpenMP/begin_declare_variant_messages.c clang/test/OpenMP/declare_reduction_messages.cpp clang/test/OpenMP/declare_variant_messages.c clang/test/OpenMP/declare_variant_messages.cpp clang/test/OpenMP/target_update_messages.cpp clang/test/Parser/cxx1z-decomposition.cpp clang/test/Parser/cxx1z-fold-expressions.cpp clang/test/Parser/cxx2c-pack-indexing.cpp clang/test/Parser/opencl-atomics-cl20.cl clang/test/Parser/recovery.c clang/test/Parser/switch-recovery.cpp clang/test/Parser/switch-typo-correction.cpp clang/test/ParserOpenACC/parse-cache-construct.cpp clang/test/ParserOpenACC/parse-clauses.c clang/test/ParserOpenACC/parse-constructs.cpp clang/test/ParserOpenACC/parse-wait-clause.c clang/test/ParserOpenACC/parse-wait-construct.c clang/test/Sema/PR28181.c clang/test/Sema/builtin-unary-fp.c clang/test/Sema/invalid-member.cpp clang/test/Sema/typo-correction-ambiguity.cpp clang/test/Sema/typo-correction-no-hang.c clang/test/Sema/typo-correction-no-hang.cpp clang/test/Sema/typo-correction-recursive.cpp clang/test/Sema/typo-correction.c clang/test/SemaCXX/arrow-operator.cpp clang/test/SemaCXX/constant-expression-cxx11.cpp clang/test/SemaCXX/conversion-function.cpp clang/test/SemaCXX/coroutines.cpp clang/test/SemaCXX/cxx1z-decomposition.cpp clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp clang/test/SemaCXX/destructor.cpp clang/test/SemaCXX/invalid-if-constexpr.cpp clang/test/SemaCXX/member-expr.cpp clang/test/SemaCXX/nested-name-spec.cpp clang/test/SemaCXX/return.cpp clang/test/SemaCXX/typo-correction-crash.cpp clang/test/SemaCXX/typo-correction-cxx11.cpp clang/test/SemaCXX/typo-correction.cpp clang/test/SemaCXX/virtuals.cpp clang/test/SemaOpenACC/compute-construct-num_gangs-clause.cpp clang/test/SemaOpenCL/atomic-ops.cl clang/test/SemaOpenCL/clang-builtin-version.cl clang/test/SemaTemplate/concepts-recovery-expr.cpp clang/test/SemaTemplate/concepts.cpp clang/test/SemaTemplate/typo-variadic.cpp clang/tools/libclang/CXCursor.cpp clang/unittests/Sema/ExternalSemaSourceTest.cppView the diff from clang-format here.diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
index 8ef16a4d3..1fb011259 100644
--- a/clang/lib/Parse/ParseObjc.cpp
+++ b/clang/lib/Parse/ParseObjc.cpp
@@ -3098,7 +3098,7 @@ ExprResult Parser::ParseObjCArrayLiteral(SourceLocation AtLoc) {
ExprResult Parser::ParseObjCDictionaryLiteral(SourceLocation AtLoc) {
SmallVector<ObjCDictionaryElement, 4> Elements; // dictionary elements.
- ConsumeBrace(); // consume the l_square.
+ ConsumeBrace(); // consume the l_square.
while (Tok.isNot(tok::r_brace)) {
// Parse the comma separated key : value expressions.
ExprResult KeyExpr;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index b7031bc8c..6d6b14aae 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8906,9 +8906,8 @@ static QualType computeConditionalNullability(QualType ResTy, bool IsBin,
}
ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
- SourceLocation ColonLoc,
- Expr *CondExpr, Expr *LHSExpr,
- Expr *RHSExpr) {
+ SourceLocation ColonLoc, Expr *CondExpr,
+ Expr *LHSExpr, Expr *RHSExpr) {
// If this is the gnu "x ?: y" extension, analyze the types as though the LHS
// was the condition.
OpaqueValueExpr *opaqueValue = nullptr;
@@ -18067,7 +18066,7 @@ HandleImmediateInvocations(Sema &SemaRef,
}
void Sema::PopExpressionEvaluationContext() {
- ExpressionEvaluationContextRecord& Rec = ExprEvalContexts.back();
+ ExpressionEvaluationContextRecord &Rec = ExprEvalContexts.back();
if (!Rec.Lambdas.empty()) {
using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
if (!getLangOpts().CPlusPlus20 &&
diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index 5dca509d4..f63d38bf1 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -696,7 +696,7 @@ ExprResult Sema::BuildMemberReferenceExpr(
SS, TemplateArgs != nullptr, TemplateKWLoc))
return ExprError();
- // Explicit member accesses.
+ // Explicit member accesses.
} else {
ExprResult BaseResult = Base;
ExprResult Result =
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 572dbf2e7..c223dd574 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -740,7 +740,7 @@ ExprResult Sema::CheckPackExpansion(Expr *Pattern, SourceLocation EllipsisLoc,
// expansion.
if (!Pattern->containsUnexpandedParameterPack()) {
Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
- << Pattern->getSourceRange();
+ << Pattern->getSourceRange();
return ExprError();
}
|
erichkeane
approved these changes
Jun 9, 2025
| @@ -58,10 +58,8 @@ struct Base { | |||
| }; | |||
|
|
|||
| struct Derived final : Base { | |||
| virtual ~Derived() = defaul; // #default | |||
| virtual ~Derived() = defaul; // expected-error {{use of undeclared identifier 'defaul'}} | |||
There was a problem hiding this comment.
I would sort of expect us to try to do typo correction here: we know we're expecting one of a few keywords. Maybe worth filing a bug,
There was a problem hiding this comment.
Agreed: #143509
| }; | ||
| int test(Foo f) { | ||
| if (UsesMetadata) // expected-error-re {{use of undeclared identifier 'UsesMetadata'{{$}}}} | ||
| return 5; | ||
| if (f.UsesMetadata) // expected-error {{no member named 'UsesMetadata' in 'testNonStaticMemberHandling::Foo'; did you mean 'usesMetadata'?}} | ||
| if (f.UsesMetadata) // expected-error {{no member named 'UsesMetadata' in 'testNonStaticMemberHandling::Foo'}} |
There was a problem hiding this comment.
Why are we losing typo correction here?
There was a problem hiding this comment.
Same as above; it was relying on delayed typo correction for some reason.
| @@ -1,29 +0,0 @@ | |||
| // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s | |||
| // Don't crash (PR13394). | |||
There was a problem hiding this comment.
Is this just not worth keeping because it's strongly connected to delayed typo correction? Or did this move?
There was a problem hiding this comment.
It only existed for testing a crash with delayed typo correction. When I looked at correcting the diagnostics for the test, it seemed better to just drop the test because it wasn't really testing anything we don't already cover elsewhere.
| @@ -207,7 +207,7 @@ namespace PR15045 { | |||
|
|
|||
| // Show that recovery has happened by also triggering typo correction | |||
| e->Func(); // expected-error {{member reference type 'bar' is not a pointer; did you mean to use '.'?}} \ | |||
| // expected-error {{no member named 'Func' in 'PR15045::bar'; did you mean 'func'?}} | |||
There was a problem hiding this comment.
Why are we losing typo correction here?
There was a problem hiding this comment.
It's one of the cases where we were relying on delayed typo correction for the typo; not certain why though. We lost a few such diagnostics that we'll want to recover someday.
| @@ -1888,10 +1888,11 @@ namespace PR15884 { | |||
| } | |||
|
|
|||
| namespace AfterError { | |||
| constexpr int error() { | |||
| constexpr int error() { // pre-cxx23-error {{no return statement in constexpr function}} | |||
There was a problem hiding this comment.
It would be nice to suppress this diagnostic: if we discard a "return" statement, we shouldn't be complaining that constexpr eval of the function fails.
There was a problem hiding this comment.
Agreed, I've filed an issue for a few situations you've brought up in this PR plus some I'm aware of.
There was a problem hiding this comment.
Did you mean to refer to some other issue report? The issue here isn't typo handling, really; it's that we're discarding the "return" without noting it anywhere.
There was a problem hiding this comment.
Hmm okay, I didn't write it in the issue but I was thinking that suppression of follow-on diagnostics is also about the context of the typo (if typo correction results in us dropping a statement, we should suppress follow-on diagnostics about the dropped statement). But you're right, this is orthogonal, it's more general about error recovery. We previously would not drop the return statement here but would instead give it a RecoveryExpr for its operand, that no longer happens.
I've filed #143688 on the assumption we're removing delayed typos so this will eventually be true. :-D
shafik
reviewed
Jun 10, 2025
Sirraide
approved these changes
Jun 10, 2025
Absolutely correct. :-D |
|
Linux precommit CI failures appear to be unrelated: |
kadircet
reviewed
Jun 12, 2025
| @@ -974,7 +974,7 @@ class Foo final {})cpp"; | |||
| HI.Name = "abc"; | |||
| HI.Kind = index::SymbolKind::Variable; | |||
| HI.NamespaceScope = ""; | |||
| HI.Definition = "int abc = <recovery - expr>()"; | |||
There was a problem hiding this comment.
despite typoexprs fragile and complicated nature, i think they were providing most of their value by preserving more nodes in the AST, that can later on be used by source tools (especially for more error correction :)).
I don't think we need TypoExprs for most of this, if possible, preserving these TypoExpr nodes through RecoveryExprs would actually help a lot with preserving current behavior of tools but also shape of the AST for any other application.
I wouldn't block on this though, and wouldn't be surprised if this isn't trivial, we don't seem to be preserving typoexprs as recovery-exprs most of the time and I am not sure why.
There was a problem hiding this comment.
Yeah, making a RecoveryExpr in these cases would be helpful (though potentially nontrivial). There was another case where we dropped a ReturnStmt rather than giving it a RecoveryExpr operand, and that caused follow-on diagnostics that were less than helpful.
|
@efriedma-quic and I spoke yesterday about this and he said he thinks it's good to go, so landing the changes now. If there are further concerns, we can address them post-commit. |
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
This removes the delayed typo correction functionality from Clang (regular typo correction still remains) due to fragility of the solution. An RFC was posted here: https://discourse.llvm.org/t/rfc-removing-support-for-delayed-typo-correction/86631 and while that RFC was asking for folks to consider stepping up to be maintainers, and we did have a few new contributors show some interest, experiments show that it's likely worth it to remove this functionality entirely and focus efforts on improving regular typo correction. This removal fixes ~20 open issues (quite possibly more), improves compile time performance by roughly .3-.4% (https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=AaronBallman&sortBy=date), and does not appear to regress diagnostic behavior in a way we wouldn't find acceptable. Fixes llvm#142457 Fixes llvm#139913 Fixes llvm#138850 Fixes llvm#137867 Fixes llvm#137860 Fixes llvm#107840 Fixes llvm#93308 Fixes llvm#69470 Fixes llvm#59391 Fixes llvm#58172 Fixes llvm#46215 Fixes llvm#45915 Fixes llvm#45891 Fixes llvm#44490 Fixes llvm#36703 Fixes llvm#32903 Fixes llvm#23312 Fixes llvm#69874
|
I was working on trying to land #144033, and ended up bisecting a change in diagnostic behavior impacting a libc++ test in that new configuration to this patch. Before: After: This doesn't seem related to typo correction, but I'm not very familiar with how clang's diagnostics work. I'll keep investigating and see what pops out. |
boomanaiden154
added a commit
to boomanaiden154/llvm-project
that referenced
this pull request
Jun 17, 2025
This patch disables unexpected_disabled_cpp17.verify.cpp under clang modules builds because it changes diagnostics criteria post llvm#143423, causing the test to fail. This patch follows a similar style to 853059a. This was found when working on trying to land llvm#144033.
|
Looks like this has occurred before and is just handled by disabling the test on the libc++ side. I've put up #144466 to try and get this fixed so we can finally switch over to the runtimes build. |
I was surprised by this while working on the patch as well. The "you must import" message was tied to delayed typo correction originally. I ended up bringing that functionality back through regular typo correction with this commit in this PR: 88ad970 So the behavioral change is somewhat expected (it was quite possible that the reason why it was hooked up to delayed typos was for this kind of situation).
I think that's a reasonable path forward for now, thank you (and sorry for the disruption)! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the delayed typo correction functionality from Clang (regular typo correction still remains) due to fragility of the solution.
An RFC was posted here: https://discourse.llvm.org/t/rfc-removing-support-for-delayed-typo-correction/86631 and while that RFC was asking for folks to consider stepping up to be maintainers, and we did have a few new contributors show some interest, experiments show that it's likely worth it to remove this functionality entirely and focus efforts on improving regular typo correction.
This removal fixes ~20 open issues (quite possibly more), improves compile time performance by roughly .3-.4% (https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=AaronBallman&sortBy=date), and does not appear to regress diagnostic behavior in a way we wouldn't find acceptable.
Fixes #142457
Fixes #139913
Fixes #138850
Fixes #137867
Fixes #137860
Fixes #107840
Fixes #93308
Fixes #69470
Fixes #59391
Fixes #58172
Fixes #46215
Fixes #45915
Fixes #45891
Fixes #44490
Fixes #36703
Fixes #32903
Fixes #23312
Fixes #69874