Skip to content

Commit 1dc4218

Browse files
guillaume-dequenneguillaume-dequenne
committed
SONARPY-1934 Migrate rule S3699 UseOfEmptyReturnValue to typeV2 (#1865)
1 parent 02bd1da commit 1dc4218

File tree

11 files changed

+235
-35
lines changed

11 files changed

+235
-35
lines changed

‎its/ruling/src/test/resources/expected/python-S3699.json

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
11
{
2-
"project:biopython/Bio/Restriction/Restriction.py": [
3-
2432
4-
],
52
"project:django-2.2.3/django/core/files/locks.py": [
63
108,
74
112

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
import org.sonar.plugins.python.api.tree.CallExpression;
2828
import org.sonar.plugins.python.api.tree.Expression;
2929
import org.sonar.plugins.python.api.tree.RegularArgument;
30-
import org.sonar.python.types.InferredTypes;
30+
import org.sonar.python.types.v2.TriBool;
31+
import org.sonar.python.types.v2.TypeCheckBuilder;
3132

3233
import static org.sonar.plugins.python.api.tree.Tree.Kind.ASSIGNMENT_STMT;
3334
import static org.sonar.plugins.python.api.tree.Tree.Kind.CALL_EXPR;
@@ -45,7 +46,12 @@ public void initialize(Context context) {
4546
}
4647

4748
private static void checkReturnValue(Expression expression, SubscriptionContext ctx) {
48-
if (expression.is(CALL_EXPR) && expression.type().equals(InferredTypes.NONE)) {
49+
if (!expression.is(CALL_EXPR)) {
50+
return;
51+
}
52+
TypeCheckBuilder typeCheckBuilder = ctx.typeChecker().typeCheckBuilder().isBuiltinWithName("NoneType");
53+
boolean noneType = typeCheckBuilder.check(expression.typeV2()) == TriBool.TRUE;
54+
if (noneType) {
4955
CallExpression callExpression = (CallExpression) expression;
5056
Optional.ofNullable(callExpression.calleeSymbol())
5157
.ifPresent(symbol -> ctx.addIssue(expression, String.format(MESSAGE, symbol.name(), symbol.name())));

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,40 @@ def win32():
3030
# We should not raise issues as the stubs of win32 are inaccurate
3131
path = win32pdh.MakeCounterPath((None, None, None, None, -1, None)) # OK
3232
hc = win32pdh.AddCounter(None, path)
33+
34+
35+
class SomeClass:
36+
def __init__(self):
37+
...
38+
39+
class SomeOtherClass:
40+
...
41+
42+
def foo():
43+
# FN: we should fallback to "type.__init__" when actual type not available
44+
x = SomeClass.__init__(...) # FN
45+
y = SomeOtherClass.__init__(...) # FN SONARPY-2007
46+
47+
48+
def using_fcntl():
49+
import fcntl
50+
ret = fcntl.flock(..., ...) # Noncompliant
51+
52+
53+
54+
def list_comprehensions():
55+
exp_list = [x() for y in unknown()]
56+
actual_list = [...]
57+
for _ in range(len(exp_list)):
58+
actual_list.append(smth())
59+
self.assertEqual(
60+
exp_list.sort(), # Noncompliant
61+
actual_list.sort() # Noncompliant
62+
)
63+
64+
65+
def import_in_different_branch():
66+
if x:
67+
import fcntl
68+
def lock():
69+
ret = fcntl.flock(..., ...) # Noncompliant

‎python-frontend/src/main/java/org/sonar/python/semantic/v2/SymbolsModuleTypeProvider.java

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,16 @@
4949
public class SymbolsModuleTypeProvider {
5050
private final ProjectLevelSymbolTable projectLevelSymbolTable;
5151
private final TypeShed typeShed;
52+
private ModuleType rootModule;
5253

5354
public SymbolsModuleTypeProvider(ProjectLevelSymbolTable projectLevelSymbolTable, TypeShed typeShed) {
5455
this.projectLevelSymbolTable = projectLevelSymbolTable;
5556
this.typeShed = typeShed;
57+
this.rootModule = createModuleFromSymbols(null, null, typeShed.builtinSymbols().values());
5658
}
5759

5860
public ModuleType createBuiltinModule() {
59-
return createModuleFromSymbols(null, null, typeShed.builtinSymbols().values());
61+
return rootModule;
6062
}
6163

6264
public ModuleType createModuleType(List<String> moduleFqn, ModuleType parent) {
@@ -117,10 +119,7 @@ private PythonType convertToFunctionType(FunctionSymbol symbol, Map<Symbol, Pyth
117119

118120
InferredType inferredType = ((FunctionSymbolImpl) symbol).declaredReturnType();
119121
ClassSymbol classSymbol = inferredType.runtimeTypeSymbol();
120-
var returnType = PythonType.UNKNOWN;
121-
if (classSymbol != null) {
122-
returnType = convertToType(classSymbol, createdTypesBySymbol);
123-
}
122+
var returnType = resolveReturnType(createdTypesBySymbol, classSymbol);
124123

125124
FunctionTypeBuilder functionTypeBuilder =
126125
new FunctionTypeBuilder(symbol.name())
@@ -137,15 +136,20 @@ private PythonType convertToFunctionType(FunctionSymbol symbol, Map<Symbol, Pyth
137136
return functionType;
138137
}
139138

140-
private static ParameterV2 convertParameter(FunctionSymbol.Parameter parameter) {
141-
return new ParameterV2(parameter.name(),
142-
PythonType.UNKNOWN,
143-
parameter.hasDefaultValue(),
144-
parameter.isKeywordOnly(),
145-
parameter.isPositionalOnly(),
146-
parameter.isKeywordVariadic(),
147-
parameter.isPositionalVariadic(),
148-
null);
139+
private PythonType resolveReturnType(Map<Symbol, PythonType> createdTypesBySymbol, @Nullable ClassSymbol classSymbol) {
140+
if (classSymbol == null) {
141+
return PythonType.UNKNOWN;
142+
}
143+
if (rootModule == null) {
144+
return convertToType(classSymbol, createdTypesBySymbol);
145+
}
146+
// If the symbol is a built-in symbol and the root module already exists, we search for it instead of recreating the symbol
147+
// This is necessary to avoid duplicated symbols in the original ProjectSymbolTable to translate into duplicated PythonType
148+
// TODO: SONARPY-2010 to fix this
149+
return Optional.ofNullable(classSymbol.fullyQualifiedName())
150+
.filter(fqn -> !fqn.contains("."))
151+
.flatMap(f -> rootModule.resolveMember(f))
152+
.orElseGet(() -> convertToType(classSymbol, createdTypesBySymbol));
149153
}
150154

151155
private PythonType convertToClassType(ClassSymbol symbol, Map<Symbol, PythonType> createdTypesBySymbol) {
@@ -176,6 +180,17 @@ private PythonType convertToClassType(ClassSymbol symbol, Map<Symbol, PythonType
176180
return classType;
177181
}
178182

183+
private static ParameterV2 convertParameter(FunctionSymbol.Parameter parameter) {
184+
return new ParameterV2(parameter.name(),
185+
PythonType.UNKNOWN,
186+
parameter.hasDefaultValue(),
187+
parameter.isKeywordOnly(),
188+
parameter.isPositionalOnly(),
189+
parameter.isKeywordVariadic(),
190+
parameter.isPositionalVariadic(),
191+
null);
192+
}
193+
179194
private Symbol typeshedSymbolWithFQN(String fullyQualifiedName) {
180195
String[] fqnSplitByDot = fullyQualifiedName.split("\\.");
181196
String localName = fqnSplitByDot[fqnSplitByDot.length - 1];

‎python-frontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ public void visitName(Name name) {
440440
.map(Expression.class::cast)
441441
.map(Expression::typeV2)
442442
// TODO: classes (SONARPY-1829) and functions should be propagated like other types
443-
.filter(t -> (t instanceof ClassType) || (t instanceof FunctionType))
443+
.filter(t -> (t instanceof ClassType) || (t instanceof FunctionType) || (t instanceof ModuleType))
444444
.ifPresent(type -> setTypeToName(name, type));
445445
}
446446

‎python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,12 @@ public TypeCheckBuilder isTypeHintTypeSource() {
4747
return this;
4848
}
4949

50+
public TypeCheckBuilder isBuiltinWithName(String name) {
51+
PythonType builtinType = projectLevelTypeTable.getModule().resolveMember(name).orElse(PythonType.UNKNOWN);
52+
predicates.add(new IsSameAsTypePredicate(builtinType));
53+
return this;
54+
}
55+
5056
public TriBool check(PythonType pythonType) {
5157
TriBool result = TriBool.TRUE;
5258
for (TypePredicate predicate : predicates) {
@@ -70,6 +76,7 @@ public HasMemberTypePredicate(String memberName) {
7076
this.memberName = memberName;
7177
}
7278

79+
@Override
7380
public TriBool test(PythonType pythonType) {
7481
return pythonType.hasMember(memberName);
7582
}
@@ -82,6 +89,7 @@ public InstancesHaveMemberTypePredicate(String memberName) {
8289
this.memberName = memberName;
8390
}
8491

92+
@Override
8593
public TriBool test(PythonType pythonType) {
8694
if (pythonType instanceof ClassType classType) {
8795
return classType.instancesHaveMember(memberName);
@@ -97,4 +105,24 @@ public TriBool test(PythonType pythonType) {
97105
return pythonType.typeSource() == typeSource ? TriBool.TRUE : TriBool.FALSE;
98106
}
99107
}
108+
109+
static class IsSameAsTypePredicate implements TypePredicate {
110+
111+
PythonType expectedType;
112+
113+
public IsSameAsTypePredicate(PythonType expectedType) {
114+
this.expectedType = expectedType;
115+
}
116+
117+
@Override
118+
public TriBool test(PythonType pythonType) {
119+
if (pythonType instanceof ObjectType objectType) {
120+
pythonType = objectType.unwrappedType();
121+
}
122+
if (pythonType == PythonType.UNKNOWN || expectedType == PythonType.UNKNOWN) {
123+
return TriBool.UNKNOWN;
124+
}
125+
return pythonType.equals(expectedType) ? TriBool.TRUE : TriBool.FALSE;
126+
}
127+
}
100128
}

‎python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeInferenceV2Test.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1791,7 +1791,6 @@ def bar(self): ...
17911791
}
17921792

17931793
@Test
1794-
@Disabled("Duplicate PythonType for NONE_TYPE")
17951794
void imported_symbol_call_return_type() {
17961795
assertThat(lastExpression(
17971796
"""
@@ -1812,6 +1811,20 @@ void basic_imported_symbol() {
18121811
).typeV2()).isInstanceOf(ModuleType.class);
18131812
}
18141813

1814+
@Test
1815+
void imported_symbol_in_different_branch() {
1816+
FileInput fileInput = inferTypes("""
1817+
if x:
1818+
import fcntl
1819+
def lock():
1820+
fcntl
1821+
""");
1822+
Statement functionDef = fileInput.statements().statements().get(1);
1823+
ExpressionStatement fcntlStatement = ((ExpressionStatement) TreeUtils.firstChild(functionDef, t -> t.is(Tree.Kind.EXPRESSION_STMT)).get());
1824+
assertThat(fcntlStatement.expressions().get(0).typeV2()).isInstanceOf(ModuleType.class);
1825+
assertThat(fcntlStatement.expressions().get(0).typeV2().name()).isEqualTo("fcntl");
1826+
}
1827+
18151828
@Test
18161829
void basic_imported_symbols() {
18171830
FileInput fileInput = inferTypes(

‎python-frontend/src/test/java/org/sonar/python/types/TypeInferenceTest.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,25 @@ void returned_value_type() {
339339
assertThat(((ReturnStatement) lastStatement("return (42, 1337)")).returnValueType()).isEqualTo(TUPLE);
340340
}
341341

342+
@Test
343+
void random() {
344+
assertThat(lastExpressionInFunction(
345+
"x = [1,2,3]",
346+
"a = x.append(42)",
347+
"a").type()).isEqualTo(NONE);
348+
}
349+
350+
@Test
351+
void imported_symbol() {
352+
assertThat(lastExpression(
353+
"""
354+
import fcntl
355+
ret = fcntl.flock(..., ...)
356+
ret
357+
"""
358+
).type()).isEqualTo(NONE);
359+
}
360+
342361
@Test
343362
void propagate_return_type_to_variable() {
344363
assertThat(lastExpressionInFunction(

‎python-frontend/src/test/java/org/sonar/python/types/v2/ClassTypeTest.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,8 @@
3131
import org.sonar.plugins.python.api.tree.Name;
3232
import org.sonar.plugins.python.api.tree.Tree;
3333
import org.sonar.python.PythonTestUtils;
34-
import org.sonar.python.semantic.ProjectLevelSymbolTable;
3534
import org.sonar.python.semantic.SymbolUtils;
3635
import org.sonar.python.semantic.v2.ClassTypeBuilder;
37-
import org.sonar.python.semantic.v2.ProjectLevelTypeTable;
3836
import org.sonar.python.semantic.v2.SymbolTableBuilderV2;
3937
import org.sonar.python.semantic.v2.SymbolV2;
4038
import org.sonar.python.semantic.v2.TypeInferenceV2;
@@ -48,7 +46,6 @@
4846
public class ClassTypeTest {
4947

5048
static PythonFile pythonFile = PythonTestUtils.pythonFile("");
51-
TypeChecker typeChecker = new TypeChecker(new ProjectLevelTypeTable(ProjectLevelSymbolTable.empty()));
5249

5350
@Test
5451
void no_parents() {
@@ -65,9 +62,6 @@ void no_parents() {
6562
assertThat(classType.hasMember("unknown")).isEqualTo(TriBool.UNKNOWN);
6663
assertThat(classType.instancesHaveMember("__call__")).isEqualTo(TriBool.FALSE);
6764
assertThat(classType.instancesHaveMember("unknown")).isEqualTo(TriBool.FALSE);
68-
assertThat(typeChecker.typeCheckBuilder().hasMember("__call__").check(classType)).isEqualTo(TriBool.TRUE);
69-
assertThat(typeChecker.typeCheckBuilder().hasMember("unknown").check(classType)).isEqualTo(TriBool.UNKNOWN);
70-
assertThat(typeChecker.typeCheckBuilder().instancesHaveMember("__call__").check(classType)).isEqualTo(TriBool.FALSE);
7165

7266
assertThat(classType.displayName()).contains("type");
7367
assertThat(classType.instanceDisplayName()).contains("C");

‎python-frontend/src/test/java/org/sonar/python/types/v2/ObjectTypeTest.java

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,7 @@
3636
import org.sonar.plugins.python.api.tree.Tree;
3737
import org.sonar.plugins.python.api.tree.Tuple;
3838
import org.sonar.python.PythonTestUtils;
39-
import org.sonar.python.semantic.ProjectLevelSymbolTable;
4039
import org.sonar.python.semantic.SymbolUtils;
41-
import org.sonar.python.semantic.v2.ProjectLevelTypeTable;
4240
import org.sonar.python.tree.TreeUtils;
4341

4442
import static org.assertj.core.api.Assertions.assertThat;
@@ -47,8 +45,6 @@
4745

4846
class ObjectTypeTest {
4947

50-
TypeChecker typeChecker = new TypeChecker(new ProjectLevelTypeTable(ProjectLevelSymbolTable.empty()));
51-
5248
@Test
5349
void simpleObject() {
5450
PythonFile pythonFile = PythonTestUtils.pythonFile("");
@@ -65,8 +61,6 @@ class A: ...
6561
assertThat(objectType.displayName()).contains("A");
6662
assertThat(objectType.isCompatibleWith(classType)).isTrue();
6763
assertThat(objectType.hasMember("foo")).isEqualTo(TriBool.FALSE);
68-
assertThat(typeChecker.typeCheckBuilder().hasMember("foo").check(objectType)).isEqualTo(TriBool.FALSE);
69-
assertThat(typeChecker.typeCheckBuilder().instancesHaveMember("foo").check(objectType)).isEqualTo(TriBool.FALSE);
7064
String fileId = SymbolUtils.pathOf(pythonFile).toString();
7165
assertThat(objectType.definitionLocation()).contains(new LocationInFile(fileId, 1, 6, 1, 7));
7266
}
@@ -86,8 +80,6 @@ def foo(self): ...
8680
assertThat(objectType.displayName()).contains("A");
8781
assertThat(objectType.isCompatibleWith(classType)).isTrue();
8882
assertThat(objectType.hasMember("foo")).isEqualTo(TriBool.TRUE);
89-
assertThat(typeChecker.typeCheckBuilder().hasMember("foo").check(objectType)).isEqualTo(TriBool.TRUE);
90-
assertThat(typeChecker.typeCheckBuilder().instancesHaveMember("foo").check(objectType)).isEqualTo(TriBool.FALSE);
9183
}
9284

9385
@Test

0 commit comments

Comments
 (0)