From 9387dba3fa47844fc38bcf488968e31f1471d51d Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Thu, 6 Aug 2020 15:36:38 +0200 Subject: [PATCH 1/4] Add InferredType::mustBeOrExtend API --- .../python/api/types/InferredType.java | 3 +++ .../java/org/sonar/python/types/AnyType.java | 5 ++++ .../org/sonar/python/types/DeclaredType.java | 13 +++++++++ .../org/sonar/python/types/RuntimeType.java | 4 +++ .../org/sonar/python/types/UnionType.java | 4 +++ .../org/sonar/python/types/AnyTypeTest.java | 5 ++++ .../sonar/python/types/DeclaredTypeTest.java | 27 +++++++++++++++++++ .../sonar/python/types/RuntimeTypeTest.java | 16 +++++++++++ .../org/sonar/python/types/UnionTypeTest.java | 11 ++++++++ 9 files changed, 88 insertions(+) diff --git a/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java b/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java index 8cd70875ac..cc9f5c1aa8 100644 --- a/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java +++ b/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java @@ -44,4 +44,7 @@ public interface InferredType { @Beta boolean isCompatibleWith(InferredType other); + @Beta + boolean mustBeOrExtend(String typeName); + } diff --git a/python-frontend/src/main/java/org/sonar/python/types/AnyType.java b/python-frontend/src/main/java/org/sonar/python/types/AnyType.java index b7ef74595f..c9dfce0880 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/AnyType.java +++ b/python-frontend/src/main/java/org/sonar/python/types/AnyType.java @@ -55,4 +55,9 @@ public boolean canBeOrExtend(String typeName) { public boolean isCompatibleWith(InferredType other) { return true; } + + @Override + public boolean mustBeOrExtend(String typeName) { + return false; + } } diff --git a/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java b/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java index c12034c07b..6c3c658fd4 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java +++ b/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java @@ -25,6 +25,9 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.sonar.plugins.python.api.symbols.AmbiguousSymbol; +import org.sonar.plugins.python.api.symbols.ClassSymbol; import org.sonar.plugins.python.api.symbols.Symbol; import org.sonar.plugins.python.api.types.BuiltinTypes; import org.sonar.plugins.python.api.types.InferredType; @@ -92,6 +95,16 @@ public boolean isCompatibleWith(InferredType other) { return true; } + @Override + public boolean mustBeOrExtend(String typeName) { + return alternativeTypeSymbols().stream().flatMap(a -> { + if (a.is(Symbol.Kind.AMBIGUOUS)) { + return ((AmbiguousSymbol) a).alternatives().stream().filter(alternative -> alternative.is(Symbol.Kind.CLASS)); + } + return Stream.of(a); + }).allMatch(a -> ((ClassSymbol) a).isOrExtends(typeName)); + } + @Override public String toString() { return "DeclaredType(" + typeName() + ')'; diff --git a/python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java b/python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java index a537874338..920ea3a9d9 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java +++ b/python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java @@ -86,6 +86,10 @@ public boolean isCompatibleWith(InferredType other) { return true; } + public boolean mustBeOrExtend(String fullyQualifiedName) { + return typeClass.isOrExtends(fullyQualifiedName); + } + private boolean areSymbolsCompatible(Symbol other) { if (!other.is(CLASS)) { return true; diff --git a/python-frontend/src/main/java/org/sonar/python/types/UnionType.java b/python-frontend/src/main/java/org/sonar/python/types/UnionType.java index b8ba670fe8..31e0588f15 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/UnionType.java +++ b/python-frontend/src/main/java/org/sonar/python/types/UnionType.java @@ -96,6 +96,10 @@ public boolean isCompatibleWith(InferredType other) { return types.stream().anyMatch(t -> t.isCompatibleWith(other)); } + public boolean mustBeOrExtend(String fullyQualifiedName) { + return types.stream().allMatch(t -> t.mustBeOrExtend(fullyQualifiedName)); + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/python-frontend/src/test/java/org/sonar/python/types/AnyTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/AnyTypeTest.java index c595a5362f..fda0c53f3d 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/AnyTypeTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/AnyTypeTest.java @@ -54,4 +54,9 @@ public void test_canBeOrExtend() { assertThat(InferredTypes.INT.isCompatibleWith(ANY)).isTrue(); assertThat(ANY.isCompatibleWith(InferredTypes.INT)).isTrue(); } + + @Test + public void test_mustBeOrExtend() { + assertThat(ANY.mustBeOrExtend("a")).isFalse(); + } } diff --git a/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java index c73f8955f7..8e5416a101 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java @@ -21,7 +21,12 @@ import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.Set; import org.junit.Test; +import org.sonar.plugins.python.api.symbols.AmbiguousSymbol; +import org.sonar.plugins.python.api.symbols.Symbol; +import org.sonar.python.semantic.AmbiguousSymbolImpl; import org.sonar.python.semantic.ClassSymbolImpl; import org.sonar.python.semantic.SymbolImpl; @@ -93,6 +98,28 @@ public void test_isCompatibleWith() { assertThat(new DeclaredType(x1).isCompatibleWith(new DeclaredType(x2))).isTrue(); } + @Test + public void test_mustBeOrExtend() { + ClassSymbolImpl x1 = new ClassSymbolImpl("x1", "x1"); + ClassSymbolImpl x2 = new ClassSymbolImpl("x2", "x2"); + x2.addSuperClass(x1); + + DeclaredType typeX1 = new DeclaredType(x1); + DeclaredType typeX2 = new DeclaredType(x2); + + assertThat(typeX1.mustBeOrExtend("x1")).isTrue(); + assertThat(typeX1.mustBeOrExtend("x2")).isFalse(); + + assertThat(typeX2.mustBeOrExtend("x1")).isTrue(); + assertThat(typeX2.mustBeOrExtend("x2")).isTrue(); + + ClassSymbolImpl otherX1 = new ClassSymbolImpl("x1", "x1"); + Set symbols = new HashSet<>(Arrays.asList(x1, otherX1)); + AmbiguousSymbol ambiguousSymbol = new AmbiguousSymbolImpl("x1", "x1", symbols); + DeclaredType typeAmbiguousX1 = new DeclaredType(ambiguousSymbol); + assertThat(typeAmbiguousX1.mustBeOrExtend("x1")).isTrue(); + } + @Test public void test_getClass() { ClassSymbolImpl x1 = new ClassSymbolImpl("x1", "x1"); diff --git a/python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java index bf78bce4b6..fae49754da 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java @@ -246,4 +246,20 @@ public void test_isCompatibleWith_NoneType() { assertThat(new RuntimeType(none).isCompatibleWith(new RuntimeType(x1))).isFalse(); assertThat(new RuntimeType(none).isCompatibleWith(new RuntimeType(none))).isTrue(); } + + @Test + public void test_mustBeOrExtend() { + ClassSymbolImpl x1 = new ClassSymbolImpl("x1", "x1"); + ClassSymbolImpl x2 = new ClassSymbolImpl("x2", "x2"); + x2.addSuperClass(x1); + + RuntimeType typeX1 = new RuntimeType(x1); + RuntimeType typeX2 = new RuntimeType(x2); + + assertThat(typeX1.mustBeOrExtend("x1")).isTrue(); + assertThat(typeX1.mustBeOrExtend("x2")).isFalse(); + + assertThat(typeX2.mustBeOrExtend("x1")).isTrue(); + assertThat(typeX2.mustBeOrExtend("x2")).isTrue(); + } } diff --git a/python-frontend/src/test/java/org/sonar/python/types/UnionTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/UnionTypeTest.java index 2e302d5439..69860193a0 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/UnionTypeTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/UnionTypeTest.java @@ -159,4 +159,15 @@ public void test_isCompatibleWith_NoneType() { assertThat(or(new RuntimeType(x1), new RuntimeType(none)).isCompatibleWith(or(new RuntimeType(x2), new RuntimeType(none)))).isTrue(); } + @Test + public void test_mustBeOrExtend() { + ClassSymbolImpl x1 = new ClassSymbolImpl("x1", "x1"); + ClassSymbolImpl x2 = new ClassSymbolImpl("x2", "x2"); + x2.addSuperClass(x1); + + assertThat(or(new RuntimeType(x1), new RuntimeType(x2)).mustBeOrExtend("x1")).isTrue(); + assertThat(or(new RuntimeType(x1), new RuntimeType(x2)).mustBeOrExtend("x2")).isFalse(); + + assertThat(or(new RuntimeType(x1), new RuntimeType(x1)).mustBeOrExtend("x1")).isTrue(); + } } From 17f49bd6e17fe379504cb75345354d78fe32c525 Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Thu, 6 Aug 2020 16:31:29 +0200 Subject: [PATCH 2/4] Update InferredTypes to handle non-subscription aliases and None --- .../test/resources/expected/python-S5727.json | 4 ++++ .../org/sonar/python/types/InferredTypes.java | 18 +++++++++++---- .../sonar/python/types/InferredTypesTest.java | 22 ++++++++++++++++++- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/its/ruling/src/test/resources/expected/python-S5727.json b/its/ruling/src/test/resources/expected/python-S5727.json index 8d708f69c1..95553aacab 100644 --- a/its/ruling/src/test/resources/expected/python-S5727.json +++ b/its/ruling/src/test/resources/expected/python-S5727.json @@ -5,6 +5,10 @@ 'project:buildbot-0.8.6p1/buildbot/status/web/feeds.py':[ 240, ], +'project:django-2.2.3/django/core/files/locks.py':[ +109, +113, +], 'project:tensorflow/python/keras/layers/normalization.py':[ 1131, ], diff --git a/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java b/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java index 1332b1304d..31ba3634c7 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java +++ b/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java @@ -131,7 +131,8 @@ private static DeclaredType declaredTypeFromTypeAnnotation(Expression expression if (expression.is(Kind.NAME) && !((Name) expression).name().equals("Any")) { Symbol symbol = ((Name) expression).symbol(); if (symbol != null) { - return new DeclaredType(symbol); + String builtinFqn = ALIASED_ANNOTATIONS.get(symbol.fullyQualifiedName()); + return builtinFqn != null ? new DeclaredType(builtinSymbols.get(builtinFqn)) : new DeclaredType(symbol); } } if (expression.is(Kind.SUBSCRIPTION)) { @@ -149,16 +150,22 @@ private static DeclaredType declaredTypeFromTypeAnnotation(Expression expression }) .orElse(null); } + if (expression.is(Kind.NONE)) { + return new DeclaredType(builtinSymbols.get(BuiltinTypes.NONE_TYPE)); + } return null; } private static InferredType runtimeTypefromTypeAnnotation(Expression expression, Map builtinSymbols) { if (expression.is(Kind.NAME) && !((Name) expression).name().equals("Any")) { Symbol symbol = ((Name) expression).symbol(); - if (symbol != null && "typing.Text".equals(symbol.fullyQualifiedName())) { - return InferredTypes.runtimeType(builtinSymbols.get("str")); + if (symbol != null ) { + if ("typing.Text".equals(symbol.fullyQualifiedName())) { + return InferredTypes.runtimeType(builtinSymbols.get("str")); + } + return InferredTypes.genericType(symbol, Collections.emptyList(), builtinSymbols); } - return InferredTypes.runtimeType(symbol); + return InferredTypes.anyType(); } if (expression.is(Kind.SUBSCRIPTION)) { SubscriptionExpression subscription = (SubscriptionExpression) expression; @@ -166,6 +173,9 @@ private static InferredType runtimeTypefromTypeAnnotation(Expression expression, .map(symbol -> InferredTypes.genericType(symbol, subscription.subscripts().expressions(), builtinSymbols)) .orElse(InferredTypes.anyType()); } + if (expression.is(Kind.NONE)) { + return InferredTypes.runtimeType(builtinSymbols.get(BuiltinTypes.NONE_TYPE)); + } return InferredTypes.anyType(); } diff --git a/python-frontend/src/test/java/org/sonar/python/types/InferredTypesTest.java b/python-frontend/src/test/java/org/sonar/python/types/InferredTypesTest.java index f584ababa7..0c37875141 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/InferredTypesTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/InferredTypesTest.java @@ -36,6 +36,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.sonar.python.types.InferredTypes.INT; +import static org.sonar.python.types.InferredTypes.NONE; import static org.sonar.python.types.InferredTypes.STR; import static org.sonar.python.types.InferredTypes.anyType; import static org.sonar.python.types.InferredTypes.fromTypeAnnotation; @@ -137,6 +138,13 @@ public void test_aliased_type_annotations() { ); type = fromTypeAnnotation(typeAnnotation); assertThat(typeName(type)).isEqualTo("Deque[int]"); + + typeAnnotation = typeAnnotation( + "from typing import List", + "l : List" + ); + assertThat(fromTypeshedTypeAnnotation(typeAnnotation)).isEqualTo(InferredTypes.LIST); + assertThat(((DeclaredType) fromTypeAnnotation(typeAnnotation)).alternativeTypeSymbols()).extracting(Symbol::fullyQualifiedName).containsExactly("list"); } private void assertAliasedTypeAnnotation(String type, String... code) { @@ -202,13 +210,25 @@ public void test_text_annotation() { .containsExactlyInAnyOrder("str"); } + @Test + public void test_none_annotation() { + TypeAnnotation typeAnnotation = typeAnnotation( + "l : None" + ); + assertThat(fromTypeshedTypeAnnotation(typeAnnotation)).isEqualTo(NONE); + InferredType declaredType = fromTypeAnnotation(typeAnnotation); + assertThat(declaredType).isInstanceOf(DeclaredType.class); + assertThat(((DeclaredType) declaredType).alternativeTypeSymbols()).extracting(Symbol::fullyQualifiedName) + .containsExactlyInAnyOrder("NoneType"); + } + @Test public void test_optional_type_annotations() { TypeAnnotation typeAnnotation = typeAnnotation( "from typing import Optional", "l : Optional[int]" ); - assertThat(fromTypeshedTypeAnnotation(typeAnnotation)).isEqualTo(InferredTypes.or(InferredTypes.INT, InferredTypes.NONE)); + assertThat(fromTypeshedTypeAnnotation(typeAnnotation)).isEqualTo(InferredTypes.or(InferredTypes.INT, NONE)); InferredType declaredType = fromTypeAnnotation(typeAnnotation); assertThat(declaredType).isInstanceOf(DeclaredType.class); assertThat(((DeclaredType) declaredType).alternativeTypeSymbols()).extracting(Symbol::fullyQualifiedName) From a10ddc50326c4f73fb1b59afae04699e3eb646be Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Thu, 6 Aug 2020 17:27:20 +0200 Subject: [PATCH 3/4] SONARPY-751 Rule S5886: Function return types should be consistent with their type hint --- .../test/resources/expected/python-S5886.json | 9 + .../org/sonar/python/checks/CheckList.java | 1 + .../checks/FunctionReturnTypeCheck.java | 131 ++++++++++ .../org/sonar/l10n/py/rules/python/S5886.html | 27 +++ .../org/sonar/l10n/py/rules/python/S5886.json | 16 ++ .../py/rules/python/Sonar_way_profile.json | 3 +- .../checks/FunctionReturnTypeCheckTest.java | 31 +++ .../resources/checks/functionReturnType.py | 228 ++++++++++++++++++ .../org/sonar/python/types/DeclaredType.java | 3 + .../sonar/python/types/DeclaredTypeTest.java | 8 + 10 files changed, 456 insertions(+), 1 deletion(-) create mode 100644 its/ruling/src/test/resources/expected/python-S5886.json create mode 100644 python-checks/src/main/java/org/sonar/python/checks/FunctionReturnTypeCheck.java create mode 100644 python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.html create mode 100644 python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.json create mode 100644 python-checks/src/test/java/org/sonar/python/checks/FunctionReturnTypeCheckTest.java create mode 100644 python-checks/src/test/resources/checks/functionReturnType.py diff --git a/its/ruling/src/test/resources/expected/python-S5886.json b/its/ruling/src/test/resources/expected/python-S5886.json new file mode 100644 index 0000000000..c51c4a2e96 --- /dev/null +++ b/its/ruling/src/test/resources/expected/python-S5886.json @@ -0,0 +1,9 @@ +{ +'project:biopython/Bio/PDB/PICIO.py':[ +178, +191, +205, +246, +250, +], +} diff --git a/python-checks/src/main/java/org/sonar/python/checks/CheckList.java b/python-checks/src/main/java/org/sonar/python/checks/CheckList.java index 21f28b75ef..0d3e975a71 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/CheckList.java +++ b/python-checks/src/main/java/org/sonar/python/checks/CheckList.java @@ -113,6 +113,7 @@ public static Iterable getChecks() { FixmeCommentCheck.class, FunctionComplexityCheck.class, FunctionNameCheck.class, + FunctionReturnTypeCheck.class, FunctionUsingLoopVariableCheck.class, GenericExceptionRaisedCheck.class, HardCodedCredentialsCheck.class, diff --git a/python-checks/src/main/java/org/sonar/python/checks/FunctionReturnTypeCheck.java b/python-checks/src/main/java/org/sonar/python/checks/FunctionReturnTypeCheck.java new file mode 100644 index 0000000000..c3a9dc71da --- /dev/null +++ b/python-checks/src/main/java/org/sonar/python/checks/FunctionReturnTypeCheck.java @@ -0,0 +1,131 @@ +/* + * SonarQube Python Plugin + * Copyright (C) 2011-2020 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.python.checks; + +import java.util.ArrayList; +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.plugins.python.api.PythonSubscriptionCheck; +import org.sonar.plugins.python.api.SubscriptionContext; +import org.sonar.plugins.python.api.symbols.Symbol; +import org.sonar.plugins.python.api.tree.BaseTreeVisitor; +import org.sonar.plugins.python.api.tree.Expression; +import org.sonar.plugins.python.api.tree.FunctionDef; +import org.sonar.plugins.python.api.tree.ReturnStatement; +import org.sonar.plugins.python.api.tree.Tree; +import org.sonar.plugins.python.api.tree.TypeAnnotation; +import org.sonar.plugins.python.api.tree.YieldStatement; +import org.sonar.plugins.python.api.types.InferredType; +import org.sonar.python.semantic.FunctionSymbolImpl; +import org.sonar.python.types.InferredTypes; + +@Rule(key = "S5886") +public class FunctionReturnTypeCheck extends PythonSubscriptionCheck { + + private static final String MESSAGE = "Return a value of type \"%s\" instead of \"%s\" or update function \"%s\" type hint."; + + @Override + public void initialize(Context context) { + context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, ctx -> { + FunctionDef functionDef = (FunctionDef) ctx.syntaxNode(); + Symbol symbol = functionDef.name().symbol(); + if (symbol == null || !symbol.is(Symbol.Kind.FUNCTION)) { + return; + } + FunctionSymbolImpl functionSymbol = (FunctionSymbolImpl) symbol; + InferredType declaredReturnType = functionSymbol.declaredReturnType(); + if (declaredReturnType == InferredTypes.anyType()) { + return; + } + ReturnTypeVisitor returnTypeVisitor = new ReturnTypeVisitor(declaredReturnType); + functionDef.accept(returnTypeVisitor); + raiseIssues(ctx, functionDef, declaredReturnType, returnTypeVisitor); + }); + } + + private static void raiseIssues(SubscriptionContext ctx, FunctionDef functionDef, InferredType declaredReturnType, ReturnTypeVisitor returnTypeVisitor) { + String functionName = functionDef.name().name(); + String returnTypeName = InferredTypes.typeName(declaredReturnType); + if (!returnTypeVisitor.yieldStatements.isEmpty()) { + if (declaredReturnType.mustBeOrExtend("typing.Generator")) { + return; + } + returnTypeVisitor.yieldStatements + .forEach(y -> ctx.addIssue(y, String.format("Remove this yield statement or annotate function \"%s\" with \"typing.Generator\".", functionName))); + } + returnTypeVisitor.invalidReturnStatements.forEach(i -> { + PreciseIssue issue; + if (i.expressions().size() > 1) { + issue = ctx.addIssue(i, String.format(MESSAGE, returnTypeName, "tuple", functionName)); + } else if (i.expressions().size() == 1 && InferredTypes.typeName(i.expressions().get(0).type()) != null) { + issue = ctx.addIssue(i.expressions().get(0), String.format(MESSAGE, returnTypeName, InferredTypes.typeName(i.expressions().get(0).type()), functionName)); + } else { + issue = ctx.addIssue(i, String.format("Return a value of type \"%s\" or update function \"%s\" type hint.", returnTypeName, functionName)); + } + addSecondaries(issue, functionDef); + }); + } + + private static void addSecondaries(PreciseIssue issue, FunctionDef functionDef) { + issue.secondary(functionDef.name(), "Function definition."); + TypeAnnotation returnTypeAnnotation = functionDef.returnTypeAnnotation(); + if (returnTypeAnnotation != null) { + issue.secondary(returnTypeAnnotation.expression(), "Type hint."); + } + } + + private static class ReturnTypeVisitor extends BaseTreeVisitor { + + InferredType returnType; + List yieldStatements = new ArrayList<>(); + List invalidReturnStatements = new ArrayList<>(); + + ReturnTypeVisitor(InferredType returnType) { + this.returnType = returnType; + } + + @Override + public void visitReturnStatement(ReturnStatement returnStatement) { + List expressions = returnStatement.expressions(); + if (expressions.isEmpty()) { + if (!returnType.mustBeOrExtend("NoneType")) { + invalidReturnStatements.add(returnStatement); + } + } else if (expressions.size() > 1) { + if (!returnType.canBeOrExtend("tuple")) { + invalidReturnStatements.add(returnStatement); + } + } else { + Expression expression = expressions.get(0); + InferredType inferredType = expression.type(); + if (!inferredType.isCompatibleWith(returnType)) { + invalidReturnStatements.add(returnStatement); + } + } + super.visitReturnStatement(returnStatement); + } + + @Override + public void visitYieldStatement(YieldStatement yieldStatement) { + yieldStatements.add(yieldStatement); + super.visitYieldStatement(yieldStatement); + } + } +} diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.html b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.html new file mode 100644 index 0000000000..bc2d3a2bd6 --- /dev/null +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.html @@ -0,0 +1,27 @@ +

Developers can use type hints to specify which type a function is expected to return. These annotations are not enforced at runtime and returning a +different type might not fail. It is however likely to be unintended and will lead to maintainability issues, if not bugs.

+

This rule raises an issue when a function or method returns a value that contradicts its type hint.

+

Noncompliant Code Example

+
+def hello() -> str:
+    return 42  # Noncompliant. Function's type hint asks for a string return value
+
+def should_return_a_string(condition) -> str:
+    if condition:
+        return "a string"
+    # Noncompliant. The function returns None if the condition is not met
+
+

Compliant Solution

+
+def hello() -> str:
+    return "Hello"
+
+def should_return_a_string() -> Optional[str]:
+    if condition:
+        return "a string"
+
+

See

+ + diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.json b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.json new file mode 100644 index 0000000000..6d4345573c --- /dev/null +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S5886.json @@ -0,0 +1,16 @@ +{ + "title": "Function return types should be consistent with their type hint", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-5886", + "sqKey": "S5886", + "scope": "All" +} diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json index a4bfd55c58..94b32c3ab6 100644 --- a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json @@ -127,6 +127,7 @@ "S5799", "S5806", "S5807", - "S5828" + "S5828", + "S5886" ] } diff --git a/python-checks/src/test/java/org/sonar/python/checks/FunctionReturnTypeCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/FunctionReturnTypeCheckTest.java new file mode 100644 index 0000000000..253e83e841 --- /dev/null +++ b/python-checks/src/test/java/org/sonar/python/checks/FunctionReturnTypeCheckTest.java @@ -0,0 +1,31 @@ +/* + * SonarQube Python Plugin + * Copyright (C) 2011-2020 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.python.checks; + +import org.junit.Test; +import org.sonar.python.checks.utils.PythonCheckVerifier; + +public class FunctionReturnTypeCheckTest { + + @Test + public void test() { + PythonCheckVerifier.verify("src/test/resources/checks/functionReturnType.py", new FunctionReturnTypeCheck()); + } +} diff --git a/python-checks/src/test/resources/checks/functionReturnType.py b/python-checks/src/test/resources/checks/functionReturnType.py new file mode 100644 index 0000000000..352f705cd4 --- /dev/null +++ b/python-checks/src/test/resources/checks/functionReturnType.py @@ -0,0 +1,228 @@ +from typing import List, SupportsFloat, Set, Dict, NoReturn, Text, Generator, Tuple, Union, AnyStr + +def builtins(): + def my_str_nok() -> str: + return 42 # Noncompliant + + def my_str_ok() -> str: + return "hello" + + def my_int_nok() -> int: + # ^^^^^^^^^^> ^^^> + return "42" # Noncompliant {{Return a value of type "int" instead of "str" or update function "my_int_nok" type hint.}} + # ^^^^ + + def my_int_ok() -> int: + return 42 + + def my_none_nok() -> None: + return 42 # Noncompliant + + def my_none_ok() -> None: + print("hello") + + def my_none_ok_2(param) -> None: + if param: + return + print("hello") + +def tuples(): + def my_tuple_nok() -> int: + return 1, 2, 3 # Noncompliant + + def my_tuple_ok() -> tuple: + return 1, 2, 3 + + def my_tuple_ok_2(param) -> tuple: + if param: + x = 1, 2, 3 + return x + else: + import collections + Person = collections.namedtuple('Person', ['name', 'age', 'gender']) + return Person(name="Bob", age=30, gender="male") # OK + + def my_tuple_ok_3(param) -> Union[tuple, bool]: + if param: + return 1, 2, 3 + else: + return False + + def my_tuple_ok_4(param) -> Union[Tuple["a", "b"], bool]: + if param: + return 1, 2 + else: + return False + +def collections(): + def my_list_nok() -> List: + return 42 # Noncompliant + + def my_list_nok_2() -> List[int]: + return 42 # Noncompliant + + def my_list_ok() -> List: + return [42] + + def my_set_nok() -> Set: + # {} is Empty dict literal + return {} # Noncompliant + + def my_set_nok_2() -> Set[str]: + # {} is Empty dict literal + return {} # Noncompliant + + def my_set_ok() -> Set: + return {42} # OK + + def my_dict() -> Dict: + return {} # OK + + +def type_aliases(): + """We should avoid FPs for type aliases used as type hint""" + def my_supports_float() -> SupportsFloat: + return 42 # OK + + def my_supports_float() -> SupportsFloat: + return "42" # FN + + def returns_text() -> Text: + return "Hello" # OK + +def other_returns(): + def my_int(param) -> int: + if param: + return 42 + else: + return "42" # Noncompliant + + def my_int_returns_none() -> int: + return # Noncompliant + + def my_int_union_type_ok(cond) -> int: + if cond: + x = 42 + else: + x = "hello" + return x # OK + + def my_list_union_nok(cond) -> List[str]: + if cond: + value = 42 + else: + value = "hello" + return value # Noncompliant {{Return a value of type "list[str]" or update function "my_list_union_nok" type hint.}} + +def functions_with_try_except(): + def my_int_try_except(cond) -> int: + class A: ... + try: + x = "hello" + x = A() + return x # Noncompliant {{Return a value of type "int" or update function "my_int_try_except" type hint.}} + except: ... + + def my_list_union_nok(cond) -> List[str]: + if cond: + value = 42 + else: + value = "hello" + return value # FN (SONARPY-775) + +def custom_classes(): + class A: ... + + class B(A): + def foo(): ... + + def my_func() -> A: + return B() # OK + + def my_func2() -> B: + return A() # Noncompliant + + +def generators(): + def my_generator() -> Generator: + for i in range(10): + yield i + return "nothing left" # OK, still a generator + + def my_generator_2() -> Generator[str]: + for i in range(10): + yield i # Out of scope FN + return "nothing left" # OK, still a generator + + def not_a_generator(param) -> int: + if param: + return "hello" # Noncompliant {{Return a value of type "int" instead of "str" or update function "not_a_generator" type hint.}} + else: + yield 42 # Noncompliant {{Remove this yield statement or annotate function "not_a_generator" with "typing.Generator".}} + +def missing_return(): + """No issue if functions do not return as it might be due to custom error handling or a stub definition""" + def raise_custom_error(message): + raise ValueError(message) + + def get_int_might_raise_exception(x) -> int: # OK + if x: + return 42 + raise_custom_error("invalid input") + + def my_int_2(param) -> int: # FN + if param: + return 42 + else: + print("hello") + + def my_int_4() -> int: # FN + try: + return 42 + except IndexError as e: + print("hello") + + def not_implemented() -> int: + return NotImplemented + + def my_stub() -> int: + ... + +def out_of_scope(): + def my_noreturn() -> NoReturn: + """NoReturn functions should never return normally. To be checked with actual usage on Peach to avoid noise""" + return None # FN + + def my_list_oos() -> List[str]: + return [1, 2, 3] # FN + + def my_list_oos2() -> List[str]: + return [1, "my_str", 2] # OK + + def my_list_oos3() -> List[str]: + x = 1 + y = 2 + return [x, y] # FN + + def my_list_oos4() -> List[str]: + x = 1 + y = 2 + my_list = [x, y] + print("hello") + my_list.append(3) + return my_list # FN + + def my_set_str() -> Set[str]: + return {42, 43} # FN + + def my_anystr(param) -> AnyStr: + if param == 1: + return "hello" + elif param == 2: + return u"foo" + elif param == 3: + return b"bar" + elif param == 4: + return bytes("hello") + elif param == 5: + return 42 # FN diff --git a/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java b/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java index 6c3c658fd4..8ae178a341 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java +++ b/python-frontend/src/main/java/org/sonar/python/types/DeclaredType.java @@ -82,6 +82,9 @@ public Optional resolveMember(String memberName) { @Override public boolean canBeOrExtend(String typeName) { + if (typeName.equals("typing.Tuple") || typeName.equals("tuple")) { + return alternativeTypeSymbols().stream().map(Symbol::fullyQualifiedName).anyMatch(fqn -> "typing.Tuple".equals(fqn) || "tuple".equals(fqn)); + } return true; } diff --git a/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java index 8e5416a101..e32cf4216e 100644 --- a/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java +++ b/python-frontend/src/test/java/org/sonar/python/types/DeclaredTypeTest.java @@ -83,8 +83,16 @@ public void test_canOnlyBe() { @Test public void test_canBeOrExtend() { ClassSymbolImpl x = new ClassSymbolImpl("x", "x"); + ClassSymbolImpl tuple = new ClassSymbolImpl("tuple", "tuple"); assertThat(new DeclaredType(x).canBeOrExtend("x")).isTrue(); assertThat(new DeclaredType(x).canBeOrExtend("y")).isTrue(); + assertThat(new DeclaredType(x).canBeOrExtend("tuple")).isFalse(); + assertThat(new DeclaredType(x).canBeOrExtend("typing.Tuple")).isFalse(); + assertThat(new DeclaredType(tuple).canBeOrExtend("typing.Tuple")).isTrue(); + assertThat(new DeclaredType(tuple).canBeOrExtend("tuple")).isTrue(); + ClassSymbolImpl typingTuple = new ClassSymbolImpl("Tuple", "typing.Tuple"); + assertThat(new DeclaredType(typingTuple).canBeOrExtend("typing.Tuple")).isTrue(); + assertThat(new DeclaredType(typingTuple).canBeOrExtend("tuple")).isTrue(); } @Test From 92eb475e5d0f09094bb65f22ff389e6944752bde Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Mon, 10 Aug 2020 14:58:36 +0200 Subject: [PATCH 4/4] Fixes after review --- .../org/sonar/plugins/python/api/types/InferredType.java | 6 ++++++ .../main/java/org/sonar/python/types/InferredTypes.java | 9 +++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java b/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java index cc9f5c1aa8..e9b63c4c60 100644 --- a/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java +++ b/python-frontend/src/main/java/org/sonar/plugins/python/api/types/InferredType.java @@ -44,6 +44,12 @@ public interface InferredType { @Beta boolean isCompatibleWith(InferredType other); + /** + * mustBeOrExtend implies we know for sure the given type is either of the given typeName or a subtype of it. + * As opposed to "canBeOrExtend", this will return true only when we are sure the subtyping relationship is present. + * For types inferred from type annotations (DeclaredType), the actual underlying type might be different from what has been declared, + * but it must be or extend the declared type. + */ @Beta boolean mustBeOrExtend(String typeName); diff --git a/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java b/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java index 31ba3634c7..c34ffebfab 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java +++ b/python-frontend/src/main/java/org/sonar/python/types/InferredTypes.java @@ -49,6 +49,7 @@ public class InferredTypes { private static final Map ALIASED_ANNOTATIONS = new HashMap<>(); + static { ALIASED_ANNOTATIONS.put("typing.List", BuiltinTypes.LIST); ALIASED_ANNOTATIONS.put("typing.Tuple", BuiltinTypes.TUPLE); @@ -79,7 +80,7 @@ private InferredTypes() { } public static boolean isInitialized() { - return builtinSymbols != null; + return builtinSymbols != null; } public static InferredType anyType() { @@ -140,8 +141,8 @@ private static DeclaredType declaredTypeFromTypeAnnotation(Expression expression return TreeUtils.getSymbolFromTree(subscription.object()) .map(symbol -> { List args = subscription.subscripts().expressions().stream() - .map(exp -> declaredTypeFromTypeAnnotation(exp, builtinSymbols)) - .collect(Collectors.toList()); + .map(exp -> declaredTypeFromTypeAnnotation(exp, builtinSymbols)) + .collect(Collectors.toList()); if (args.stream().anyMatch(Objects::isNull)) { args = Collections.emptyList(); } @@ -159,7 +160,7 @@ private static DeclaredType declaredTypeFromTypeAnnotation(Expression expression private static InferredType runtimeTypefromTypeAnnotation(Expression expression, Map builtinSymbols) { if (expression.is(Kind.NAME) && !((Name) expression).name().equals("Any")) { Symbol symbol = ((Name) expression).symbol(); - if (symbol != null ) { + if (symbol != null) { if ("typing.Text".equals(symbol.fullyQualifiedName())) { return InferredTypes.runtimeType(builtinSymbols.get("str")); }