Skip to content

Commit 8a45969

Browse files
sonar-nigel[bot]Sonar Vibe BotSeppli11
authored and
sonartech
committed
SONARPY-3972 Rule S8500 Comparison methods should be defined completely (#1034)
Co-authored-by: Sonar Vibe Bot <vibe-bot@sonarsource.com> Co-authored-by: Sebastian Zumbrunn <sebastian.zumbrunn@sonarsource.com> GitOrigin-RevId: d90cd70b2e2a570011900f7278b29f98b6c83142
1 parent 58e2125 commit 8a45969

7 files changed

Lines changed: 651 additions & 0 deletions

File tree

‎python-checks/src/main/java/org/sonar/python/checks/IncompleteComparisonMethodsCheck.java‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* You can redistribute and/or modify this program under the terms of
7+
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks;
18+
19+
import java.util.LinkedHashMap;
20+
import java.util.List;
21+
import java.util.Map;
22+
import java.util.Optional;
23+
import java.util.Set;
24+
import org.sonar.check.Rule;
25+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
26+
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.tree.AnnotatedAssignment;
28+
import org.sonar.plugins.python.api.tree.AssignmentStatement;
29+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
30+
import org.sonar.plugins.python.api.tree.ClassDef;
31+
import org.sonar.plugins.python.api.tree.Decorator;
32+
import org.sonar.plugins.python.api.tree.Expression;
33+
import org.sonar.plugins.python.api.tree.ExpressionList;
34+
import org.sonar.plugins.python.api.tree.FunctionDef;
35+
import org.sonar.plugins.python.api.tree.Name;
36+
import org.sonar.plugins.python.api.tree.Tree;
37+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
38+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
39+
40+
@Rule(key = "S8500")
41+
public class IncompleteComparisonMethodsCheck extends PythonSubscriptionCheck {
42+
43+
private static final String MESSAGE = "Add the missing comparison methods or use \"functools.total_ordering\".";
44+
private static final String SECONDARY_MESSAGE = "\"%s\" is defined here.";
45+
46+
private static final Set<String> ORDERING_METHODS = Set.of("__lt__", "__le__", "__gt__", "__ge__");
47+
48+
private static final TypeMatcher TOTAL_ORDERING_MATCHER = TypeMatchers.withFQN("functools.total_ordering");
49+
50+
@Override
51+
public void initialize(Context context) {
52+
context.registerSyntaxNodeConsumer(Tree.Kind.CLASSDEF, IncompleteComparisonMethodsCheck::checkClassDef);
53+
}
54+
55+
private static void checkClassDef(SubscriptionContext ctx) {
56+
ClassDef classDef = (ClassDef) ctx.syntaxNode();
57+
58+
CollectOrderingMethodNamesVisitor visitor = new CollectOrderingMethodNamesVisitor();
59+
classDef.body().accept(visitor);
60+
61+
if (visitor.definitions.isEmpty() || visitor.definitions.size() == ORDERING_METHODS.size()) {
62+
return;
63+
}
64+
65+
for (Decorator decorator : classDef.decorators()) {
66+
if (TOTAL_ORDERING_MATCHER.isTrueFor(decorator.expression(), ctx)) {
67+
return;
68+
}
69+
}
70+
71+
PreciseIssue issue = ctx.addIssue(classDef.name(), MESSAGE);
72+
visitor.definitions.forEach((name, tree) -> issue.secondary(tree, String.format(SECONDARY_MESSAGE, name)));
73+
}
74+
75+
/**
76+
* Collects ordering methods defined at the top level of a class body, whether they are
77+
* introduced by a {@code def} or by an assignment such as {@code __lt__ = lambda ...}.
78+
* The map preserves the first occurrence per name in source order, which keeps secondary
79+
* locations stable when a method is shadowed.
80+
* Mirrors the recursion-control pattern of {@code TreeUtils.CollectFunctionDefsVisitor}:
81+
* does not descend into nested classes or nested functions.
82+
*/
83+
private static class CollectOrderingMethodNamesVisitor extends BaseTreeVisitor {
84+
private final Map<String, Tree> definitions = new LinkedHashMap<>();
85+
86+
@Override
87+
public void visitClassDef(ClassDef nestedClass) {
88+
// Do not descend into nested classes
89+
}
90+
91+
@Override
92+
public void visitFunctionDef(FunctionDef functionDef) {
93+
Name name = functionDef.name();
94+
if (ORDERING_METHODS.contains(name.name())) {
95+
definitions.putIfAbsent(name.name(), name);
96+
}
97+
// Do not descend into nested functions
98+
}
99+
100+
@Override
101+
public void visitAssignmentStatement(AssignmentStatement assignment) {
102+
assignmentTargetName(assignment)
103+
.filter(name -> ORDERING_METHODS.contains(name.name()))
104+
.ifPresent(name -> definitions.putIfAbsent(name.name(), name));
105+
super.visitAssignmentStatement(assignment);
106+
}
107+
108+
@Override
109+
public void visitAnnotatedAssignment(AnnotatedAssignment annotated) {
110+
if (annotated.assignedValue() != null && annotated.variable() instanceof Name name && ORDERING_METHODS.contains(name.name())) {
111+
definitions.putIfAbsent(name.name(), name);
112+
}
113+
super.visitAnnotatedAssignment(annotated);
114+
}
115+
116+
private static Optional<Name> assignmentTargetName(AssignmentStatement assignment) {
117+
List<ExpressionList> lhsList = assignment.lhsExpressions();
118+
if (lhsList.size() != 1) {
119+
return Optional.empty();
120+
}
121+
List<Expression> expressions = lhsList.get(0).expressions();
122+
if (expressions.size() == 1 && expressions.get(0) instanceof Name name) {
123+
return Optional.of(name);
124+
}
125+
return Optional.empty();
126+
}
127+
}
128+
}

‎python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ public Stream<Class<?>> getChecks() {
286286
ImpossibleBoundariesCheck.class,
287287
IncompatibleOperandsCheck.class,
288288
InconsistentTupleReturnCheck.class,
289+
IncompleteComparisonMethodsCheck.class,
289290
InconsistentTypeHintCheck.class,
290291
IncorrectExceptionTypeCheck.class,
291292
IncorrectParameterDatetimeConstructorsCheck.class,

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8500.html‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
<p>This rule raises an issue when a class defines one or more rich comparison methods (<code>__lt__</code>, <code>__le__</code>, <code>__gt__</code>,
2+
<code>__ge__</code>) without using the <code>functools.total_ordering</code> decorator or defining all of them.</p>
3+
<h2>Why is this an issue?</h2>
4+
<p>Python classes can implement rich comparison methods to enable sorting and comparison operations. These methods include:</p>
5+
<ul>
6+
<li><code>__lt__</code> for less than (<code>&lt;</code>)</li>
7+
<li><code>__le__</code> for less than or equal to (<code>&lt;=</code>)</li>
8+
<li><code>__gt__</code> for greater than (<code>&gt;</code>)</li>
9+
<li><code>__ge__</code> for greater than or equal to (<code>&gt;=</code>)</li>
10+
<li><code>__eq__</code> for equality (<code>==</code>)</li>
11+
<li><code>__ne__</code> for inequality (<code>!=</code>)</li>
12+
</ul>
13+
<p>When you define only some of these methods, Python cannot automatically infer the missing ones. This leads to inconsistent behavior:</p>
14+
<ul>
15+
<li>sorting algorithms may fail or produce incorrect results</li>
16+
<li>comparison operations may raise <code>TypeError</code> exceptions</li>
17+
<li>code that expects symmetric comparisons, such as <code>a &lt; b</code> being true implying <code>b &gt; a</code> is also true, may behave
18+
unexpectedly</li>
19+
</ul>
20+
<p>For example, if a class defines only <code>__lt__</code>, then <code>a &lt; b</code> will work, but <code>a &gt; b</code> will fall back to
21+
Python’s default behavior of comparing object identities, which is almost never what you want.</p>
22+
<p>Python provides the <code>functools.total_ordering</code> decorator specifically to solve this problem. It allows you to define just
23+
<code>__eq__</code> and one other comparison method, and it automatically generates the rest. This ensures consistent and correct comparison behavior
24+
across all operations.</p>
25+
<h3>What is the potential impact?</h3>
26+
<p>Incomplete comparison method implementations can cause several problems:</p>
27+
<ul>
28+
<li><strong>Sorting failures</strong>: built-in functions like <code>sorted()</code> or list methods like <code>sort()</code> may raise exceptions
29+
or produce incorrect ordering</li>
30+
<li><strong>Unexpected comparison results</strong>: code that relies on comparison symmetry may produce wrong results, leading to logic errors</li>
31+
<li><strong>Maintenance difficulties</strong>: future developers may not realize that certain comparison operations are missing, leading to bugs
32+
when they use those operations</li>
33+
<li><strong>Container behavior</strong>: data structures like heaps or priority queues that depend on comparisons may malfunction</li>
34+
</ul>
35+
<p>While this typically will not cause security vulnerabilities, it can lead to application logic errors that are difficult to diagnose and debug.</p>
36+
<h2>How to fix it</h2>
37+
<p>Either use the <code>@functools.total_ordering</code> decorator and define <code>__eq__</code> and one ordering method, typically
38+
<code>__lt__</code>, or define all four ordering methods (<code>__lt__</code>, <code>__le__</code>, <code>__gt__</code>, <code>__ge__</code>)
39+
explicitly.</p>
40+
<h3>Code examples</h3>
41+
<h4>Noncompliant code example</h4>
42+
<pre data-diff-id="1" data-diff-type="noncompliant">
43+
class Person:
44+
def __init__(self, name, age):
45+
self.name = name
46+
self.age = age
47+
48+
def __lt__(self, other): # Noncompliant
49+
return self.age &lt; other.age
50+
</pre>
51+
<h4>Compliant solution</h4>
52+
<pre data-diff-id="1" data-diff-type="compliant">
53+
from functools import total_ordering
54+
55+
@total_ordering
56+
class Person:
57+
def __init__(self, name, age):
58+
self.name = name
59+
self.age = age
60+
61+
def __eq__(self, other):
62+
return self.age == other.age
63+
64+
def __lt__(self, other):
65+
return self.age &lt; other.age
66+
</pre>
67+
<h4>Noncompliant code example</h4>
68+
<pre data-diff-id="2" data-diff-type="noncompliant">
69+
class Point:
70+
def __init__(self, x, y):
71+
self.x = x
72+
self.y = y
73+
74+
def __lt__(self, other): # Noncompliant
75+
return self.x &lt; other.x
76+
</pre>
77+
<h4>Compliant solution</h4>
78+
<pre data-diff-id="2" data-diff-type="compliant">
79+
class Point:
80+
def __init__(self, x, y):
81+
self.x = x
82+
self.y = y
83+
84+
def __lt__(self, other):
85+
return self.x &lt; other.x
86+
87+
def __le__(self, other):
88+
return self.x &lt;= other.x
89+
90+
def __gt__(self, other):
91+
return self.x &gt; other.x
92+
93+
def __ge__(self, other):
94+
return self.x &gt;= other.x
95+
</pre>
96+
<h3>How does this work?</h3>
97+
<p>The <code>@functools.total_ordering</code> decorator automatically generates all missing comparison methods from the ones you define. This ensures
98+
consistent and correct comparison behavior across all operations.</p>
99+
<h2>Resources</h2>
100+
<h3>Documentation</h3>
101+
<ul>
102+
<li>Python Documentation - <a
103+
href="https://docs.python.org/3/library/functools.html#functools.total_ordering"><code>functools.total_ordering</code></a></li>
104+
<li>Python Documentation - <a href="https://docs.python.org/3/reference/datamodel.html#object.&lt;em&gt;lt&lt;/em&gt;">Rich Comparison
105+
Methods</a></li>
106+
<li>PEP 207 - <a href="https://peps.python.org/pep-0207/">Rich Comparisons</a></li>
107+
</ul>
108+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8500.json‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
{
2+
"title": "Comparison methods should be defined completely",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5 min"
8+
},
9+
"tags": [
10+
"pitfall",
11+
"comparison"
12+
],
13+
"defaultSeverity": "Major",
14+
"ruleSpecification": "RSPEC-8500",
15+
"sqKey": "S8500",
16+
"scope": "All",
17+
"quickfix": "unknown",
18+
"code": {
19+
"impacts": {
20+
"RELIABILITY": "HIGH",
21+
"MAINTAINABILITY": "HIGH"
22+
},
23+
"attribute": "COMPLETE"
24+
}
25+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@
333333
"S8493",
334334
"S8494",
335335
"S8495",
336+
"S8500",
336337
"S8503",
337338
"S8504",
338339
"S8505",

‎python-checks/src/test/java/org/sonar/python/checks/IncompleteComparisonMethodsCheckTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* You can redistribute and/or modify this program under the terms of
7+
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks;
18+
19+
import org.junit.jupiter.api.Test;
20+
import org.sonar.python.checks.utils.PythonCheckVerifier;
21+
22+
class IncompleteComparisonMethodsCheckTest {
23+
24+
@Test
25+
void test() {
26+
PythonCheckVerifier.verify("src/test/resources/checks/incompleteComparisonMethods.py", new IncompleteComparisonMethodsCheck());
27+
}
28+
}

0 commit comments

Comments
 (0)