From 301ebcb12b3042d829b3ecafa3b1a0aa6ec353dc Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 4 Mar 2025 12:55:44 +0000 Subject: [PATCH 1/4] Python: Extend test cases for "unused global var" query Adds two test cases having to do with type annotations. The first one demonstrates that type annotations (even if they are never executed by the Python interpreter) count as uses for the purposes of the unused variable query. The second one demonstrates that this is _not_ the case if all such uses are inside strings (i.e. forward declarations), as we do not currently inspect the content of these strings. --- .../unused/UnusedModuleVariable.expected | 3 +++ .../Variables/unused/variables_test.py | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected b/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected index 248ce2d0637f..6eb83f9cada3 100644 --- a/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected +++ b/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected @@ -4,3 +4,6 @@ | variables_test.py:86:3:86:3 | b | The global variable 'b' is not used. | | variables_test.py:86:5:86:5 | c | The global variable 'c' is not used. | | variables_test.py:100:1:100:8 | glob_var | The global variable 'glob_var' is not used. | +| variables_test.py:147:5:147:26 | ForwardParamAnnotation | The global variable 'ForwardParamAnnotation' is not used. | +| variables_test.py:148:5:148:27 | ForwardReturnAnnotation | The global variable 'ForwardReturnAnnotation' is not used. | +| variables_test.py:149:5:149:31 | ForwardAssignmentAnnotation | The global variable 'ForwardAssignmentAnnotation' is not used. | diff --git a/python/ql/test/query-tests/Variables/unused/variables_test.py b/python/ql/test/query-tests/Variables/unused/variables_test.py index ff8ac50a541c..611b9fbd6b2a 100644 --- a/python/ql/test/query-tests/Variables/unused/variables_test.py +++ b/python/ql/test/query-tests/Variables/unused/variables_test.py @@ -137,3 +137,21 @@ def test_dict_unpacking(queryset, field_name, value): for tag in value.split(','): queryset = queryset.filter(**{field_name + '__name': tag}) return queryset + +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + ParamAnnotation = int + ReturnAnnotation = int + AssignmentAnnotation = int + ForwardParamAnnotation = int + ForwardReturnAnnotation = int + ForwardAssignmentAnnotation = int + +def test_direct_annotation(x: ParamAnnotation) -> ReturnAnnotation: + if x: + y : AssignmentAnnotation = 1 + +def test_forward_annotation(x: "ForwardParamAnnotation") -> "ForwardReturnAnnotation": + if x: + y : "ForwardAssignmentAnnotation" = 1 From 88615f427ba59b1a39f7f4bdfe14fe0a37f7818b Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 4 Mar 2025 14:03:33 +0000 Subject: [PATCH 2/4] Python: Add support for forward declarations in unused var query Fixes the false positive reported in https://github.com/github/codeql/issues/18910 Adds a new `Annotation` class (subclass of `Expr`) which encompasses all possible kinds of annotations in Python. Using this, we look for string literals which are part of an annotation, and which have the same content as the name of a (potentially) unused global variable, and in that case we do not produce an alert. In future, we may want to support inspecting such string literals more deeply (e.g. to support stuff like "list[unused_var]"), but I think for now this level of support is sufficient. --- python/ql/lib/semmle/python/Exprs.qll | 18 ++++++++++++++++++ .../ql/src/Variables/UnusedModuleVariable.ql | 11 ++++++++++- .../unused/UnusedModuleVariable.expected | 3 --- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/python/ql/lib/semmle/python/Exprs.qll b/python/ql/lib/semmle/python/Exprs.qll index 47e31bb07cdd..4b31cb0ba91f 100644 --- a/python/ql/lib/semmle/python/Exprs.qll +++ b/python/ql/lib/semmle/python/Exprs.qll @@ -746,6 +746,24 @@ class Guard extends Guard_ { override Expr getASubExpression() { result = this.getTest() } } +/** An annotation, such as the `int` part of `x: int` */ +class Annotation extends Expr { + Annotation() { + this = any(AnnAssign a).getAnnotation() + or + exists(Arguments args | args = any(FunctionExpr f).getArgs() | + this in [ + args.getAnAnnotation(), + args.getAKwAnnotation(), + args.getKwargannotation(), + args.getVarargannotation() + ] + ) + or + this = any(FunctionExpr f).getReturns() + } +} + /* Expression Contexts */ /** A context in which an expression used */ class ExprContext extends ExprContext_ { } diff --git a/python/ql/src/Variables/UnusedModuleVariable.ql b/python/ql/src/Variables/UnusedModuleVariable.ql index 869c31cb4fa1..c9009d9bf369 100644 --- a/python/ql/src/Variables/UnusedModuleVariable.ql +++ b/python/ql/src/Variables/UnusedModuleVariable.ql @@ -34,6 +34,14 @@ predicate complex_all(Module m) { ) } +predicate used_in_forward_declaration(Name used, Module mod) { + exists(StringLiteral s, Annotation annotation | + s.getS() = used.getId() and + s.getEnclosingModule() = mod and + annotation.getASubExpression*() = s + ) +} + predicate unused_global(Name unused, GlobalVariable v) { not exists(ImportingStmt is | is.contains(unused)) and forex(DefinitionNode defn | defn.getNode() = unused | @@ -55,7 +63,8 @@ predicate unused_global(Name unused, GlobalVariable v) { unused.defines(v) and not name_acceptable_for_unused_variable(v) and not complex_all(unused.getEnclosingModule()) - ) + ) and + not used_in_forward_declaration(unused, unused.getEnclosingModule()) } from Name unused, GlobalVariable v diff --git a/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected b/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected index 6eb83f9cada3..248ce2d0637f 100644 --- a/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected +++ b/python/ql/test/query-tests/Variables/unused/UnusedModuleVariable.expected @@ -4,6 +4,3 @@ | variables_test.py:86:3:86:3 | b | The global variable 'b' is not used. | | variables_test.py:86:5:86:5 | c | The global variable 'c' is not used. | | variables_test.py:100:1:100:8 | glob_var | The global variable 'glob_var' is not used. | -| variables_test.py:147:5:147:26 | ForwardParamAnnotation | The global variable 'ForwardParamAnnotation' is not used. | -| variables_test.py:148:5:148:27 | ForwardReturnAnnotation | The global variable 'ForwardReturnAnnotation' is not used. | -| variables_test.py:149:5:149:31 | ForwardAssignmentAnnotation | The global variable 'ForwardAssignmentAnnotation' is not used. | From 5d3b40d514ae37a130cb70a34cb1c1a9521dbb5a Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 4 Mar 2025 14:47:03 +0000 Subject: [PATCH 3/4] Python: Add change note --- ...4-fix-forward-annotation-fp-in-unused-global-var-query.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 python/ql/src/change-notes/2025-03-04-fix-forward-annotation-fp-in-unused-global-var-query.md diff --git a/python/ql/src/change-notes/2025-03-04-fix-forward-annotation-fp-in-unused-global-var-query.md b/python/ql/src/change-notes/2025-03-04-fix-forward-annotation-fp-in-unused-global-var-query.md new file mode 100644 index 000000000000..78142ea3fc68 --- /dev/null +++ b/python/ql/src/change-notes/2025-03-04-fix-forward-annotation-fp-in-unused-global-var-query.md @@ -0,0 +1,5 @@ +--- +category: fix +--- + +- The `py/unused-global-variable` now no longer flags variables that are only used in forward references (e.g. the `Foo` in `def bar(x: "Foo"): ...`). From 50a01b1244dce5d1a8847e841c1a451032f364a9 Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 4 Mar 2025 15:53:34 +0000 Subject: [PATCH 4/4] Python: Remove superfluous reference to `FunctionExpr` This way we also get annotations that appear in `Lambda`s --- python/ql/lib/semmle/python/Exprs.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/lib/semmle/python/Exprs.qll b/python/ql/lib/semmle/python/Exprs.qll index 4b31cb0ba91f..accc370481aa 100644 --- a/python/ql/lib/semmle/python/Exprs.qll +++ b/python/ql/lib/semmle/python/Exprs.qll @@ -751,7 +751,7 @@ class Annotation extends Expr { Annotation() { this = any(AnnAssign a).getAnnotation() or - exists(Arguments args | args = any(FunctionExpr f).getArgs() | + exists(Arguments args | this in [ args.getAnAnnotation(), args.getAKwAnnotation(),