diff --git a/CHANGELOG.md b/CHANGELOG.md index c5a16bd..71033ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#108](https://github.com/green-code-initiative/creedengo-python/pull/108) Add rule GCI109 Avoid using exceptions for control flow - [#109](https://github.com/green-code-initiative/creedengo-python/pull/109) Add rule GCI110 Avoid wildcard imports - [#122](https://github.com/green-code-initiative/creedengo-python/pull/122) Add rule GCI111 Logging format, prefer using %s +- [#23](https://github.com/green-code-initiative/creedengo-python/issues/23) Correction of false positive in GCI4 with TypeVar ### Changed diff --git a/src/it/java/org/greencodeinitiative/creedengo/python/integration/tests/GCIRulesIT.java b/src/it/java/org/greencodeinitiative/creedengo/python/integration/tests/GCIRulesIT.java index c1c2740..315af32 100644 --- a/src/it/java/org/greencodeinitiative/creedengo/python/integration/tests/GCIRulesIT.java +++ b/src/it/java/org/greencodeinitiative/creedengo/python/integration/tests/GCIRulesIT.java @@ -84,9 +84,22 @@ void testGCI7_nonCompliant() { } @Test - void testGCI4() { + void testGCI4_compliant() { - String filePath = "src/avoidGlobalVariableInFunction.py"; + String filePath = "src/avoidGlobalVariableInFunctionCompliant.py"; + String ruleId = "creedengo-python:GCI4"; + String ruleMsg = "Use local variable (function/class scope) instead of global variable (application scope)"; + int[] startLines = new int[]{}; + int[] endLines = new int[]{}; + + checkIssuesForFile(filePath, ruleId, ruleMsg, startLines, endLines, SEVERITY, TYPE, EFFORT_5MIN); + + } + + @Test + void testGCI4_nonCompliant() { + + String filePath = "src/avoidGlobalVariableInFunctionNonCompliant.py"; String ruleId = "creedengo-python:GCI4"; String ruleMsg = "Use local variable (function/class scope) instead of global variable (application scope)"; int[] startLines = new int[]{4, 5, 6, 7, 9, 11}; diff --git a/src/test/resources/checks/avoidGlobalVariableInFunctionNonCompliant2.py b/src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunctionCompliant.py similarity index 90% rename from src/test/resources/checks/avoidGlobalVariableInFunctionNonCompliant2.py rename to src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunctionCompliant.py index f1bfa74..0f4fe80 100644 --- a/src/test/resources/checks/avoidGlobalVariableInFunctionNonCompliant2.py +++ b/src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunctionCompliant.py @@ -6,5 +6,5 @@ U = TypeVar('U') # Declare type variable "U" def second(l: Sequence[U]) -> U: # Function is generic over the TypeVar "U" - print(U) #Noncompliant + print(U) return l[1] \ No newline at end of file diff --git a/src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunction.py b/src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunctionNonCompliant.py similarity index 100% rename from src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunction.py rename to src/it/test-projects/creedengo-python-plugin-test-project/src/avoidGlobalVariableInFunctionNonCompliant.py diff --git a/src/main/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheck.java b/src/main/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheck.java index 74f0f30..d2eb152 100644 --- a/src/main/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheck.java +++ b/src/main/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheck.java @@ -17,16 +17,16 @@ */ package org.greencodeinitiative.creedengo.python.checks; -import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; -import java.util.List; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import org.sonar.check.Rule; import org.sonar.plugins.python.api.PythonSubscriptionCheck; import org.sonar.plugins.python.api.SubscriptionCheck; import org.sonar.plugins.python.api.SubscriptionContext; -import org.sonar.plugins.python.api.symbols.Symbol; import org.sonar.plugins.python.api.tree.AnnotatedAssignment; import org.sonar.plugins.python.api.tree.ArgList; import org.sonar.plugins.python.api.tree.AssertStatement; @@ -60,6 +60,7 @@ import org.sonar.plugins.python.api.tree.ParameterList; import org.sonar.plugins.python.api.tree.ParenthesizedExpression; import org.sonar.plugins.python.api.tree.PrintStatement; +import org.sonar.plugins.python.api.tree.QualifiedExpression; import org.sonar.plugins.python.api.tree.RaiseStatement; import org.sonar.plugins.python.api.tree.RegularArgument; import org.sonar.plugins.python.api.tree.ReprExpression; @@ -85,24 +86,95 @@ public class AvoidGlobalVariableInFunctionCheck extends PythonSubscriptionCheck public static final String DESCRIPTION = "Use local variable (function/class scope) instead of global variable (application scope)"; - private List globalVariables; - private List definedLocalVariables; + private static final Set TYPING_CONSTRUCTS = new HashSet<>(Arrays.asList("TypeVar", "TypeVarTuple", "ParamSpec", "NewType")); + + private Set globalVariables; + private Set definedLocalVariables; private Map usedLocalVariables; @Override public void initialize(SubscriptionCheck.Context context) { - globalVariables = new ArrayList<>(); + globalVariables = new HashSet<>(); context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::visitFileInput); context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, this::visitFuncDef); } public void visitFileInput(SubscriptionContext ctx) { FileInput fileInput = (FileInput) ctx.syntaxNode(); - fileInput.globalVariables().stream().filter(v -> v.is(Symbol.Kind.OTHER)).forEach(v -> this.globalVariables.add(v.name())); + + // Add all module-level assignments except TypeVar and similar typing constructs + StatementList statements = fileInput.statements(); + if (statements != null) { + statements.statements().forEach(this::extractGlobalVariablesFromStatement); + } + } + + private void extractGlobalVariablesFromStatement(Tree statement) { + if (statement == null) { + return; + } + + switch (statement.getKind()) { + case ASSIGNMENT_STMT: + AssignmentStatement assignmentStatement = (AssignmentStatement) statement; + if (isTypingConstruct(assignmentStatement.assignedValue())) { + return; // Skip TypeVar and similar typing constructs + } + assignmentStatement.lhsExpressions().forEach(this::extractNamesFromExpression); + break; + case ANNOTATED_ASSIGNMENT: + AnnotatedAssignment annotatedAssignment = (AnnotatedAssignment) statement; + if (annotatedAssignment.assignedValue() != null && isTypingConstruct(annotatedAssignment.assignedValue())) { + return; + } + if (annotatedAssignment.variable().is(Tree.Kind.NAME)) { + this.globalVariables.add(((Name) annotatedAssignment.variable()).name()); + } + break; + default: + break; + } + } + + private void extractNamesFromExpression(Tree expression) { + if (expression.is(Tree.Kind.EXPRESSION_LIST)) { + ((ExpressionList) expression).expressions().forEach(expr -> { + if (expr.is(Tree.Kind.NAME)) { + this.globalVariables.add(((Name) expr).name()); + } + }); + } else if (expression.is(Tree.Kind.NAME)) { + this.globalVariables.add(((Name) expression).name()); + } + } + + private boolean isTypingConstruct(Tree tree) { + if (tree == null || !tree.is(Tree.Kind.CALL_EXPR)) { + return false; + } + CallExpression callExpr = (CallExpression) tree; + Tree callee = callExpr.callee(); + + // Handle direct call: TypeVar(...) + if (callee.is(Tree.Kind.NAME)) { + String calleeName = ((Name) callee).name(); + return TYPING_CONSTRUCTS.contains(calleeName); + } + + // Handle qualified call: typing.TypeVar(...) + if (callee.is(Tree.Kind.QUALIFIED_EXPR)) { + QualifiedExpression qualifiedExpr = (QualifiedExpression) callee; + if (qualifiedExpr.name().is(Tree.Kind.NAME)) { + String methodName = ((Name) qualifiedExpr.name()).name(); + return TYPING_CONSTRUCTS.contains(methodName); + } + } + + return false; } void visitFuncDef(SubscriptionContext ctx) { - this.definedLocalVariables = new ArrayList<>(); + this.definedLocalVariables = new HashSet<>(); this.usedLocalVariables = new HashMap<>(); FunctionDef functionDef = (FunctionDef) ctx.syntaxNode(); diff --git a/src/test/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheckTest.java b/src/test/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheckTest.java index f886bf4..57fd882 100644 --- a/src/test/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheckTest.java +++ b/src/test/java/org/greencodeinitiative/creedengo/python/checks/AvoidGlobalVariableInFunctionCheckTest.java @@ -20,10 +20,11 @@ import org.junit.jupiter.api.Test; import org.sonar.python.checks.utils.PythonCheckVerifier; -public class AvoidGlobalVariableInFunctionCheckTest { +class AvoidGlobalVariableInFunctionCheckTest { + @Test - public void test() { + void test() { PythonCheckVerifier.verify("src/test/resources/checks/avoidGlobalVariableInFunctionNonCompliant.py", new AvoidGlobalVariableInFunctionCheck()); - PythonCheckVerifier.verify("src/test/resources/checks/avoidGlobalVariableInFunctionNonCompliant2.py", new AvoidGlobalVariableInFunctionCheck()); + PythonCheckVerifier.verifyNoIssue("src/test/resources/checks/avoidGlobalVariableInFunctionCompliant.py", new AvoidGlobalVariableInFunctionCheck()); } } diff --git a/src/test/resources/checks/avoidGlobalVariableInFunctionCompliant.py b/src/test/resources/checks/avoidGlobalVariableInFunctionCompliant.py new file mode 100644 index 0000000..0f4fe80 --- /dev/null +++ b/src/test/resources/checks/avoidGlobalVariableInFunctionCompliant.py @@ -0,0 +1,10 @@ +# example from official Python documentation + +from collections.abc import Sequence +from typing import TypeVar + +U = TypeVar('U') # Declare type variable "U" + +def second(l: Sequence[U]) -> U: # Function is generic over the TypeVar "U" + print(U) + return l[1] \ No newline at end of file