[clang-tidy] New checker bugprone-incomplete-comparison-operator

[kallehuttunen]

Summary

The checker detects comparison operators that don't access all fields of the compared types.

[lebedev.ri]

Thank you for working on this!

This looks questionable to me.
Is this based on some coding standard?
Are there any numbers on the false-positive rate of theck?

[kallehuttunen]

The checker gives quite many warnings on LLVM code base. For example, running it for lib/Transforms/Scalar:

/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const Expression &other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: commutative not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: other.commutative not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const CHIArg &A) { return VN == A.VN; }
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: I not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.I not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator!=(const CHIArg &A) { return !(*this == A); }
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: I not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.I not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator>(const SinkingInstructionCandidate &Other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumBlocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumInstructions not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumPHIs not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumMemoryInsts not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Blocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumBlocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumInstructions not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumPHIs not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumMemoryInsts not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.Blocks not accessed

/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator<(const BCEAtom &O) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: GEP not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: LoadI not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.GEP not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.LoadI not accessed

/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const Expression &Other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.Opcode not accessed
/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.HashVal not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  friend LLVM_ATTRIBUTE_UNUSED bool operator<(const Slice &LHS,
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: LHS.EndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: LHS.UseAndIsSplittable not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  friend LLVM_ATTRIBUTE_UNUSED bool operator<(uint64_t LHSOffset,
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: RHS.EndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: RHS.UseAndIsSplittable not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const partition_iterator &RHS) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: MaxSplitSliceEndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: RHS.MaxSplitSliceEndOffset not accessed

Running the checker on 100k SLOC proprietary code base I'm working with resulted in finding 5 bugs and 1 false positive (the code had a comment that one member was not accessed on purpose).

I'm anticipating that the checker can produce lots of false positives in some code bases because of operator<. That's why I'm making the checked operators configurable from the start.

[kallehuttunen]

In D59103#1421875, @lebedev.ri wrote:

Thank you for working on this!

This looks questionable to me.
Is this based on some coding standard?
Are there any numbers on the false-positive rate of theck?

It's not based on any coding standard, but is rather an attempt to eliminate one class of bugs (forgetting to update comparison operator when adding struct members) that I've encountered several times.

[lebedev.ri]

clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp

242–243

These should point to the actual field decl location in the class

[MyDeveloperDay]

I definately been burnt by not handling all the cases, but imagine a string class

class mystring
{
     const char *ptr;
     int size;
     int numallocated;
}

to compare the strings I wouldn't want to compare all the fields?

I think this could lead to a lot of false positives

[JonasToth]

@lebedev.ri @kallehuttunen what do you think of putting this into context of the proposal (i believe its being standardized with c++20) of operator==() = default and the spaceship-operator.
This check could serve as a basis to modernize the operator== to the default if appropriate and warn/diagnose in the other cases.
WDYT?

[Eugene.Zelenko]

clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp

197

Please don't use auto where return type is not obvious.

[aaron.ballman]

I'm definitely worried about the false positive rate for this check -- from a quick look at the diagnostics from LLVM, every one of them is a false positive (though perhaps I've missed something; it was a very quick look). Requiring users to explicitly use a declaration within the operator seems like it could get onerous for complex classes that support the notion of equality from a few members as opposed to all members.

Also, can you add some tests for classes that inherit members, perhaps even private ones? e.g.,

struct Base {
  int Member;
};

struct Derived : Base {
  int OtherMember;

  bool operator==(const Derived &RHS) const { return OtherMember == RHS.OtherMember; }
};

struct Derived2 : private Base {
  int OtherMember;

  bool operator==(const Derived2 &RHS) const { return OtherMember == RHS.OtherMember; }
};

Another good test is for scenarios where you cannot compare all of the members, such as:

struct Interesting {
  bool operator==(const Interesting &Other) const = delete;
};

struct S {
  int Member;
  Interesting OtherMember;

  bool operator==(const S &Other) const { return Member == Other.Member; }
};

[kallehuttunen]

I found this checker to be useful in the code base I initially developed it for, but the usage of comparison operators there is pretty much limited to comparing simple aggregate types. It's true that this checker can produce lots of false positives, maybe too much to be included to clang-tidy proper.

@JonasToth 's proposal is interesting. Overall I think the "modernize-use-defaulted-spaceship-op" check would be much more complex than this one, but I guess the FieldAccessVisitor could be used as a building block.

[kallehuttunen]

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

[aaron.ballman]

In D59103#1422775, @kallehuttunen wrote:

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

I don't think annotate would be a good choice because the primary purpose of that one is to pass that attribute information down to the backend and using it for this purpose feels a bit hackish. However, we could always add a new attribute if needed, but I'm not convinced an attribute is the right approach either (but then again, I'm also lacking information I'm sure).

What other kinds of checks do you have in mind and what are the semantics of the attribute you're thinking of?

[kallehuttunen]

In D59103#1423307, @aaron.ballman wrote:

In D59103#1422775, @kallehuttunen wrote:

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

I don't think annotate would be a good choice because the primary purpose of that one is to pass that attribute information down to the backend and using it for this purpose feels a bit hackish. However, we could always add a new attribute if needed, but I'm not convinced an attribute is the right approach either (but then again, I'm also lacking information I'm sure).

What other kinds of checks do you have in mind and what are the semantics of the attribute you're thinking of?

I was thinking about something along the lines of having an attribute to mark types to be a "value type" as defined in Herb's metaclasses proposal (chapter 3.5 in https://herbsutter.files.wordpress.com/2017/07/p0707r1.pdf). There could be then checks for having default ctor, no virtual functions etc in addition to this check for those types. But then again now that I think about it, a "string" class is an counterexample for this: it can be a value (regular) type, but when comparing instances you don't want to access the "capacity" member.

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "BoolPointerImplicitConversionCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
+#include "IncompleteComparisonOperatorCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FoldInitTypeCheck>(
         "bugprone-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
+    CheckFactories.registerCheck<IncompleteComparisonOperatorCheck>(
+        "bugprone-incomplete-comparison-operator");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "bugprone-macro-parentheses");

clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
+  IncompleteComparisonOperatorCheck.cpp
   IncorrectRoundingsCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp

clang-tidy/bugprone/IncompleteComparisonOperatorCheck.h

+//===--- IncompleteComparisonOperatorCheck.h - clang-tidy -------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCOMPLETECOMPARISONOPERATORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCOMPLETECOMPARISONOPERATORCHECK_H
+
+#include "../ClangTidy.h"
+#include <string>
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Detects comparison operators that don't access all parameter type fields.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-incomplete-comparison-operator.html
+class IncompleteComparisonOperatorCheck : public ClangTidyCheck {
+public:
+  IncompleteComparisonOperatorCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::string CheckedFunctions;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCOMPLETECOMPARISONOPERATORCHECK_H

clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp

+//===--- IncompleteComparisonOperatorCheck.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 "IncompleteComparisonOperatorCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallSet.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+
+// Matches dependent types
+AST_MATCHER(Type, isDependent) { return Node.isDependentType(); }
+
+// This class groups info about a comparison operator operand together
+class OperandInfo {
+public:
+  // Construct from function param declaration
+  OperandInfo(const ParmVarDecl *P)
+      : Param(P),
+        Record(Param->getType().getNonReferenceType()->getAsRecordDecl()) {
+    initFields();
+  }
+
+  // Construct from struct/class declaration.
+  // Can be used if there's no function param declaration for this operand (as
+  // is the case with LHS operand on member comparison operators).
+  // paramEquals() will always return false when this ctor is used.
+  OperandInfo(const RecordDecl *R) : Param(nullptr), Record(R) { initFields(); }
+
+  bool paramEquals(const ParmVarDecl *P) const { return Param && P == Param; }
+
+  bool recordEquals(const RecordDecl *R) const { return Record && R == Record; }
+
+  const Type *getType() const {
+    return Record ? Record->getTypeForDecl() : nullptr;
+  }
+
+  bool areAllFieldsAccessed() const { return Fields.empty(); }
+
+  int fieldCount() const { return Fields.size(); }
+
+  void fieldAccessed(const FieldDecl *F) { Fields.erase(F); }
+
+  void setAllFieldsAccessed() { Fields.clear(); }
+
+  void diagFields(ClangTidyCheck &Check, SourceLocation Loc) const {
+    for (auto It : Fields)
+      Check.diag(Loc,
+                 (Param ? Param->getNameAsString() + "." : "") +
+                     It->getNameAsString() + " not accessed",
+                 DiagnosticIDs::Note);
+  }
+
+private:
+  void initFields() {
+    if (Record)
+      for (const FieldDecl *F : Record->fields())
+        Fields.insert(F);
+  }
+
+  const ParmVarDecl *Param;
+  const RecordDecl *Record;
+  llvm::SmallSet<const FieldDecl *, 8> Fields;
+};
+
+// Remove pointers, refs, typedefs, qualifiers from T
+const Type *getCanonicalNonPointerType(QualType T) {
+  QualType PointeeT =
+      T->isPointerType() ? T->getPointeeType() : T.getNonReferenceType();
+  return PointeeT.getCanonicalType().getTypePtr();
+}
+
+// FieldAccessVisitor finds field accesses from the AST
+class FieldAccessVisitor : public RecursiveASTVisitor<FieldAccessVisitor> {
+public:
+  FieldAccessVisitor(OperandInfo &L, OperandInfo &R) : Lhs(L), Rhs(R) {}
+
+  bool VisitMemberExpr(MemberExpr *M) {
+    auto Field = dyn_cast_or_null<FieldDecl>(M->getMemberDecl());
+
+    // Not a field access? -> move on
+    if (!Field)
+      return true;
+
+    auto BaseDeclRef = dyn_cast_or_null<DeclRefExpr>(M->getBase());
+    auto ParamDecl = BaseDeclRef
+                         ? dyn_cast_or_null<ParmVarDecl>(BaseDeclRef->getDecl())
+                         : nullptr;
+
+    if (Lhs.paramEquals(ParamDecl))
+      Lhs.fieldAccessed(Field);
+    else if (Rhs.paramEquals(ParamDecl))
+      Rhs.fieldAccessed(Field);
+    else {
+      // We don't know which object this field is accessed on -> consider both
+      // LHS and RHS covered
+      Lhs.fieldAccessed(Field);
+      Rhs.fieldAccessed(Field);
+    }
+
+    // Continue traversal if not all fields have been accessed yet
+    return !Lhs.areAllFieldsAccessed() || !Rhs.areAllFieldsAccessed();
+  }
+
+  bool VisitCallExpr(CallExpr *C) {
+    auto Fun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl());
+
+    if (!Fun) {
+      // A function is called but we can't get a declaration for the function
+      // (this can happen inside templates) -> stop analysis and issue no
+      // warnings
+      Lhs.setAllFieldsAccessed();
+      Rhs.setAllFieldsAccessed();
+      return false;
+    }
+
+    if (!isInteresting(Fun))
+      return true;
+
+    const FunctionDecl *FunDef = nullptr;
+    const bool HasBody = Fun->hasBody(FunDef);
+
+    static constexpr int CALL_DEPTH_LIMIT = 10;
+    if (!HasBody || CallDepth >= CALL_DEPTH_LIMIT) {
+      // This is a function that we should analyze, but we can't traverse into
+      // it -> stop analysis and issue no warnings
+      Lhs.setAllFieldsAccessed();
+      Rhs.setAllFieldsAccessed();
+      return false;
+    }
+
+    ++CallDepth;
+    const bool RetVal = TraverseDecl(const_cast<FunctionDecl *>(FunDef));
+    --CallDepth;
+    return RetVal;
+  }
+
+private:
+  // A FunctionDecl is considered "interesting" for the purposes of this
+  // visitor, if
+  // - the function is a member of LHS or RHS type, OR
+  // - the function takes LHS or RHS type (or pointer to them) as parameter
+  bool isInteresting(const FunctionDecl *Fun) const {
+    auto Method = dyn_cast<CXXMethodDecl>(Fun);
+    const bool OperandTypeMethod =
+        Method && (Lhs.recordEquals(Method->getParent()) ||
+                   Rhs.recordEquals(Method->getParent()));
+
+    const bool HasOperandTypeAsParam = std::any_of(
+        Fun->param_begin(), Fun->param_end(), [this](const ParmVarDecl *Param) {
+          const Type *T = getCanonicalNonPointerType(Param->getType());
+          return T == Lhs.getType() || T == Rhs.getType();
+        });
+
+    return OperandTypeMethod || HasOperandTypeAsParam;
+  }
+
+  OperandInfo &Lhs;
+  OperandInfo &Rhs;
+  int CallDepth = 0;
+};
+
+} // namespace
+
+IncompleteComparisonOperatorCheck::IncompleteComparisonOperatorCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckedFunctions(Options.get(
+          "CheckedFunctions",
+          "operator==;operator!=;operator<;operator>;operator<=;operator>=")) {}
+
+void IncompleteComparisonOperatorCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckedFunctions", CheckedFunctions);
+}
+
+void IncompleteComparisonOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  const auto Ops = utils::options::parseStringList(CheckedFunctions);
+  Finder->addMatcher(
+      functionDecl(
+          hasAnyName(std::vector<StringRef>(Ops.begin(), Ops.end())),
+          isDefinition(),
+          unless(anyOf(isDeleted(), hasAnyParameter(hasType(isDependent())))))
+          .bind("operator"),
+      this);
+}
+
+void IncompleteComparisonOperatorCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Operator = Result.Nodes.getNodeAs<FunctionDecl>("operator");
+
+  llvm::Optional<OperandInfo> Lhs;
+  llvm::Optional<OperandInfo> Rhs;
+
+  switch (Operator->getNumParams()) {
+  case 1: {
+    // Member operator
+    const auto *Method = dyn_cast<CXXMethodDecl>(Operator);
+    if (!Method)
+      return;
+    Lhs.emplace(Method->getParent());
+    Rhs.emplace(Operator->getParamDecl(0));
+    break;
+  }
+  case 2: {
+    // Non-member operator
+    Lhs.emplace(Operator->getParamDecl(0));
+    Rhs.emplace(Operator->getParamDecl(1));
+    break;
+  }
+  default:
+    return;
+  }
+
+  if (Lhs->areAllFieldsAccessed() && Rhs->areAllFieldsAccessed())
+    return; // Bail out early if no fields are found for the operand types
+
+  FieldAccessVisitor Visitor{*Lhs, *Rhs};
+  Visitor.TraverseDecl(const_cast<FunctionDecl *>(Operator));
+
+  if (!Lhs->areAllFieldsAccessed() || !Rhs->areAllFieldsAccessed()) {
+    diag(Operator->getBeginLoc(), "incomplete comparison operator");
+    Lhs->diagFields(*this, Operator->getBeginLoc());
+    Rhs->diagFields(*this, Operator->getBeginLoc());
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New :doc:`abseil-duration-unnecessary-conversion
   numeric types and back again.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 
   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
+- New :doc:`bugprone-incomplete-comparison-operator
+  <clang-tidy/checks/bugprone-incomplete-comparison-operator>` check.
+
+  Warns on comparison operator implementations that don't access all the
+  non-static data members of the compared types.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - New alias :doc:`cppcoreguidelines-explicit-virtual-functions

docs/clang-tidy/checks/bugprone-incomplete-comparison-operator.rst

+.. title:: clang-tidy - bugprone-incomplete-comparison-operator
+
+bugprone-incomplete-comparison-operator
+=======================================
+
+Warns on comparison operator implementations that don't access all the
+non-static data members of the compared types.
+
+Examples:
+
+.. code-block:: c++
+
+  struct S {
+    int Member1;
+    int Member2;
+  };
+
+  // Warning - Lhs.Member2 and Rhs.Member2 not accessed
+  bool operator==(const S &Lhs, const S &Rhs) {
+    return Lhs.Member1 == Rhs.Member1;
+  }
+
+  // No warning - suppressed with void cast
+  bool operator<(const S &Lhs, const S &Rhs) {
+    (void)Lhs.Member2;
+    (void)Rhs.Member2;
+    return Lhs.Member1 < Rhs.Member1;
+  }
+
+The checked operators can be configured.
+
+Options
+-------
+
+.. option:: CheckedFunctions
+
+   Semicolon-separated list of functions to check. Defaults to
+   ``operator==;operator!=;operator<;operator>;operator<=;operator>=``.

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-bool-pointer-implicit-conversion
    bugprone-copy-constructor-init
    bugprone-dangling-handle
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
    bugprone-forwarding-reference-overload
    bugprone-inaccurate-erase
+   bugprone-incomplete-comparison-operator
    bugprone-incorrect-roundings
    bugprone-integer-division
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-widening-cast
    bugprone-move-forwarding-reference

test/clang-tidy/bugprone-incomplete-comparison-operator-custom.cpp

+// RUN: %check_clang_tidy %s bugprone-incomplete-comparison-operator %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: bugprone-incomplete-comparison-operator.CheckedFunctions, \
+// RUN:    value: "operator==;operator!="}]}' \
+// RUN: --
+
+struct S {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const S &Lhs, const S &Rhs) {
+  return Lhs.Member1 == Rhs.Member1 && Lhs.Member2 == Rhs.Member2;
+}
+
+bool operator!=(const S &Lhs, const S &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return Lhs.Member1 != Rhs.Member1;
+}
+
+// operator not configured to be checked -> no warning
+bool operator<(const S &Lhs, const S &Rhs) {
+  return Lhs.Member1 < Rhs.Member1;
+}

test/clang-tidy/bugprone-incomplete-comparison-operator.cpp

+// RUN: %check_clang_tidy %s bugprone-incomplete-comparison-operator %t
+
+struct Good {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Good &Lhs, const Good &Rhs) {
+  return Lhs.Member1 == Rhs.Member1 && Lhs.Member2 == Rhs.Member2;
+}
+
+bool operator!=(const Good &Lhs, const Good &Rhs) {
+  return !(Lhs == Rhs);
+}
+
+bool operator<(const Good &Lhs, const Good &Rhs) {
+  if (Lhs.Member1 != Rhs.Member1)
+    return Lhs.Member1 < Rhs.Member1;
+  return Lhs.Member2 < Rhs.Member2;
+}
+
+bool operator>(const Good &Lhs, const Good &Rhs) {
+  return Rhs < Lhs;
+}
+
+bool operator<=(const Good &Lhs, const Good &Rhs) {
+  return !(Rhs < Lhs);
+}
+
+bool operator>=(const Good &Lhs, const Good &Rhs) {
+  return !(Lhs < Rhs);
+}
+
+struct Bad {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return Lhs.Member1 == Rhs.Member1;
+}
+
+bool operator!=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return !(Lhs == Rhs);
+}
+
+bool operator<(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return Lhs.Member2 < Rhs.Member2;
+}
+
+bool operator>(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return Rhs < Lhs;
+}
+
+bool operator<=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return !(Rhs < Lhs);
+}
+
+bool operator>=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return !(Lhs < Rhs);
+}
+
+struct Ugly {
+  Good GoodParts;
+  Bad BadParts;
+};
+
+// Missing field access for only one of the params -> warning
+bool operator==(const Ugly &Lhs, const Ugly &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Rhs.BadParts not accessed
+  return Lhs.GoodParts == Rhs.GoodParts && Lhs.BadParts == Lhs.BadParts;
+}
+
+// Args are passed to a function in another TU -> no warning
+bool testForInequality(const Ugly &, const Ugly &);
+bool operator!=(const Ugly &Lhs, const Ugly &Rhs) {
+  return testForInequality(Lhs, Rhs);
+}
+
+// Fields are passed to a function in another TU, but some fields are not accessed -> warning
+bool compareGoodness(const Good &, const Good &);
+bool operator<(const Ugly &Lhs, const Ugly &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.BadParts not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.BadParts not accessed
+  return compareGoodness(Lhs.GoodParts, Rhs.GoodParts);
+}
+
+// Args are passed to recursive function -> no warning
+bool strangeLoop(const Ugly &U) { return strangeLoop(U); }
+bool operator>(const Ugly &Lhs, const Ugly &Rhs) {
+  return strangeLoop(Lhs);
+}
+
+class Encapsulated {
+public:
+  int get1() const { return Member1; }
+  int get2() const { return Member2; }
+
+  // getters that are defined later in the TU
+  int query1() const;
+  int query2() const;
+
+private:
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Encapsulated &Lhs, const Encapsulated &Rhs) {
+  return Lhs.get1() == Rhs.get1() && Lhs.get2() == Rhs.get2();
+}
+
+bool operator!=(const Encapsulated &Lhs, const Encapsulated &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return Lhs.get1() != Rhs.get1();
+}
+
+bool operator<(const Encapsulated &Lhs, const Encapsulated &Rhs) {
+  if (Lhs.query1() != Rhs.query1())
+    return Lhs.query1() < Rhs.query1();
+  return Lhs.query2() < Rhs.query2();
+}
+
+bool operator>(const Encapsulated &Lhs, const Encapsulated &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return Lhs.query2() > Rhs.query2();
+}
+
+int Encapsulated::query1() const {
+  return Member1;
+}
+
+int Encapsulated::query2() const {
+  return Member2;
+}
+
+struct MemOps {
+  int Member1;
+  int Member2;
+
+  bool operator==(const MemOps &Rhs) const {
+    return Member1 == Rhs.Member1 && Member2 == Rhs.Member2;
+  }
+
+  bool operator!=(const MemOps &Rhs) const {
+    return !(*this == Rhs);
+  }
+
+  bool operator<(const MemOps &Rhs) const {
+    // CHECK-NOTES: [[@LINE-1]]:3: warning: incomplete comparison operator
+    // CHECK-NOTES: [[@LINE-2]]:3: note: Member2 not accessed
+    // CHECK-NOTES: [[@LINE-3]]:3: note: Rhs.Member2 not accessed
+    return Member1 < Rhs.Member1;
+  }
+
+  bool operator>(const MemOps &Rhs) const {
+    // CHECK-NOTES: [[@LINE-1]]:3: warning: incomplete comparison operator
+    // CHECK-NOTES: [[@LINE-2]]:3: note: Member2 not accessed
+    // CHECK-NOTES: [[@LINE-3]]:3: note: Rhs.Member2 not accessed
+    return Rhs < *this;
+  }
+};
+
+struct StaticMem {
+  int Member1;
+  static int Member2;
+};
+
+// static member not accessed -> no warning
+bool operator==(const StaticMem &Lhs, const StaticMem &Rhs) {
+  return Lhs.Member1 == Rhs.Member1;
+}
+
+// deleted operators -> no warning
+struct DeletedOps {
+  int Member1;
+  bool operator==(const DeletedOps &) const = delete;
+};
+
+bool operator<(const DeletedOps &, const DeletedOps &) = delete;
+
+// comparisons between different types
+struct TypeX {
+  int Member1;
+  int Member2;
+};
+
+struct TypeY {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const TypeX &X, const TypeY &Y) {
+  return X.Member1 == Y.Member1 && X.Member2 == Y.Member2;
+}
+
+bool operator!=(const TypeX &X, const TypeY &Y) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: X.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Y.Member2 not accessed
+  return X.Member1 != Y.Member1;
+}
+
+template <typename T>
+struct TemplateType {
+  T Member;
+
+  bool operator<(const TemplateType &Rhs) const {
+    return Member < Rhs.Member;
+  }
+
+  bool operator>(const TemplateType &Rhs) const {
+    // CHECK-NOTES: [[@LINE-1]]:3: warning: incomplete comparison operator
+    // CHECK-NOTES: [[@LINE-2]]:3: note: Member not accessed
+    // CHECK-NOTES: [[@LINE-3]]:3: note: Rhs.Member not accessed
+    return false;
+  }
+
+  // template not instantiated -> no warning
+  bool operator<=(const TemplateType &Rhs) const {
+    return false;
+  }
+};
+
+template <typename T>
+bool operator==(const TemplateType<T> &Lhs, const TemplateType<T> &Rhs) {
+  return Lhs.Member == Rhs.Member;
+}
+
+template <typename T>
+bool operator!=(const TemplateType<T> &Lhs, const TemplateType<T> &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member not accessed
+  return false;
+}
+
+// template not instantiated -> no warning
+template <typename T>
+bool operator>=(const TemplateType<T> &Lhs, const TemplateType<T> &Rhs) {
+  return false;
+}
+
+bool useTemplates(const TemplateType<Good> &Lhs, const TemplateType<Good> &Rhs) {
+  return Lhs == Rhs || Lhs != Rhs || Lhs < Rhs || Lhs > Rhs;
+}
+
+struct Suppress {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Suppress &Lhs, const Suppress &Rhs) {
+  (void)Lhs.Member2;
+  (void)Rhs.Member2;
+  return Lhs.Member1 == Rhs.Member1;
+}