Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move typechecking imports under TYPE_CHECKING flag #9970

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Move typechecking imports under TYPE_CHECKING flag #9970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b202aa
Move typechecking imports under TYPE_CHECKING flag
nickdrozd Sep 24, 2024
0977531
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 25, 2024
8d455ab
Revert one change
nickdrozd Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pylint/checkers/bad_chained_comparison.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@

from typing import TYPE_CHECKING

from astroid import nodes

from pylint.checkers import BaseChecker
from pylint.interfaces import HIGH

if TYPE_CHECKING:
from astroid import nodes

Comment on lines -9 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it does impose a greater workload on developers and us maintainer. If we change something, we constantly need to make sure to double check all imports or run the risk of something failing.

I don't understand what you mean. There is the minor one-time cost of reviewing this PR (and the other one). What other workload is there?

This is the just the first change here.

I assume at the moment nodes is only used for type annotations so it's "safe" in the if TYPE_CHECKING block. What happens if you go ahead and modify / add a new check that uses isinstance(nodes, nodes.ClassDef) for example? You would run the tests only to notice that they fail since nodes isn't defined at runtime. So you have to go back, adjust the import, and run it again.

On its own maybe not that much of work but now you move that check to another file. So the imports need to change again. Keeping that in sync will be quite frustrating, even if it can be automated with dev tools.

More importantly though, it isn't really worth it IMO.

In the example, nodes is probably already cached as it's used at runtime in a different checker. Guarding an import behind if TYPE_CHECKING for that purpose only really makes sense if you can avoid having to import a package in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scenario you're proposing, you're taking a module which does not have any runtime dependency on astroid.nodes and adding a new runtime dependency on it. A new dependency where before there was none. Is a new module dependency a good idea? Can the feature be implemented without adding a new module dependency? These are questions that are worth taking a few seconds to think about. If the code is structured in such a way as to highlight when a new runtime module dependency is added, well that seems like a good thing to me. Currently the true cost of adding a new dependency is hidden because typechecking imports and runtime imports are not distinguished.

Pylint remains bedeviled by two severe problems: it is very slow and it is not type-correct. Fixing the type errors would help the speed problem, since it would allow compiling with Mypyc. But the typing problem is difficult due to an internal problem, namely a gnarly module structure based on circular imports.

In light of the these well-known and long-standing problems, your idea is that the code should be optimized to make it as easy as possible to add new runtime module dependencies? That is a very strange thing to prioritize. Personally I think adding a little friction to the process might be a good thing, if it can stanch the complexity.

from pylint.lint import PyLinter

COMPARISON_OP = frozenset(("<", "<=", ">", ">=", "!=", "=="))
Expand Down
8 changes: 5 additions & 3 deletions pylint/checkers/base/basic_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,22 @@

import collections
import itertools
from collections.abc import Iterator
from typing import TYPE_CHECKING, Literal, cast

import astroid
from astroid import nodes, objects, util

from pylint import utils as lint_utils
from pylint.checkers import BaseChecker, utils
from pylint.interfaces import HIGH, INFERENCE, Confidence
from pylint.interfaces import HIGH, INFERENCE
from pylint.reporters.ureports import nodes as reporter_nodes
from pylint.utils import LinterStats

if TYPE_CHECKING:
from collections.abc import Iterator

from pylint.interfaces import Confidence
from pylint.lint.pylinter import PyLinter
from pylint.utils import LinterStats


class _BasicChecker(BaseChecker):
Expand Down
6 changes: 5 additions & 1 deletion pylint/checkers/base/basic_error_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,20 @@
from __future__ import annotations

import itertools
from typing import TYPE_CHECKING

import astroid
from astroid import nodes
from astroid.typing import InferenceResult

from pylint.checkers import utils
from pylint.checkers.base.basic_checker import _BasicChecker
from pylint.checkers.utils import infer_all
from pylint.interfaces import HIGH

if TYPE_CHECKING:
from astroid.typing import InferenceResult


ABC_METACLASSES = {"_py_abc.ABCMeta", "abc.ABCMeta"} # Python 3.7+,
# List of methods which can be redefined
REDEFINABLE_METHODS = frozenset(("__module__",))
Expand Down
6 changes: 5 additions & 1 deletion pylint/checkers/base/docstring_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from __future__ import annotations

import re
from typing import Literal
from typing import TYPE_CHECKING

import astroid
from astroid import nodes
Expand All @@ -21,6 +21,10 @@
is_property_setter,
)

if TYPE_CHECKING:
from typing import Literal


# do not require a doc string on private/system methods
NO_REQUIRED_DOC_RGX = re.compile("^_")

Expand Down
9 changes: 5 additions & 4 deletions pylint/checkers/base/name_checker/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,11 @@

from __future__ import annotations

import argparse
import collections
import itertools
import re
import sys
from collections.abc import Iterable
from enum import Enum, auto
from re import Pattern
from typing import TYPE_CHECKING

import astroid
Expand All @@ -29,10 +26,14 @@
_create_naming_options,
)
from pylint.checkers.utils import is_property_deleter, is_property_setter
from pylint.typing import Options

if TYPE_CHECKING:
import argparse
from collections.abc import Iterable
from re import Pattern

from pylint.lint.pylinter import PyLinter
from pylint.typing import Options

_BadNamesTuple = tuple[nodes.NodeNG, str, str, interfaces.Confidence]

Expand Down
8 changes: 6 additions & 2 deletions pylint/checkers/base/name_checker/naming_style.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@
from __future__ import annotations

import re
from re import Pattern
from typing import TYPE_CHECKING

from pylint import constants
from pylint.typing import OptionDict, Options

if TYPE_CHECKING:
from re import Pattern

from pylint.typing import OptionDict, Options


class NamingStyle:
Expand Down
28 changes: 15 additions & 13 deletions pylint/checkers/base_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,31 @@

import abc
import functools
from collections.abc import Iterable, Sequence
from inspect import cleandoc
from tokenize import TokenInfo
from typing import TYPE_CHECKING, Any

from astroid import nodes
from typing import TYPE_CHECKING

from pylint.config.arguments_provider import _ArgumentsProvider
from pylint.constants import _MSG_ORDER, MAIN_CHECKER_NAME, WarningScope
from pylint.exceptions import InvalidMessageError
from pylint.interfaces import Confidence
from pylint.message.message_definition import MessageDefinition
from pylint.typing import (
ExtraMessageOptions,
MessageDefinitionTuple,
OptionDict,
Options,
ReportsCallable,
)
from pylint.typing import ExtraMessageOptions
from pylint.utils import get_rst_section, get_rst_title

if TYPE_CHECKING:
from collections.abc import Iterable, Sequence
from tokenize import TokenInfo
from typing import Any

from astroid import nodes

from pylint.interfaces import Confidence
from pylint.lint import PyLinter
from pylint.typing import (
MessageDefinitionTuple,
OptionDict,
Options,
ReportsCallable,
)


@functools.total_ordering
Expand Down
20 changes: 12 additions & 8 deletions pylint/checkers/classes/class_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,12 @@
from __future__ import annotations

from collections import defaultdict
from collections.abc import Callable, Sequence
from functools import cached_property
from itertools import chain, zip_longest
from re import Pattern
from typing import TYPE_CHECKING, Any, NamedTuple, Union
from typing import TYPE_CHECKING, NamedTuple

import astroid
from astroid import bases, nodes, util
from astroid.nodes import LocalsDictNodeNG
from astroid.typing import SuccessfulInferenceResult
from astroid import nodes, util

from pylint.checkers import BaseChecker, utils
from pylint.checkers.utils import (
Expand All @@ -40,13 +36,21 @@
uninferable_final_decorators,
)
from pylint.interfaces import HIGH, INFERENCE
from pylint.typing import MessageDefinitionTuple

if TYPE_CHECKING:
from collections.abc import Callable, Sequence
from re import Pattern
from typing import Any, Union

from astroid import bases
from astroid.nodes import LocalsDictNodeNG
from astroid.typing import SuccessfulInferenceResult

from pylint.lint.pylinter import PyLinter
from pylint.typing import MessageDefinitionTuple

_AccessNodes = Union[nodes.Attribute, nodes.AssignAttr]

_AccessNodes = Union[nodes.Attribute, nodes.AssignAttr]

INVALID_BASE_CLASSES = {"bool", "range", "slice", "memoryview"}
ALLOWED_PROPERTIES = {"bultins.property", "functools.cached_property"}
Expand Down
14 changes: 10 additions & 4 deletions pylint/checkers/classes/special_methods_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@

from __future__ import annotations

from collections.abc import Callable
from typing import TYPE_CHECKING

import astroid
from astroid import bases, nodes, util
from astroid.context import InferenceContext
from astroid.typing import InferenceResult

from pylint.checkers import BaseChecker
from pylint.checkers.utils import (
Expand All @@ -22,7 +20,15 @@
only_required_for_messages,
safe_infer,
)
from pylint.lint.pylinter import PyLinter

if TYPE_CHECKING:
from collections.abc import Callable

from astroid.context import InferenceContext
from astroid.typing import InferenceResult

from pylint.lint.pylinter import PyLinter


NEXT_METHOD = "__next__"

Expand Down
9 changes: 7 additions & 2 deletions pylint/checkers/deprecated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

from __future__ import annotations

from collections.abc import Container, Iterable
from itertools import chain
from typing import TYPE_CHECKING

import astroid
from astroid import nodes
Expand All @@ -17,7 +17,12 @@
from pylint.checkers.base_checker import BaseChecker
from pylint.checkers.utils import get_import_name, infer_all, safe_infer
from pylint.interfaces import INFERENCE
from pylint.typing import MessageDefinitionTuple

if TYPE_CHECKING:
from collections.abc import Container, Iterable

from pylint.typing import MessageDefinitionTuple


ACCEPTABLE_NODES = (
astroid.BoundMethod,
Expand Down
9 changes: 6 additions & 3 deletions pylint/checkers/design_analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,22 @@

import re
from collections import defaultdict
from collections.abc import Iterator
from typing import TYPE_CHECKING

import astroid
from astroid import nodes

from pylint.checkers import BaseChecker
from pylint.checkers.utils import is_enum, only_required_for_messages
from pylint.interfaces import HIGH
from pylint.typing import MessageDefinitionTuple

if TYPE_CHECKING:
from collections.abc import Iterator

from astroid import nodes

from pylint.lint import PyLinter
from pylint.typing import MessageDefinitionTuple


MSGS: dict[str, MessageDefinitionTuple] = (
{ # pylint: disable=consider-using-namedtuple-or-dataclass
Expand Down
16 changes: 10 additions & 6 deletions pylint/checkers/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,25 @@

import builtins
import inspect
from collections.abc import Generator
from typing import TYPE_CHECKING, Any
from typing import TYPE_CHECKING

import astroid
from astroid import nodes, objects, util
from astroid.context import InferenceContext
from astroid.typing import InferenceResult, SuccessfulInferenceResult
from astroid import nodes, util

from pylint import checkers
from pylint.checkers import utils
from pylint.interfaces import HIGH, INFERENCE
from pylint.typing import MessageDefinitionTuple

if TYPE_CHECKING:
from collections.abc import Generator
from typing import Any

from astroid import objects
from astroid.context import InferenceContext
from astroid.typing import InferenceResult, SuccessfulInferenceResult

from pylint.lint import PyLinter
from pylint.typing import MessageDefinitionTuple


def _builtin_exceptions() -> set[str]:
Expand Down
8 changes: 5 additions & 3 deletions pylint/checkers/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,22 @@

import tokenize
from functools import reduce
from re import Match
from typing import TYPE_CHECKING, Literal
from typing import TYPE_CHECKING

from astroid import nodes

from pylint.checkers import BaseRawFileChecker, BaseTokenChecker
from pylint.checkers.utils import only_required_for_messages
from pylint.constants import WarningScope
from pylint.interfaces import HIGH
from pylint.typing import MessageDefinitionTuple
from pylint.utils.pragma_parser import OPTION_PO, PragmaParserError, parse_pragma

if TYPE_CHECKING:
from re import Match
from typing import Literal

from pylint.lint import PyLinter
from pylint.typing import MessageDefinitionTuple


_KEYWORD_TOKENS = {
Expand Down
13 changes: 8 additions & 5 deletions pylint/checkers/imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@
import os
import sys
from collections import defaultdict
from collections.abc import ItemsView, Sequence
from functools import cached_property
from typing import TYPE_CHECKING, Any, Union

import astroid
from astroid import nodes
from astroid.nodes._base_nodes import ImportNode

from pylint.checkers import BaseChecker, DeprecatedMixin
from pylint.checkers.utils import (
Expand All @@ -32,13 +30,18 @@
from pylint.exceptions import EmptyReportError
from pylint.graph import DotBackend, get_cycles
from pylint.interfaces import HIGH
from pylint.reporters.ureports.nodes import Paragraph, Section, VerbatimText
from pylint.typing import MessageDefinitionTuple
from pylint.reporters.ureports.nodes import Paragraph, VerbatimText
from pylint.utils import IsortDriver
from pylint.utils.linterstats import LinterStats

if TYPE_CHECKING:
from collections.abc import ItemsView, Sequence

from astroid.nodes._base_nodes import ImportNode

from pylint.lint import PyLinter
from pylint.reporters.ureports.nodes import Section
from pylint.typing import MessageDefinitionTuple
from pylint.utils.linterstats import LinterStats


# The dictionary with Any should actually be a _ImportTree again
Expand Down
Loading
Loading