Skip to content

Commit 21653e8

Browse files
sonar-nigel[bot]marc-jasper-sonarsource
authored and
sonartech
committed
SONARPY-4051 Rule S8505 "@singledispatch" and "@singledispatchmethod" should not be confused (#1060)
Co-authored-by: Marc Jasper <marc.jasper@sonarsource.com> GitOrigin-RevId: 7b91ea27e57ef4b847944f9075d67db89d2faac3
1 parent 1e4c9bd commit 21653e8

File tree

7 files changed

+350
-0
lines changed

7 files changed

+350
-0
lines changed

‎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
@@ -406,6 +406,7 @@ public Stream<Class<?>> getChecks() {
406406
SillyIdentityCheck.class,
407407
SingleCharacterAlternationCheck.class,
408408
SingleCharCharacterClassCheck.class,
409+
SingleDispatchMixupCheck.class,
409410
SkippedTestNoReasonCheck.class,
410411
SkLearnEstimatorDontInitializeEstimatedValuesCheck.class,
411412
SleepZeroInAsyncCheck.class,

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

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
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.List;
20+
import org.sonar.check.Rule;
21+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
22+
import org.sonar.plugins.python.api.SubscriptionContext;
23+
import org.sonar.plugins.python.api.tree.Decorator;
24+
import org.sonar.plugins.python.api.tree.FunctionDef;
25+
import org.sonar.plugins.python.api.tree.Tree;
26+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
27+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
28+
29+
@Rule(key = "S8505")
30+
public class SingleDispatchMixupCheck extends PythonSubscriptionCheck {
31+
32+
private static final TypeMatcher SINGLEDISPATCH_MATCHER = TypeMatchers.isType("functools.singledispatch");
33+
private static final TypeMatcher SINGLEDISPATCHMETHOD_MATCHER = TypeMatchers.isType("functools.singledispatchmethod");
34+
private static final TypeMatcher STATICMETHOD_MATCHER = TypeMatchers.isType("builtins.staticmethod");
35+
36+
private static final String MSG_USE_SINGLEDISPATCHMETHOD =
37+
"Use \"@singledispatchmethod\" instead of \"@singledispatch\" on methods defined in a class body.";
38+
private static final String MSG_USE_SINGLEDISPATCH =
39+
"Use \"@singledispatch\" instead of \"@singledispatchmethod\" on standalone functions.";
40+
41+
@Override
42+
public void initialize(Context context) {
43+
context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, SingleDispatchMixupCheck::checkFunctionDef);
44+
}
45+
46+
private static void checkFunctionDef(SubscriptionContext ctx) {
47+
FunctionDef functionDef = (FunctionDef) ctx.syntaxNode();
48+
List<Decorator> decorators = functionDef.decorators();
49+
if (decorators.isEmpty()) {
50+
return;
51+
}
52+
53+
boolean isMethod = functionDef.isMethodDefinition();
54+
55+
for (int i = 0; i < decorators.size(); i++) {
56+
Decorator decorator = decorators.get(i);
57+
if (SINGLEDISPATCH_MATCHER.isTrueFor(decorator.expression(), ctx)) {
58+
// @singledispatch on a method is broken: dispatch happens on self's/cls's type.
59+
// The only valid combination on a method is @staticmethod applied as an outer decorator
60+
if (isMethod && !isWrappedByOuterStaticmethod(decorators, i, ctx)) {
61+
ctx.addIssue(decorator, MSG_USE_SINGLEDISPATCHMETHOD);
62+
}
63+
} else if (SINGLEDISPATCHMETHOD_MATCHER.isTrueFor(decorator.expression(), ctx) && !isMethod) {
64+
ctx.addIssue(decorator, MSG_USE_SINGLEDISPATCH);
65+
}
66+
}
67+
}
68+
69+
private static boolean isWrappedByOuterStaticmethod(List<Decorator> decorators, int innerIndex, SubscriptionContext ctx) {
70+
for (int i = 0; i < innerIndex; i++) {
71+
if (STATICMETHOD_MATCHER.isTrueFor(decorators.get(i).expression(), ctx)) {
72+
return true;
73+
}
74+
}
75+
return false;
76+
}
77+
}

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

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
<p>This rule raises an issue when <code>@singledispatch</code> is used on a class method instead of <code>@singledispatchmethod</code>, or when
2+
<code>@singledispatchmethod</code> is used on a standalone function instead of <code>@singledispatch</code>.</p>
3+
<h2>Why is this an issue?</h2>
4+
<p>The <code>@singledispatch</code> and <code>@singledispatchmethod</code> decorators from <code>functools</code> serve different purposes and are not
5+
interchangeable.</p>
6+
<p><code>@singledispatch</code> is designed for standalone functions. It dispatches based on the type of the first argument. When applied to an
7+
instance method, the first argument is <code>self</code>, so dispatch is performed on the type of the instance instead of on the actual data argument.
8+
The registered type implementations (for example, for <code>int</code> or <code>str</code>) are therefore never selected based on the data, and the
9+
behavior depends on the receiver:</p>
10+
<ul>
11+
<li>When called on an instance of the class where the method is defined, the base implementation is invoked regardless of the data argument’s
12+
type.</li>
13+
<li>When called on an instance of a subclass, dispatch may resolve to a <strong>different</strong> registered implementation depending on the
14+
subclass’s MRO, so the same call can produce different results in different parts of a class hierarchy. This is typically worse than always falling
15+
back to the base implementation, because the bug becomes intermittent and hierarchy-dependent.</li>
16+
</ul>
17+
<p><code>@singledispatchmethod</code> is designed for instance methods and class methods. It returns a non-callable descriptor that relies on the
18+
descriptor protocol to bind to an instance or class. When applied to a standalone function, the resulting object cannot be invoked at all, and calling
19+
it raises <code>TypeError: 'singledispatchmethod' object is not callable</code>.</p>
20+
<p>This rule also flags <code>@singledispatch</code> applied to a method decorated with <code>@classmethod</code>. In that case, the dispatcher
21+
attempts to call a <code>classmethod</code> object directly and raises <code>TypeError</code> at call time.</p>
22+
<p>In all of these cases, no error or warning is raised at decoration time, making the bug silent and hard to detect by reading the code alone.</p>
23+
<h3>Exceptions</h3>
24+
<p>The rule’s behavior with <code>@staticmethod</code> depends on the decorator order:</p>
25+
<ul>
26+
<li><code>@singledispatch</code> placed <strong>above</strong> <code>@staticmethod</code> <strong>is</strong> flagged. In that order,
27+
<code>@singledispatch</code> wraps the <code>staticmethod</code> descriptor object, which prevents it from working as a static method and breaks
28+
dispatch on instances (<code>MyClass().process(value)</code> raises <code>TypeError</code>).</li>
29+
<li><code>@staticmethod</code> placed <strong>above</strong> <code>@singledispatch</code> is <strong>not</strong> flagged, because
30+
<code>@singledispatch</code> is then applied to a regular function and <code>@staticmethod</code> simply prevents Python from binding
31+
<code>self</code>, so dispatch correctly happens on the data argument.</li>
32+
</ul>
33+
<p>Note that even the non-flagged order is uncommon and easy to misuse: stacked decorators must be ordered carefully, and a future refactor that swaps
34+
the order silently reintroduces the bug. Prefer using <code>@singledispatch</code> on a module-level function, or <code>@singledispatchmethod</code>
35+
when the dispatcher needs to live on a class.</p>
36+
<h3>What is the potential impact?</h3>
37+
<p>Confusing these two decorators causes the type dispatch mechanism to behave incorrectly:</p>
38+
<ul>
39+
<li>registered type implementations will not be called for the expected argument</li>
40+
<li>with <code>@singledispatch</code> on an instance method, calls on subclasses may resolve to <strong>different</strong> registered
41+
implementations than calls on the base class, so the same call site can produce different results depending on the receiver’s type</li>
42+
<li>the code may appear to work in simple tests but fail or behave inconsistently in production with different input types or in larger class
43+
hierarchies</li>
44+
<li>debugging is difficult because no error or warning is raised at decoration time</li>
45+
<li>the application may process data incorrectly, potentially leading to data corruption or security issues if type-specific validation or
46+
sanitization is bypassed</li>
47+
<li>combining <code>@singledispatch</code> with <code>@classmethod</code> raises a <code>TypeError</code> at call time</li>
48+
</ul>
49+
<h2>How to fix it</h2>
50+
<h3>Code examples</h3>
51+
<h4>Noncompliant code example</h4>
52+
<p>Using <code>@singledispatch</code> on a class method:</p>
53+
<pre data-diff-id="1" data-diff-type="noncompliant">
54+
from functools import singledispatch
55+
56+
class Processor:
57+
@singledispatch # Noncompliant: should use @singledispatchmethod for methods
58+
def process(self, data):
59+
return f"Processing: {data}"
60+
61+
@process.register(int)
62+
def _(self, data):
63+
return f"Processing int: {data}"
64+
65+
@process.register(str)
66+
def _(self, data):
67+
return f"Processing str: {data}"
68+
</pre>
69+
<h4>Compliant solution</h4>
70+
<pre data-diff-id="1" data-diff-type="compliant">
71+
from functools import singledispatchmethod
72+
73+
class Processor:
74+
@singledispatchmethod
75+
def process(self, data):
76+
return f"Processing: {data}"
77+
78+
@process.register(int)
79+
def _(self, data):
80+
return f"Processing int: {data}"
81+
82+
@process.register(str)
83+
def _(self, data):
84+
return f"Processing str: {data}"
85+
</pre>
86+
<h4>Noncompliant code example</h4>
87+
<p>Using <code>@singledispatchmethod</code> on a standalone function:</p>
88+
<pre data-diff-id="2" data-diff-type="noncompliant">
89+
from functools import singledispatchmethod
90+
91+
@singledispatchmethod # Noncompliant: should use @singledispatch for standalone functions
92+
def process(data):
93+
return f"Processing: {data}"
94+
95+
@process.register(int)
96+
def _(data):
97+
return f"Processing int: {data}"
98+
99+
@process.register(str)
100+
def _(data):
101+
return f"Processing str: {data}"
102+
</pre>
103+
<h4>Compliant solution</h4>
104+
<pre data-diff-id="2" data-diff-type="compliant">
105+
from functools import singledispatch
106+
107+
@singledispatch
108+
def process(data):
109+
return f"Processing: {data}"
110+
111+
@process.register(int)
112+
def _(data):
113+
return f"Processing int: {data}"
114+
115+
@process.register(str)
116+
def _(data):
117+
return f"Processing str: {data}"
118+
</pre>
119+
<h3>How does this work?</h3>
120+
<p>Use <code>@singledispatch</code> for standalone, module-level functions and <code>@singledispatchmethod</code> for instance methods and class
121+
methods. The <code>@singledispatch</code> decorator dispatches on the first argument, while <code>@singledispatchmethod</code> skips <code>self</code>
122+
or <code>cls</code> and dispatches on the next argument.</p>
123+
<p>When combining <code>@singledispatchmethod</code> with <code>@classmethod</code>, <code>@singledispatchmethod</code> must remain the outermost
124+
decorator so that the registration mechanism (<code>process.register</code>) is still available. <code>@classmethod</code> is then applied to the base
125+
implementation and to each registered overload:</p>
126+
<pre>
127+
from functools import singledispatchmethod
128+
129+
class Processor:
130+
@singledispatchmethod
131+
@classmethod
132+
def process(cls, data):
133+
return f"Processing: {data}"
134+
135+
@process.register
136+
@classmethod
137+
def _(cls, data: int):
138+
return f"Processing int: {data}"
139+
</pre>
140+
<p>For static methods, prefer turning the function into a module-level function decorated with <code>@singledispatch</code>. If the dispatcher must
141+
live on the class, place <code>@staticmethod</code> <strong>above</strong> <code>@singledispatch</code> so that <code>@singledispatch</code> wraps a
142+
regular function rather than a <code>staticmethod</code> descriptor; the inverse order breaks dispatch on instance calls and is reported by this
143+
rule.</p>
144+
<h2>Resources</h2>
145+
<h3>Documentation</h3>
146+
<ul>
147+
<li>Python Documentation - <a
148+
href="https://docs.python.org/3/library/functools.html#functools.singledispatch"><code>functools.singledispatch</code></a></li>
149+
<li>Python Documentation - <a
150+
href="https://docs.python.org/3/library/functools.html#functools.singledispatchmethod"><code>functools.singledispatchmethod</code></a></li>
151+
<li>PEP 443 - <a href="https://www.python.org/dev/peps/pep-0443/">Single-dispatch generic functions</a></li>
152+
</ul>
153+

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
{
2+
"title": "\"@singledispatch\" and \"@singledispatchmethod\" should not be confused",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"pitfall",
11+
"confusing"
12+
],
13+
"defaultSeverity": "Blocker",
14+
"ruleSpecification": "RSPEC-8505",
15+
"sqKey": "S8505",
16+
"scope": "Main",
17+
"quickfix": "no",
18+
"code": {
19+
"impacts": {
20+
"RELIABILITY": "BLOCKER",
21+
"MAINTAINABILITY": "HIGH"
22+
},
23+
"attribute": "LOGICAL"
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
@@ -332,6 +332,7 @@
332332
"S8494",
333333
"S8495",
334334
"S8504",
335+
"S8505",
335336
"S8507",
336337
"S8513",
337338
"S8517",

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
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 SingleDispatchMixupCheckTest {
23+
24+
@Test
25+
void test() {
26+
PythonCheckVerifier.verify("src/test/resources/checks/singleDispatchMixup.py", new SingleDispatchMixupCheck());
27+
}
28+
29+
}

‎python-checks/src/test/resources/checks/singleDispatchMixup.py‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
from functools import singledispatch, singledispatchmethod
2+
3+
4+
class WithSingledispatch:
5+
@singledispatch # Noncompliant {{Use "@singledispatchmethod" instead of "@singledispatch" on methods defined in a class body.}}
6+
#^^^^^^^^^^^^^^^
7+
def process(self, data):
8+
pass
9+
10+
11+
class WithWrongStaticmethodOrder:
12+
@singledispatch # Noncompliant {{Use "@singledispatchmethod" instead of "@singledispatch" on methods defined in a class body.}}
13+
@staticmethod
14+
def handle(data):
15+
pass
16+
17+
18+
@singledispatchmethod # Noncompliant {{Use "@singledispatch" instead of "@singledispatchmethod" on standalone functions.}}
19+
#^[sc=1;ec=21]
20+
def process_standalone(data):
21+
pass
22+
23+
24+
@singledispatch
25+
def compliant_standalone(data):
26+
pass
27+
28+
29+
class WithSingledispatchmethod:
30+
@singledispatchmethod
31+
def process(self, data):
32+
pass
33+
34+
35+
class WithValidStaticmethodOrder:
36+
@staticmethod
37+
@singledispatch
38+
def process(data):
39+
pass
40+
41+
42+
class WithClassmethodAndSingledispatch:
43+
@singledispatch # Noncompliant {{Use "@singledispatchmethod" instead of "@singledispatch" on methods defined in a class body.}}
44+
@classmethod
45+
def handle(cls, data):
46+
pass
47+
48+
49+
class WithSingledispatchmethodAndClassmethod:
50+
@singledispatchmethod
51+
@classmethod
52+
def process(cls, data):
53+
pass
54+
55+
56+
class WithNestedFunction:
57+
def outer(self):
58+
@singledispatch # Compliant: nested function, not a method definition
59+
def inner(data):
60+
pass
61+
62+
63+
def no_dispatch(data):
64+
pass

0 commit comments

Comments
 (0)