[clang-tidy] Ignore concepts in `misc-redundant-expression`

[Izaron]

Summary

The checker should ignore requirement expressions inside concept
definitions, because redundant expressions still make sense here

Fixes https://github.com/llvm/llvm-project/issues/54114

[aaron.ballman]

Thanks for the fix, can you also add a release note for it?

I'm a bit worried about using hasAncestor() for this; that has a tendency to do surprising things in addition to being expensive because it's very greedy. However, I can't see a situation in which it's going to be wrong, because the body of the requires expression is always unevaluated (so we really don't need to worry about nonsense like defining a lambda in the requires body, and then defining a class in the lambda, and having a redundant expression we care to diagnose in a member function of that class).

@alexfh or @sammccall -- do you see any concerns with this use of hasAncestor()?

[sammccall]

I don't actually know a great deal about matcher performance :-( I wish I did.
I think this is pretty much in line with how plenty of check matchers already work though, in particular unless(isInTemplateInstantiation()) is basically the same thing and is used in many places including this check.
So I wouldn't be particularly worried.

(Obviously from first principles you'd prefer to prune the subtree rather than match within it and then walk up the parent chain, but I don't think matchers supports this pattern well).

[aaron.ballman]

In D122078#3407981, @sammccall wrote:

I don't actually know a great deal about matcher performance :-( I wish I did.
I think this is pretty much in line with how plenty of check matchers already work though, in particular unless(isInTemplateInstantiation()) is basically the same thing and is used in many places including this check.
So I wouldn't be particularly worried.

(Obviously from first principles you'd prefer to prune the subtree rather than match within it and then walk up the parent chain, but I don't think matchers supports this pattern well).

Okie dokie, thanks for weighing in! This LGTM (feel free to add the release note when landing). If @alexfh has some technical concerns with hasAncestor(), we can always revert and address them once we know more.

[njames93]

In D122078#3407984, @aaron.ballman wrote:

Okie dokie, thanks for weighing in! This LGTM (feel free to add the release note when landing). If @alexfh has some technical concerns with hasAncestor(), we can always revert and address them once we know more.

I feel like this is another issue which could be solved with traversal scopes. isInEvaluationContext This traversal scope wouldn't traverse things like concepts, static_asserts(?), Noexcept & explicit specifiers, sizeof, decltype etc.

[Izaron]

Add release note

[njames93]

Just address that test issue then it'll be good

clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp

1 ↗

(On Diff #460209)

This is removing test coverage for previous language versions unnecessarily. Can you create a new test file redundant-expression-cxx20 instead and move this new test case into there.

[Izaron]

Created redundant-expression-cxx20.cpp

[Izaron]

The new file was failing with message

CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input

So I had to add an additional urrelevant check.
I added a check for consteval function (there were no such tests previously).

[njames93]

In D122078#3797519, @Izaron wrote:

The new file was failing with message

CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input

So I had to add an additional urrelevant check.
I added a check for consteval function (there were no such tests previously).

You don't need the unrelated tests, you can use a special run line that will check that no warnings are emitted.
// RUN: clang-tidy %s --checks=-*,misc-redundant-expression -- | count 0

[Izaron]

Fix test with count 0, thanks to @njames93 !

[Izaron]

Hi! A friendly ping =)

[njames93]

LGTM, just one small nit.

clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

3

This comment isn't strictly necessary

[Izaron]

Removed redundant comment

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

 //===--- RedundantExpressionCheck.cpp - clang-tidy-------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RedundantExpressionCheck.h"
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"

 }
 
 AST_MATCHER(Expr, isIntegerConstantExpr) {
   if (Node.isInstantiationDependent())
     return false;
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
+AST_MATCHER(Expr, isRequiresExpr) { return isa<RequiresExpr>(Node); }
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
 
 AST_MATCHER(BinaryOperator, nestedOperandsAreEquivalent) {
   return markDuplicateOperands(&Node, Builder, Finder->getASTContext());
 }
 

 } // namespace
 
 void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto AnyLiteralExpr = ignoringParenImpCasts(
       anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
 
   const auto BannedIntegerLiteral =
       integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedAncestor = expr(isRequiresExpr());
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
       traverse(TK_AsIs,
                binaryOperator(
                    anyOf(isComparisonOperator(),
                          hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&",
                                             "||", "=")),
                    operandsAreEquivalent(),
                    // Filter noisy false positives.
                    unless(isInTemplateInstantiation()),
                    unless(binaryOperatorIsInMacro()),
                    unless(hasType(realFloatingPointType())),
                    unless(hasEitherOperand(hasType(realFloatingPointType()))),
                    unless(hasLHS(AnyLiteralExpr)),
-                   unless(hasDescendant(BannedIntegerLiteral)))
+                   unless(hasDescendant(BannedIntegerLiteral)),
+                   unless(hasAncestor(BannedAncestor)))
                    .bind("binary")),
       this);
 
   // Logical or bitwise operator with equivalent nested operands, like (X && Y
   // && X) or (X && (Y && X))
   Finder->addMatcher(
       binaryOperator(hasAnyOperatorName("|", "&", "||", "&&", "^"),
                      nestedOperandsAreEquivalent(),
                      // Filter noisy false positives.
                      unless(isInTemplateInstantiation()),
                      unless(binaryOperatorIsInMacro()),
                      // TODO: if the banned macros are themselves duplicated
-                     unless(hasDescendant(BannedIntegerLiteral)))
+                     unless(hasDescendant(BannedIntegerLiteral)),
+                     unless(hasAncestor(BannedAncestor)))
           .bind("nested-duplicates"),
       this);
 
   // Conditional (ternary) operator with equivalent operands, like (Y ? X : X).
   Finder->addMatcher(
       traverse(TK_AsIs,
                conditionalOperator(expressionsAreEquivalent(),
                                    // Filter noisy false positives.
                                    unless(conditionalOperatorIsInMacro()),
-                                   unless(isInTemplateInstantiation()))
+                                   unless(isInTemplateInstantiation()),
+                                   unless(hasAncestor(BannedAncestor)))
                    .bind("cond")),
       this);
 
   // Overloaded operators with equivalent operands.
   Finder->addMatcher(
       traverse(TK_AsIs,
                cxxOperatorCallExpr(
                    hasAnyOverloadedOperatorName("-", "/", "%", "|", "&", "^",
                                                 "==", "!=", "<", "<=", ">",
                                                 ">=", "&&", "||", "="),
                    parametersAreEquivalent(),
                    // Filter noisy false positives.
-                   unless(isMacro()), unless(isInTemplateInstantiation()))
+                   unless(isMacro()), unless(isInTemplateInstantiation()),
+                   unless(hasAncestor(BannedAncestor)))
                    .bind("call")),
       this);
 
   // Overloaded operators with equivalent operands.
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("|", "&", "||", "&&", "^"),
           nestedParametersAreEquivalent(), argumentCountIs(2),
           // Filter noisy false positives.
-          unless(isMacro()), unless(isInTemplateInstantiation()))
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          unless(hasAncestor(BannedAncestor)))
           .bind("nested-duplicates"),
       this);
 
   // Match expressions like: !(1 | 2 | 3)
   Finder->addMatcher(
       traverse(TK_AsIs,
                implicitCastExpr(
                    hasImplicitDestinationType(isInteger()),
                    has(unaryOperator(
                            hasOperatorName("!"),
                            hasUnaryOperand(ignoringParenImpCasts(binaryOperator(
                                hasAnyOperatorName("|", "&"),
                                hasLHS(anyOf(
                                    binaryOperator(hasAnyOperatorName("|", "&")),
                                    integerLiteral())),
                                hasRHS(integerLiteral())))))
-                           .bind("logical-bitwise-confusion")))),
+                           .bind("logical-bitwise-confusion")),
+                   unless(hasAncestor(BannedAncestor)))),
       this);
 
   // Match expressions like: (X << 8) & 0xFF
   Finder->addMatcher(
       traverse(TK_AsIs,
                binaryOperator(
                    hasOperatorName("&"),
                    hasOperands(ignoringParenImpCasts(binaryOperator(
                                    hasOperatorName("<<"),
                                    hasRHS(ignoringParenImpCasts(
                                        integerLiteral().bind("shift-const"))))),
                                ignoringParenImpCasts(
-                                   integerLiteral().bind("and-const"))))
+                                   integerLiteral().bind("and-const"))),
+                   unless(hasAncestor(BannedAncestor)))
                    .bind("left-right-shift-confusion")),
       this);
 
   // Match common expressions and apply more checks to find redundant
   // sub-expressions.
   //   a) Expr <op> K1 == K2
   //   b) Expr <op> K1 == Expr
   //   c) Expr <op> K1 == Expr <op> K2
   // see: 'checkArithmeticExpr' and 'checkBitwiseExpr'
   const auto BinOpCstLeft = matchBinOpIntegerConstantExpr("lhs");
   const auto BinOpCstRight = matchBinOpIntegerConstantExpr("rhs");
   const auto CstRight = matchIntegerConstantExpr("rhs");
   const auto SymRight = matchSymbolicExpr("rhs");
 
   // Match expressions like: x <op> 0xFF == 0xF00.
   Finder->addMatcher(
       traverse(TK_AsIs, binaryOperator(isComparisonOperator(),
-                                       hasOperands(BinOpCstLeft, CstRight))
+                                       hasOperands(BinOpCstLeft, CstRight),
+                                       unless(hasAncestor(BannedAncestor)))
                             .bind("binop-const-compare-to-const")),
       this);
 
   // Match expressions like: x <op> 0xFF == x.
   Finder->addMatcher(
       traverse(
           TK_AsIs,
           binaryOperator(isComparisonOperator(),
                          anyOf(allOf(hasLHS(BinOpCstLeft), hasRHS(SymRight)),
-                               allOf(hasLHS(SymRight), hasRHS(BinOpCstLeft))))
+                               allOf(hasLHS(SymRight), hasRHS(BinOpCstLeft))),
+                         unless(hasAncestor(BannedAncestor)))
               .bind("binop-const-compare-to-sym")),
       this);
 
   // Match expressions like: x <op> 10 == x <op> 12.
   Finder->addMatcher(
       traverse(TK_AsIs,
                binaryOperator(isComparisonOperator(), hasLHS(BinOpCstLeft),
                               hasRHS(BinOpCstRight),
                               // Already reported as redundant.
-                              unless(operandsAreEquivalent()))
+                              unless(operandsAreEquivalent()),
+                              unless(hasAncestor(BannedAncestor)))
                    .bind("binop-const-compare-to-binop-const")),
       this);
 
   // Match relational expressions combined with logical operators and find
   // redundant sub-expressions.
   // see: 'checkRelationalExpr'
 
   // Match expressions like: x < 2 && x > 2.
   const auto ComparisonLeft = matchRelationalIntegerConstantExpr("lhs");
   const auto ComparisonRight = matchRelationalIntegerConstantExpr("rhs");
   Finder->addMatcher(
       traverse(TK_AsIs,
                binaryOperator(hasAnyOperatorName("||", "&&"),
                               hasLHS(ComparisonLeft), hasRHS(ComparisonRight),
                               // Already reported as redundant.
-                              unless(operandsAreEquivalent()))
+                              unless(operandsAreEquivalent()),
+                              unless(hasAncestor(BannedAncestor)))
                    .bind("comparisons-of-symbol-and-const")),
       this);
 }
 
 void RedundantExpressionCheck::checkArithmeticExpr(
     const MatchFinder::MatchResult &Result) {
   APSInt LhsValue, RhsValue;
   const Expr *LhsSymbol = nullptr, *RhsSymbol = nullptr;

clang-tools-extra/docs/ReleaseNotes.rst

 - Improved :doc:`modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default>`
   check.
 
   The check now skips unions/union-like classes since in this case a default constructor
   with empty body is not equivalent to the explicitly defaulted one. The check also skips
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc-redundant-expression>`
+  check.
+
+  The check now skips concept definitions since redundant expressions still make sense
+  inside them.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-cxx20.cpp

+// RUN: clang-tidy %s -checks=-*,misc-redundant-expression -- -std=c++20 | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+namespace concepts {
+// redundant expressions inside concepts make sense, ignore them
+template <class I>
+concept TestConcept = requires(I i) {
+  {i - i};
+  {i && i};
+  {i ? i : i};
+};
+} // namespace concepts