From b5e38dc9c72ffce74421c6a7c7f1e4df677a8af4 Mon Sep 17 00:00:00 2001 From: M Bussonnier Date: Mon, 11 Nov 2024 11:49:26 +0100 Subject: [PATCH] Try to detect non-gloabl unused imports. We do so by tracking the imports on exist of each scopestack; and on exist, if there is an import that has not been checked; we mark it as unused. This seem to have side effects as the SourceToSource transformer is aware it cannot remove non-global imports and emit a error message. Note that this also affects conditional imports: ``` if sys.version_info > X,y: from foo import bar else: from foo.new import bar ``` The first import will be marked as unused as shoadowed by the second one (pyflyby does not understand the if/else). This also does some refactor and type anotations while I am at it. --- lib/python/pyflyby/_autoimp.py | 79 ++++++++++++++++++++++---------- lib/python/pyflyby/_importdb.py | 4 +- lib/python/pyflyby/_imports2s.py | 19 ++++---- 3 files changed, 68 insertions(+), 34 deletions(-) diff --git a/lib/python/pyflyby/_autoimp.py b/lib/python/pyflyby/_autoimp.py index c1b4c8fd..b68094b9 100644 --- a/lib/python/pyflyby/_autoimp.py +++ b/lib/python/pyflyby/_autoimp.py @@ -4,6 +4,8 @@ +from __future__ import print_function, annotations + import ast import builtins from collections.abc import Sequence @@ -18,13 +20,13 @@ from pyflyby._importstmt import Import from pyflyby._log import logger from pyflyby._modules import ModuleHandle -from pyflyby._parse import (PythonBlock, _is_ast_str, - infer_compile_mode, MatchAs) +from pyflyby._parse import (MatchAs, PythonBlock, _is_ast_str, + infer_compile_mode) from six import reraise import sys import types -from typing import Any, Set, Optional, List, Tuple +from typing import Any, List, Optional, Set, Tuple, Union, Dict if sys.version_info >= (3, 12): ATTRIBUTE_NAME = "value" @@ -130,9 +132,13 @@ def __getitem__(self, item): def __len__(self): return len(self._tup) - def with_new_scope( - self, include_class_scopes=False, new_class_scope=False, unhide_classdef=False - ): + def _with_new_scope( + self, + *, + include_class_scopes: bool, + new_class_scope: bool, + unhide_classdef: bool, + ) -> ScopeStack: """ Return a new ``ScopeStack`` with an additional empty scope. @@ -148,8 +154,8 @@ def with_new_scope( if include_class_scopes: scopes = tuple(self) else: - scopes = tuple(s for s in self - if not isinstance(s, _ClassScope)) + scopes = tuple(s for s in self if not isinstance(s, _ClassScope)) + new_scope: Union[_ClassScope, Dict[str, Any]] if new_class_scope: new_scope = _ClassScope() else: @@ -199,7 +205,7 @@ def merged_to_two(self): return (d, self[-1]) - def has_star_import(self): + def has_star_import(self) -> bool: """ Return whether there are any star-imports in this ScopeStack. Only relevant in AST-based static analysis mode. @@ -355,9 +361,10 @@ def __init__(self, name: str, source: str, lineno: int): self.name = name self.source = source # generally an Import self.lineno = lineno + logger.debug("Create _UseChecker : %r", self) def __repr__(self): - return f"<{type(self).__name__}: name:{self.name} source:{self.source!r} lineno:{self.lineno} used:{self.used}>" + return f"<{type(self).__name__}: name:{self.name!r} source:{self.source!r} lineno:{self.lineno} used:{self.used}>" class _MissingImportFinder: @@ -397,13 +404,13 @@ def __init__(self, scopestack, *, find_unused_imports:bool, parse_docstrings:boo # Create a stack of namespaces. The caller should pass in a list that # includes the globals dictionary. ScopeStack() will make sure this # includes builtins. - scopestack = ScopeStack(scopestack) + _scopestack = ScopeStack(scopestack) # Add an empty namespace to the stack. This facilitates adding stuff # to scopestack[-1] without ever modifying user globals. - scopestack = scopestack.with_new_scope() - - self.scopestack = scopestack + self.scopestack = _scopestack._with_new_scope( + include_class_scopes=False, new_class_scope=False, unhide_classdef=False + ) # Create data structure to hold the result. # missing_imports is a list of (lineno, DottedIdentifier) tuples. @@ -450,6 +457,7 @@ def scan_for_import_issues(self, codeblock: PythonBlock): # references in doctests to be noted as missing-imports. For now we # just let the code accumulate into self.missing_imports and ignore # the result. + logger.debug("unused: %r", self.unused_imports) missing_imports = sorted(self.missing_imports) if self.parse_docstrings and self.unused_imports is not None: doctest_blocks = codeblock.get_doctests() @@ -467,10 +475,8 @@ def scan_for_import_issues(self, codeblock: PythonBlock): # Currently we don't support the 'global' keyword anyway so # this doesn't matter yet, and it's uncommon to use 'global' # in a doctest, so this is low priority to fix. - oldstack = self.scopestack - self.scopestack = self.scopestack.with_new_scope() - self._scan_node(block.ast_node) - self.scopestack = oldstack + with self._NewScopeCtx(check_unused_imports=False): + self._scan_node(block.ast_node) # Find literal brace identifiers like "... `Foo` ...". # TODO: Do this inline: (1) faster; (2) can use proper scope of vars # Once we do that, use _check_load() with new args @@ -553,17 +559,36 @@ def generic_visit(self, node): @contextlib.contextmanager - def _NewScopeCtx(self, **kwargs): + def _NewScopeCtx( + self, + include_class_scopes=False, + new_class_scope=False, + unhide_classdef=False, + check_unused_imports=True, + ): """ Context manager that temporarily pushes a new empty namespace onto the stack of namespaces. """ prev_scopestack = self.scopestack - new_scopestack = prev_scopestack.with_new_scope(**kwargs) + new_scopestack = prev_scopestack._with_new_scope( + include_class_scopes=include_class_scopes, + new_class_scope=new_class_scope, + unhide_classdef=unhide_classdef, + ) self.scopestack = new_scopestack try: yield finally: + logger.debug("throwing last scope from scopestack: %r", new_scopestack[-1]) + for name, use_checker in new_scopestack[-1].items(): + if use_checker and use_checker.used == False and check_unused_imports: + logger.debug( + "unused checker %r scopestack_depth %r", + use_checker, + len(self.scopestack), + ) + self.unused_imports.append((use_checker.lineno, use_checker.source)) assert self.scopestack is new_scopestack self.scopestack = prev_scopestack @@ -693,7 +718,7 @@ def visit_Lambda(self, node): self.visit(node.body) self._in_FunctionDef = old_in_FunctionDef - def _visit_typecomment(self, typecomment): + def _visit_typecomment(self, typecomment: str) -> None: """ Warning, when a type comment the node is a string, not an ast node. We also get two types of type comments: @@ -716,6 +741,7 @@ def foo(a #type: int """ if typecomment is None: return + node: Union[ast.Module, ast.FunctionType] if '->' in typecomment: node = ast.parse(typecomment, mode='func_type') else: @@ -723,7 +749,7 @@ def foo(a #type: int self.visit(node) - def visit_arguments(self, node): + def visit_arguments(self, node) -> None: assert node._fields == ('posonlyargs', 'args', 'vararg', 'kwonlyargs', 'kw_defaults', 'kwarg', 'defaults'), node._fields # Argument/parameter list. Note that the defaults should be # considered "Load"s from the upper scope, and the argument names are @@ -754,7 +780,7 @@ def visit_arguments(self, node): else: self._visit_Store(node.kwarg) - def visit_ExceptHandler(self, node): + def visit_ExceptHandler(self, node) -> None: assert node._fields == ('type', 'name', 'body') if node.type: self.visit(node.type) @@ -948,7 +974,12 @@ def _visit_StoreImport(self, node, modulename): value = _UseChecker(name, imp, self._lineno) self._visit_Store(name, value) - def _visit_Store(self, fullname:str, value=None): + def _visit_Store(self, fullname: str, value: Optional[_UseChecker] = None): + """ + Visit a Store action, check for unused import + and add current value to the last scope. + """ + assert isinstance(value, (_UseChecker, type(None))) logger.debug("_visit_Store(%r)", fullname) if fullname is None: return diff --git a/lib/python/pyflyby/_importdb.py b/lib/python/pyflyby/_importdb.py index c0d3f07a..fd69bc9c 100644 --- a/lib/python/pyflyby/_importdb.py +++ b/lib/python/pyflyby/_importdb.py @@ -2,6 +2,8 @@ # Copyright (C) 2011, 2012, 2013, 2014, 2015 Karl Chen. # License: MIT http://opensource.org/licenses/MIT +from __future__ import annotations + from collections import defaultdict @@ -417,7 +419,7 @@ def get_default(cls, target_filename: Union[Filename, str], /): return result @classmethod - def interpret_arg(cls, arg, target_filename): + def interpret_arg(cls, arg, target_filename) -> ImportDB: if arg is None: return cls.get_default(target_filename) else: diff --git a/lib/python/pyflyby/_imports2s.py b/lib/python/pyflyby/_imports2s.py index b21ac3df..bf83d4a7 100644 --- a/lib/python/pyflyby/_imports2s.py +++ b/lib/python/pyflyby/_imports2s.py @@ -13,12 +13,12 @@ from pyflyby._util import ImportPathCtx, Inf, NullCtx, memoize import re -from typing import Union +from typing import Union, Optional, Literal from textwrap import indent -class SourceToSourceTransformationBase(object): +class SourceToSourceTransformationBase: input: PythonBlock @@ -121,7 +121,7 @@ def pretty_print(self, params=None): result = [block.pretty_print(params=params) for block in self.blocks] return FileText.concatenate(result) - def find_import_block_by_lineno(self, lineno): + def find_import_block_by_lineno(self, lineno: int): """ Find the import block containing the given line number. @@ -312,11 +312,11 @@ def ImportPathForRelativeImportsCtx(codeblock): def fix_unused_and_missing_imports( - codeblock: Union[PythonBlock, str], - add_missing=True, - remove_unused="AUTOMATIC", - add_mandatory=True, - db=None, + codeblock: Union[PythonBlock, str, Filename], + add_missing: bool = True, + remove_unused: Union[Literal["AUTOMATIC"], bool] = "AUTOMATIC", + add_mandatory: bool = True, + db: Optional[ImportDB] = None, params=None, ) -> PythonBlock: r""" @@ -345,6 +345,7 @@ def fix_unused_and_missing_imports( :rtype: `PythonBlock` """ + _codeblock: PythonBlock if isinstance(codeblock, Filename): _codeblock = PythonBlock(codeblock) if not isinstance(codeblock, PythonBlock): @@ -393,7 +394,7 @@ def fix_unused_and_missing_imports( logger.error( "%s: couldn't remove import %r", filename, imp,) except LineNumberNotFoundError as e: - logger.error( + logger.debug( "%s: unused import %r on line %d not global", filename, str(imp), e.args[0]) else: