Skip to content

Create a TypeVar style for invalid-name #5894

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.

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

Merged
merged 12 commits into from
Mar 24, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,11 @@ Release date: TBA
Insert your changelog randomly, it will reduce merge conflicts
(Ie. not necessarily at the end)

* Improve ``invalid-name`` check for ``TypeVar`` names.
The accepted pattern can be customized with ``--typevar-rgx``.

Closes #3401

* Added new checker ``typevar-name-missing-variance``. Emitted when a covariant
or contravariant ``TypeVar`` does not end with ``_co`` or ``_contra`` respectively or
when a ``TypeVar`` is not either but has a suffix.
Expand Down
13 changes: 13 additions & 0 deletions doc/user_guide/options.rst
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ name is found in, and not the type of object assigned.
+--------------------+---------------------------------------------------------------------------------------------------+
| ``inlinevar`` | Loop variables in list comprehensions and generator expressions. |
+--------------------+---------------------------------------------------------------------------------------------------+
| ``typevar`` | Type variable declared with ``TypeVar``. |
+--------------------+---------------------------------------------------------------------------------------------------+

Default behavior
~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -82,6 +84,15 @@ Following options are exposed:

.. option:: --inlinevar-naming-style=<style>

Predefined Naming Patterns
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Pylint provides predefined naming patterns for some names. These patterns are often
based on a Naming Style but there is no option to choose one of the styles mentioned above.
The pattern can be overwritten with the options discussed below.

The following type of names are checked with a predefined pattern:

- TypeVars. Accepted names include: `T`, `TypeVar`, `_CallableT`, `_T_co`.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a table would make sense here? With Name type, Good names, and Bad names.

I would prefer not to suggest TypeVar. True it's valid, but it shouldn't be used IMO. Let's use something like MyClassT to highlight PascalCase. For bad names, we should definitely mention something like T_Class, and MyClassType.

Some other names from the tests: AnyStr, DeviceTypeT, DeviceType, CALLABLE_T.


Custom regular expressions
~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -118,6 +129,8 @@ expression will lead to an instance of ``invalid-name``.

.. option:: --inlinevar-rgx=<regex>

.. option:: --typevar-rgx=<regex>

Multiple naming styles for custom regular expressions
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,11 @@ Other Changes

Closes #5360, #3877

* Improve ``invalid-name`` check for ``TypeVar`` names.
The accepted pattern can be customized with ``--typevar-rgx``.

Closes #3401

* Fixed a false positive (affecting unreleased development) for
``used-before-assignment`` involving homonyms between filtered comprehensions
and assignments in except blocks.
Expand Down
87 changes: 55 additions & 32 deletions pylint/checkers/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
import itertools
import re
import sys
from typing import TYPE_CHECKING, Any, Dict, Iterator, Optional, Pattern, cast
from typing import TYPE_CHECKING, Any, Dict, Iterator, Optional, Pattern, Tuple, cast

import astroid
from astroid import nodes
Expand Down Expand Up @@ -189,6 +189,13 @@ class AnyStyle(NamingStyle):
"any": AnyStyle,
}

# Default patterns for name types that do not have styles
DEFAULT_PATTERNS = {
"typevar": re.compile(
r"^_{0,2}(?:[^\W\da-z_]+|(?:[^\W\da-z_][^\WA-Z_]+)+T?(?<!Type))(?:_co(?:ntra)?)?$"
)
}

# do not require a doc string on private/system methods
NO_REQUIRED_DOC_RGX = re.compile("^_")
REVERSED_PROTOCOL_METHOD = "__reversed__"
Expand Down Expand Up @@ -1660,7 +1667,8 @@ def visit_for(self, node: nodes.For) -> None:
self._check_redeclared_assign_name([node.target])


KNOWN_NAME_TYPES = {
# Name types that have a style option
KNOWN_NAME_TYPES_WITH_STYLE = {
"module",
"const",
"class",
Expand All @@ -1674,6 +1682,12 @@ def visit_for(self, node: nodes.For) -> None:
"inlinevar",
}

# Name types that have a 'rgx' option
KNOWN_NAME_TYPES = {
*KNOWN_NAME_TYPES_WITH_STYLE,
"typevar",
}

DEFAULT_NAMING_STYLES = {
"module": "snake_case",
"const": "UPPER_CASE",
Expand All @@ -1693,28 +1707,37 @@ def _create_naming_options():
name_options = []
for name_type in sorted(KNOWN_NAME_TYPES):
human_readable_name = constants.HUMAN_READABLE_TYPES[name_type]
default_style = DEFAULT_NAMING_STYLES[name_type]
name_type = name_type.replace("_", "-")
name_options.append(
(
f"{name_type}-naming-style",
{
"default": default_style,
"type": "choice",
"choices": list(NAMING_STYLES.keys()),
"metavar": "<style>",
"help": f"Naming style matching correct {human_readable_name} names.",
},
name_type_hyphened = name_type.replace("_", "-")

help_msg = f"Regular expression matching correct {human_readable_name} names. "
if name_type in KNOWN_NAME_TYPES_WITH_STYLE:
help_msg += f"Overrides {name_type_hyphened}-naming-style. "
help_msg += f"If left empty, {human_readable_name} will be checked with the set naming style."

# Add style option for names that support it
if name_type in KNOWN_NAME_TYPES_WITH_STYLE:
default_style = DEFAULT_NAMING_STYLES[name_type]
name_options.append(
(
f"{name_type_hyphened}-naming-style",
{
"default": default_style,
"type": "choice",
"choices": list(NAMING_STYLES.keys()),
"metavar": "<style>",
"help": f"Naming style matching correct {human_readable_name} names.",
},
)
)
)

name_options.append(
(
f"{name_type}-rgx",
f"{name_type_hyphened}-rgx",
{
"default": None,
"type": "regexp",
"metavar": "<regexp>",
"help": f"Regular expression matching correct {human_readable_name} names. Overrides {name_type}-naming-style.",
"help": help_msg,
},
)
)
Expand Down Expand Up @@ -1862,15 +1885,19 @@ def open(self):
re.compile(rgxp) for rgxp in self.config.bad_names_rgxs
]

def _create_naming_rules(self):
regexps = {}
hints = {}
def _create_naming_rules(self) -> Tuple[Dict[str, Pattern[str]], Dict[str, str]]:
regexps: Dict[str, Pattern[str]] = {}
hints: Dict[str, str] = {}

for name_type in KNOWN_NAME_TYPES:
naming_style_option_name = f"{name_type}_naming_style"
naming_style_name = getattr(self.config, naming_style_option_name)

regexps[name_type] = NAMING_STYLES[naming_style_name].get_regex(name_type)
if name_type in KNOWN_NAME_TYPES_WITH_STYLE:
naming_style_name = getattr(self.config, f"{name_type}_naming_style")
regexps[name_type] = NAMING_STYLES[naming_style_name].get_regex(
name_type
)
else:
naming_style_name = "predefined"
regexps[name_type] = DEFAULT_PATTERNS[name_type]

custom_regex_setting_name = f"{name_type}_rgx"
custom_regex = getattr(self.config, custom_regex_setting_name, None)
Expand Down Expand Up @@ -2078,14 +2105,6 @@ def _name_disallowed_by_regex(self, name: str) -> bool:
def _check_name(self, node_type, name, node, confidence=interfaces.HIGH):
"""Check for a name using the type's regexp."""

# pylint: disable=fixme
# TODO: move this down in the function and check TypeVar
# for name patterns as well.
# Check TypeVar names for variance suffixes
if node_type == "typevar":
self._check_typevar_variance(name, node)
return

def _should_exempt_from_invalid_name(node):
if node_type == "variable":
inferred = utils.safe_infer(node)
Expand All @@ -2111,6 +2130,10 @@ def _should_exempt_from_invalid_name(node):
if match is None and not _should_exempt_from_invalid_name(node):
self._raise_name_warning(None, node, node_type, name, confidence)

# Check TypeVar names for variance suffixes
if node_type == "typevar":
self._check_typevar_variance(name, node)

def _check_assign_to_new_keyword_violation(self, name, node):
keyword_first_version = self._name_became_keyword_in_version(
name, self.KEYWORD_ONSET
Expand Down
1 change: 1 addition & 0 deletions pylint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class WarningScope:
"class_attribute": "class attribute",
"class_const": "class constant",
"inlinevar": "inline iteration",
"typevar": "type variable",
}


Expand Down
7 changes: 7 additions & 0 deletions pylint/utils/linterstats.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class BadNames(TypedDict):
method: int
module: int
variable: int
typevar: int


class CodeTypeCount(TypedDict):
Expand Down Expand Up @@ -102,6 +103,7 @@ def __init__(
method=0,
module=0,
variable=0,
typevar=0,
)
self.by_module: Dict[str, ModuleStats] = by_module or {}
self.by_msg: Dict[str, int] = by_msg or {}
Expand Down Expand Up @@ -171,6 +173,7 @@ def get_bad_names(
"method",
"module",
"variable",
"typevar",
],
) -> int:
"""Get a bad names node count."""
Expand All @@ -192,6 +195,7 @@ def increase_bad_name(self, node_name: str, increase: int) -> None:
"method",
"module",
"variable",
"typevar",
}:
raise ValueError("Node type not part of the bad_names stat")

Expand All @@ -208,6 +212,7 @@ def increase_bad_name(self, node_name: str, increase: int) -> None:
"method",
"module",
"variable",
"typevar",
],
node_name,
)
Expand All @@ -230,6 +235,7 @@ def reset_bad_names(self) -> None:
method=0,
module=0,
variable=0,
typevar=0,
)

def get_code_count(
Expand Down Expand Up @@ -317,6 +323,7 @@ def merge_stats(stats: List[LinterStats]):
merged.bad_names["method"] += stat.bad_names["method"]
merged.bad_names["module"] += stat.bad_names["module"]
merged.bad_names["variable"] += stat.bad_names["variable"]
merged.bad_names["typevar"] += stat.bad_names["typevar"]

for mod_key, mod_value in stat.by_module.items():
merged.by_module[mod_key] = mod_value
Expand Down
4 changes: 4 additions & 0 deletions pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ method-rgx=[a-z_][a-z0-9_]{2,}$
# Naming hint for method names
method-name-hint=[a-z_][a-z0-9_]{2,}$

# Regular expression which can overwrite the naming style set by typevar-naming-style.
# If left empty, type variables will be checked with the set naming style.
typevar-rgx=
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't work. If pattern is empty, it will match an empty pattern.
https://github.com/PyCQA/pylint/blob/d01e5e167834097ff7e5bf26a069aa1339669183/pylint/config/option.py#L102

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked. If it is empty the setting will be None (I think this is due to how the config file is parsed). The following lines make sure that the pattern is only picked up if the pattern is set. (Note that this is also how this works for all other patterns).

custom_regex = getattr(self.config, custom_regex_setting_name, None)
if custom_regex is not None:
    regexps[name_type] = custom_regex

Copy link
Member

Choose a reason for hiding this comment

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

An empty setting will be re.compile('') and thus overwrite regexps[name_type]. That isn't bad, we just can't leave it empty in pylintrc. Otherwise TypeVar names in pylint itself won't be checked. That's similar to the other regex settings in pylintrc, eg. class-rgx.

Alternatively, an possibly my preference would be the comment out that line. Similar to #class-const-rgx=.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I understand why I was mistaken. The functional test don't actually use this pylintrc so they passed even with this uncommented.

Commented it out!


# Regular expression which should only match function or class names that do
# not require a docstring. Use ^(?!__init__$)_ to also check __init__.
no-docstring-rgx=__.*__
Expand Down
4 changes: 2 additions & 2 deletions tests/checkers/unittest_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,13 @@ class TestNamePresets(unittest.TestCase):
def _test_name_is_correct_for_all_name_types(
self, naming_style: Type[base.NamingStyle], name: str
) -> None:
for name_type in base.KNOWN_NAME_TYPES:
for name_type in base.KNOWN_NAME_TYPES_WITH_STYLE:
self._test_is_correct(naming_style, name, name_type)

def _test_name_is_incorrect_for_all_name_types(
self, naming_style: Type[base.NamingStyle], name: str
) -> None:
for name_type in base.KNOWN_NAME_TYPES:
for name_type in base.KNOWN_NAME_TYPES_WITH_STYLE:
self._test_is_incorrect(naming_style, name, name_type)

def _test_should_always_pass(self, naming_style: Type[base.NamingStyle]) -> None:
Expand Down
72 changes: 72 additions & 0 deletions tests/functional/t/typevar_naming_style_default.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
"""Test case for typevar-name-incorrect-variance with default settings"""
# pylint: disable=too-few-public-methods

from typing import TypeVar

# PascalCase names with prefix
GoodNameT = TypeVar("GoodNameT")
_T = TypeVar("_T")
_GoodNameT = TypeVar("_GoodNameT")
__GoodNameT = TypeVar("__GoodNameT")
GoodNameWithoutContra = TypeVar( # [typevar-name-incorrect-variance]
"GoodNameWithoutContra", contravariant=True
)
GoodNameT_co = TypeVar("GoodNameT_co", covariant=True)
GoodNameT_contra = TypeVar("GoodNameT_contra", contravariant=True)
GoodBoundNameT = TypeVar("GoodBoundNameT", bound=int)
GoodBoundNameT_co = TypeVar( # [typevar-name-incorrect-variance]
"GoodBoundNameT_co", bound=int
)
GoodBoundNameT_contra = TypeVar( # [typevar-name-incorrect-variance]
"GoodBoundNameT_contra", bound=int
)
GoodBoundNameT_co = TypeVar("GoodBoundNameT_co", bound=int, covariant=True)
GoodBoundNameT_contra = TypeVar("GoodBoundNameT_contra", bound=int, contravariant=True)

GoodBoundNameT_co = TypeVar("GoodBoundNameT_co", int, str, covariant=True)
GoodBoundNameT_co = TypeVar( # [typevar-name-incorrect-variance]
"GoodBoundNameT_co", int, str, contravariant=True
)
GoodBoundNameT_contra = TypeVar("GoodBoundNameT_contra", int, str, contravariant=True)
GoodBoundNameT_contra = TypeVar( # [typevar-name-incorrect-variance]
"GoodBoundNameT_contra", int, str, covariant=True
)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove some of the GoodName TypeVars here? We already have dedicated tests for the typevar-name-incorrect-variance so I don't think we need to test this again here.

Basically just keep

# PascalCase names with prefix
GoodNameT = TypeVar("GoodNameT")
_T = TypeVar("_T")
_GoodNameT = TypeVar("_GoodNameT")
__GoodNameT = TypeVar("__GoodNameT")
GoodNameT_co = TypeVar("GoodNameT_co", covariant=True)
GoodNameT_contra = TypeVar("GoodNameT_contra", contravariant=True)


# Some of these will create a RunTime error but serve as a regression test
T = TypeVar( # [typevar-name-incorrect-variance]
"T", covariant=True, contravariant=True
)
T = TypeVar("T", covariant=False, contravariant=False)
T_co = TypeVar("T_co", covariant=True, contravariant=True)
T_contra = TypeVar( # [typevar-name-incorrect-variance]
"T_contra", covariant=True, contravariant=True
)
T_co = TypeVar("T_co", covariant=True, contravariant=False)
T_contra = TypeVar("T_contra", covariant=False, contravariant=True)

# PascalCase names without prefix
AnyStr = TypeVar("AnyStr")
DeviceTypeT = TypeVar("DeviceTypeT")
CALLABLE_T = TypeVar("CALLABLE_T") # [invalid-name]
DeviceType = TypeVar("DeviceType") # [invalid-name]
GoodNameWithoutContra = TypeVar( # [typevar-name-incorrect-variance]
"GoodNameWithoutContra", contravariant=True
)

# camelCase names with prefix
badName = TypeVar("badName") # [invalid-name]
badNameWithoutContra = TypeVar( # [invalid-name, typevar-name-incorrect-variance]]
"badNameWithoutContra", contravariant=True
)
badName_co = TypeVar("badName_co", covariant=True) # [invalid-name]
badName_contra = TypeVar("badName_contra", contravariant=True) # [invalid-name]

# PascalCase names with lower letter prefix in tuple assignment
(
a_BadName, # [invalid-name]
a_BadNameWithoutContra, # [invalid-name, typevar-name-incorrect-variance]
) = TypeVar("a_BadName"), TypeVar("a_BadNameWithoutContra", contravariant=True)
GoodName_co, a_BadName_contra = TypeVar( # [invalid-name]
"GoodName_co", covariant=True
), TypeVar("a_BadName_contra", contravariant=True)
GoodName_co, VAR = TypeVar("GoodName_co", covariant=True), "a string"
19 changes: 19 additions & 0 deletions tests/functional/t/typevar_naming_style_default.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
typevar-name-incorrect-variance:11:0:11:21::"Type variable ""GoodNameWithoutContra"" is contravariant, use ""GoodNameWithoutContra_contra"" instead":INFERENCE
typevar-name-incorrect-variance:17:0:17:17::"Type variable ""GoodBoundNameT_co"" is invariant, use ""GoodBoundNameT"" instead":INFERENCE
typevar-name-incorrect-variance:20:0:20:21::"Type variable ""GoodBoundNameT_contra"" is invariant, use ""GoodBoundNameT"" instead":INFERENCE
typevar-name-incorrect-variance:27:0:27:17::"Type variable ""GoodBoundNameT_co"" is contravariant, use ""GoodBoundNameT_contra"" instead":INFERENCE
typevar-name-incorrect-variance:31:0:31:21::"Type variable ""GoodBoundNameT_contra"" is covariant, use ""GoodBoundNameT_co"" instead":INFERENCE
typevar-name-incorrect-variance:36:0:36:1::"Type variable ""T"" is covariant, use ""T_co"" instead":INFERENCE
typevar-name-incorrect-variance:41:0:41:8::"Type variable ""T_contra"" is covariant, use ""T_co"" instead":INFERENCE
invalid-name:50:0:50:10::"Type variable name ""CALLABLE_T"" doesn't conform to predefined naming style":HIGH
invalid-name:51:0:51:10::"Type variable name ""DeviceType"" doesn't conform to predefined naming style":HIGH
typevar-name-incorrect-variance:52:0:52:21::"Type variable ""GoodNameWithoutContra"" is contravariant, use ""GoodNameWithoutContra_contra"" instead":INFERENCE
invalid-name:57:0:57:7::"Type variable name ""badName"" doesn't conform to predefined naming style":HIGH
invalid-name:58:0:58:20::"Type variable name ""badNameWithoutContra"" doesn't conform to predefined naming style":HIGH
typevar-name-incorrect-variance:58:0:58:20::"Type variable ""badNameWithoutContra"" is contravariant, use ""badNameWithoutContra_contra"" instead":INFERENCE
invalid-name:61:0:61:10::"Type variable name ""badName_co"" doesn't conform to predefined naming style":HIGH
invalid-name:62:0:62:14::"Type variable name ""badName_contra"" doesn't conform to predefined naming style":HIGH
invalid-name:66:4:66:13::"Type variable name ""a_BadName"" doesn't conform to predefined naming style":HIGH
invalid-name:67:4:67:26::"Type variable name ""a_BadNameWithoutContra"" doesn't conform to predefined naming style":HIGH
typevar-name-incorrect-variance:67:4:67:26::"Type variable ""a_BadNameWithoutContra"" is contravariant, use ""a_BadNameWithoutContra_contra"" instead":INFERENCE
invalid-name:69:13:69:29::"Type variable name ""a_BadName_contra"" doesn't conform to predefined naming style":HIGH
Loading