Skip to content

Add message name is parameter and nonlocal #7577

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

Closed
wants to merge 4 commits into from
Closed

Add message name is parameter and nonlocal #7577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fae605d
Add check for name-is-parameter-and-nonlocal
ramonsaraiva Oct 6, 2022
c3820a9
Add message args, confidence and a test case for nonlocal that is not…
ramonsaraiva Oct 6, 2022
14efefd
Fix fragment closes issue id
ramonsaraiva Oct 6, 2022
de419ac
Rephrase message
DanielNoord Nov 13, 2022
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
2 changes: 2 additions & 0 deletions doc/user_guide/checkers/features.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Basic checker Messages
Emitted when a name is used prior a global declaration, which results in an
error since Python 3.6. This message can't be emitted when using Python <
3.6.
:name-is-parameter-and-nonlocal (E0134): *Name %r is both parameter and nonlocal*
Emitted when a nonlocal variable is also a parameter.
:return-outside-function (E0104): *Return outside function*
Used when a "return" statement is found outside a function or method.
:return-arg-in-generator (E0106): *Return with argument inside generator*
Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ All messages in the error category:
error/mixed-format-string
error/modified-iterating-dict
error/modified-iterating-set
error/name-is-parameter-and-nolocal
error/no-member
error/no-method-argument
error/no-name-in-module
Expand Down
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/6882.new_check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Added ``name-is-parameter-and-nonlocal`` which flags when a nonlocal variable is also a paremeter.

Closes #6882
20 changes: 20 additions & 0 deletions pylint/checkers/base/basic_error_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ class BasicErrorChecker(_BasicChecker):
"which results in an error since Python 3.6.",
{"minversion": (3, 6)},
),
"E0134": (
"Name %r is both parameter and nonlocal",
"name-is-parameter-and-nonlocal",
"Emitted when a nonlocal variable is also a parameter.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Emitted when a nonlocal variable is also a parameter.",
"Emitted when a nonlocal variable is also a parameter within a function defintion.",

),
}

def open(self) -> None:
Expand Down Expand Up @@ -264,9 +269,11 @@ def visit_starred(self, node: nodes.Starred) -> None:
"duplicate-argument-name",
"nonlocal-and-global",
"used-prior-global-declaration",
"name-is-parameter-and-nonlocal",
)
def visit_functiondef(self, node: nodes.FunctionDef) -> None:
self._check_nonlocal_and_global(node)
self._check_nonlocal_and_parameter(node)
self._check_name_used_prior_global(node)
if not redefined_by_decorator(
node
Expand Down Expand Up @@ -327,6 +334,19 @@ def _check_name_used_prior_global(self, node: nodes.FunctionDef) -> None:
"used-prior-global-declaration", node=node_name, args=(name,)
)

Copy link
Member

Choose a reason for hiding this comment

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

We can probably optimize by using util.only_required_for_messages here.

def _check_nonlocal_and_parameter(self, node: nodes.FunctionDef) -> None:
"""Check if a name is both parameter and nonlocal."""
node_arg_names = {arg.name for arg in node.args.args}
for body_node in node.nodes_of_class(nodes.Nonlocal):
for non_local_name in body_node.names:
if non_local_name in node_arg_names:
self.add_message(
"name-is-parameter-and-nonlocal",
args=(non_local_name,),
node=body_node,
confidence=HIGH,
)

def _check_nonlocal_and_global(self, node: nodes.FunctionDef) -> None:
"""Check that a name is both nonlocal and global."""

Expand Down
6 changes: 6 additions & 0 deletions tests/functional/n/name/name_is_parameter_and_nonlocal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# pylint: disable=missing-docstring,invalid-name,nonlocal-without-binding


def tomato(is_tasty: bool = True):
nonlocal color
nonlocal is_tasty # [name-is-parameter-and-nonlocal]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nonlocal is_tasty # [name-is-parameter-and-nonlocal]
nonlocal is_tasty # [name-is-parameter-and-nonlocal]
def tomato(*is_tasty):
nonlocal is_tasty # [name-is-parameter-and-nonlocal]

I think this would also raise a syntax error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess for that I can do something like

        node_arg_names = {arg.name for arg in node.args.args}
        for local_name, local_nodes in node.locals.items():
            for local_node in local_nodes:
                if isinstance(local_node, nodes.Arguments):
                    node_arg_names.add(local_name)

Not sure if there's an easier way to retrieve locals that are considered arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For nested scopes and functions, are you looking for something like this?

def grape(is_tasty: bool = True):
    nonlocal is_tasty # [name-is-parameter-and-nonlocal]

    def grape_juice(is_tasty: bool = True):
        nonlocal is_tasty  # [name-is-parameter-and-nonlocal]

    grape_juice()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't node.args.arguments() work? According to the docstring that should be able to return all names that are used as parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, and variants of it. I haven't tested this out locally to see which combinations return a syntax error, but I think it is good to add some (regression tests) for possible combinations:

  • outer scope with nonlocal inner scope with nonlocal
  • outer scope without nonlocal inner scope with nonlocal
  • outer scope with nonlocal inner scope without nonlocal
    etc.

Scoping rules can create annoying false positives for messages like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and variants of it

Cool, I'll be coming back to this later today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shit, that means that arguments is broken. It should also return vararg. I think we need to push a fix to astroid for this: any implementation we create in pylint to solve this would eventually end up being duplicate code with a good implementation of arguments.
Would you want to fix astroid for this? Should be relatively straightforward: a test with * and ** and an assertion on the output of arguments(). I think/hope we will already have other tests for the output of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I can attempt that later today, this might also be related pylint-dev/astroid#761 - but not totally sure.

Copy link
Member

@mbyrnepr2 mbyrnepr2 Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Similar issue for node.args.find_argname when using a vararg as a parameter:

(Pdb) node.args.find_argname("tasty")
(None, None)

I wouldn't be against separating these astroid issues into an independent workflow so we can let the current issue proceed to merge. We could add a maintenance issue and update the code here once they are resolved?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed let's not block this on too much refactoring and fix stuff as we run into them. I think this is a very clear use-case for a potential fix and I'd be happy to help release a patch version of astroid for it. I think there are other issues waiting on a patch as well, a regression in dataclasses for example.

1 change: 1 addition & 0 deletions tests/functional/n/name/name_is_parameter_and_nonlocal.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
name-is-parameter-and-nonlocal:6:4:6:21:tomato:Name 'is_tasty' is both parameter and nonlocal:HIGH