Author: Congcong Cai Date: 2025-03-02T19:39:49+08:00 New Revision: 60afce2df97d1f8fd78405a039e8e818c5154565
URL: https://github.com/llvm/llvm-project/commit/60afce2df97d1f8fd78405a039e8e818c5154565 DIFF: https://github.com/llvm/llvm-project/commit/60afce2df97d1f8fd78405a039e8e818c5154565.diff LOG: [clang-tidy] fix fp when modifying variant by ``operator[]`` with template in parameters (#128407) `ArraySubscriptExpr` can switch base and idx. For dependent array subscript access, we should check both base and idx conservatively. Added: Modified: clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp clang/lib/Analysis/ExprMutationAnalyzer.cpp clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 07a79d6bbe807..71edb704b49d6 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -122,7 +122,8 @@ Changes in existing checks - Improved :doc:`misc-const-correctness <clang-tidy/checks/misc/const-correctness>` check by adding the option `AllowedTypes`, that excludes specified types from const-correctness - checking. + checking and fixing false positives when modifying variant by ``operator[]`` + with template in parameters. - Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc/redundant-expression>` check by providing additional diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp index 5efb64bca2374..654deead4efc8 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp @@ -998,3 +998,11 @@ void member_pointer_const(Value &x, PointerToConstMemberFunction m) { // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'member_pointer_tmp' of type 'Value &' can be declared 'const' (member_pointer_tmp.*m)(); } + +namespace gh127776_false_positive { +template <class T> struct vector { T &operator[](int t); }; +template <typename T> void f() { + vector<int> x; + x[T{}] = 3; +} +} // namespace gh127776_false_positive diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp index 8944343484e58..823d7543f085f 100644 --- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp @@ -80,6 +80,17 @@ static bool canExprResolveTo(const Expr *Source, const Expr *Target) { namespace { +// `ArraySubscriptExpr` can switch base and idx, e.g. `a[4]` is the same as +// `4[a]`. When type is dependent, we conservatively assume both sides are base. +AST_MATCHER_P(ArraySubscriptExpr, hasBaseConservative, + ast_matchers::internal::Matcher<Expr>, InnerMatcher) { + if (Node.isTypeDependent()) { + return InnerMatcher.matches(*Node.getLHS(), Finder, Builder) || + InnerMatcher.matches(*Node.getRHS(), Finder, Builder); + } + return InnerMatcher.matches(*Node.getBase(), Finder, Builder); +} + AST_MATCHER(Type, isDependentType) { return Node.isDependentType(); } AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) { @@ -513,8 +524,8 @@ ExprMutationAnalyzer::Analyzer::findArrayElementMutation(const Expr *Exp) { // Check whether any element of an array is mutated. const auto SubscriptExprs = match( findAll(arraySubscriptExpr( - anyOf(hasBase(canResolveToExpr(Exp)), - hasBase(implicitCastExpr(allOf( + anyOf(hasBaseConservative(canResolveToExpr(Exp)), + hasBaseConservative(implicitCastExpr(allOf( hasCastKind(CK_ArrayToPointerDecay), hasSourceExpression(canResolveToExpr(Exp))))))) .bind(NodeID<Expr>::value)), @@ -716,7 +727,8 @@ ExprMutationAnalyzer::Analyzer::findPointeeValueMutation(const Expr *Exp) { unaryOperator(hasOperatorName("*"), hasUnaryOperand(canResolveToExprPointee(Exp))), // deref by [] - arraySubscriptExpr(hasBase(canResolveToExprPointee(Exp))))) + arraySubscriptExpr( + hasBaseConservative(canResolveToExprPointee(Exp))))) .bind(NodeID<Expr>::value))), Stm, Context); return findExprMutation(Matches); diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp index cc277d56b37a2..720999207083d 100644 --- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -870,6 +870,19 @@ TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) { EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y")); } +TEST(ExprMutationAnalyzerTest, T1) { + const auto AST = buildASTFromCodeWithArgs( + "template <class T> struct vector { T &operator[](int t); };" + "template <typename T> void func() {" + " vector<int> x;" + " x[T{}] = 3;" + "}", + {"-fno-delayed-template-parsing"}); + const auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + // section: special case: all created references are non-mutating themself // and therefore all become 'const'/the value is not modified! _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
