Add message name is parameter and nonlocal #7577

ramonsaraiva · 2022-10-06T04:10:37Z

Type of Changes

	Type
✓	✨ New feature

Description

Adds a new message for when a nonlocal name is also a parameter in a function scope

Closes #6882

… a parameter

mbyrnepr2 · 2022-10-06T08:34:33Z

At first glance since this is a new check, could you also please add a good.py/bad.py example please? Here is where they live: https://github.com/PyCQA/pylint/tree/main/doc/data/messages.
Some recent additions here: https://github.com/PyCQA/pylint/pull/7576/files
(Often the names of fruit or animals are used for variable naming in the examples)

These can be tested locally with tox test_doc.

DanielNoord

Could you also add some test with nested scopes and functions?

DanielNoord · 2022-10-06T11:23:00Z

tests/functional/n/name/name_is_parameter_and_nonlocal.py

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


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?

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.

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()

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

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.

Yeah, and variants of it

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

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.

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

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?

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.

DanielNoord · 2022-10-06T11:23:21Z

pylint/checkers/base/basic_error_checker.py

+        "E0134": (
+            "Name %r is parameter and nonlocal",
+            "name-is-parameter-and-nonlocal",
+            "Emitted when a nonlocal variable is also a parameter.",


Suggested change

"Emitted when a nonlocal variable is also a parameter.",

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

DanielNoord · 2022-10-06T11:23:28Z

pylint/checkers/base/basic_error_checker.py

@@ -204,6 +204,11 @@ class BasicErrorChecker(_BasicChecker):
            "which results in an error since Python 3.6.",
            {"minversion": (3, 6)},
        ),
+        "E0134": (
+            "Name %r is parameter and nonlocal",


Suggested change

"Name %r is parameter and nonlocal",

"Name %r is both parameter and nonlocal",

I'm good to change to .. is both parameter and nonlocal, but python says .. is parameter and nonlocal:

>>> def tomato(is_tasty): ... nonlocal is_tasty ... File "<stdin>", line 2 SyntaxError: name 'is_tasty' is parameter and nonlocal

wdyt?

Hm, I'd say let's overwrite Python here and use more "readable" English. As pylint output is often read form IDEs I think we an be a little bit more verbose than the traditional error messages Python uses.

Totally agree.

Pierre-Sassoulas · 2022-10-06T11:45:14Z

doc/user_guide/checkers/features.rst

@@ -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 parameter and nonlocal*


You're one of the very few contributors that actually generated the doc before opening a merge request, conratulation 👍 (Actually, you're probably the first one 😄 )

Pierre-Sassoulas · 2022-10-06T11:46:28Z

pylint/checkers/base/basic_error_checker.py

@@ -327,6 +333,19 @@ def _check_name_used_prior_global(self, node: nodes.FunctionDef) -> None:
                    "used-prior-global-declaration", node=node_name, args=(name,)
                )



We can probably optimize by using util.only_required_for_messages here.

DanielNoord · 2022-11-09T19:44:26Z

This seems like an easy PR for a maintainer to pick up.

@ramonsaraiva are you still planning to return to this? If not, would you mind if I picked it up?

ramonsaraiva · 2022-11-09T21:45:45Z

All good if you want to pick it up @DanielNoord - things got busier around here. I'll keep an eye on the solution!

DanielNoord · 2022-11-13T13:01:12Z

The tests here show a genuine failing false positive.

github-actions · 2022-11-13T13:53:01Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on django:
The following messages are now emitted:

name-is-parameter-and-nonlocal:
Name 'condition' is both parameter and nonlocal
https://github.com/django/django/blob/1be7e36f85c927560e8c3c1eda05a7e43a66cd22/django/test/testcases.py#L1580

This comment was generated for commit de419ac

ramonsaraiva added 3 commits October 6, 2022 01:02

Add check for name-is-parameter-and-nonlocal

fae605d

Add message args, confidence and a test case for nonlocal that is not…

c3820a9

… a parameter

Fix fragment closes issue id

14efefd

DanielNoord added the Enhancement ✨ Improvement to a component label Oct 6, 2022

DanielNoord added this to the 2.16.0 milestone Oct 6, 2022

DanielNoord requested changes Oct 6, 2022

View reviewed changes

Pierre-Sassoulas requested changes Oct 6, 2022

View reviewed changes

Pierre-Sassoulas added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Nov 11, 2022

Rephrase message

de419ac

Pierre-Sassoulas modified the milestones: 2.16.0, 2.17.0 Jan 8, 2023

Pierre-Sassoulas modified the milestones: 2.17.0, 3.0.0 Mar 7, 2023

jacobtylerwalls mentioned this pull request Jun 18, 2023

.arguments property ignores keyword-only args, *args, and **kwargs pylint-dev/astroid#2213

Closed

mbyrnepr2 closed this Jun 19, 2023

jacobtylerwalls removed the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Jun 19, 2023

Pierre-Sassoulas modified the milestones: 3.0.0, 3.0.0a8 Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add message name is parameter and nonlocal #7577

Add message name is parameter and nonlocal #7577

ramonsaraiva commented Oct 6, 2022 •

edited by mbyrnepr2

Loading

mbyrnepr2 commented Oct 6, 2022 •

edited

Loading

DanielNoord left a comment

DanielNoord Oct 6, 2022

ramonsaraiva Oct 6, 2022

ramonsaraiva Oct 6, 2022

DanielNoord Oct 6, 2022

DanielNoord Oct 6, 2022

ramonsaraiva Oct 6, 2022

DanielNoord Oct 6, 2022

ramonsaraiva Oct 6, 2022

mbyrnepr2 Oct 6, 2022 •

edited

Loading

DanielNoord Oct 6, 2022

DanielNoord Oct 6, 2022

DanielNoord Oct 6, 2022

ramonsaraiva Oct 6, 2022

DanielNoord Oct 6, 2022

ramonsaraiva Oct 6, 2022

Pierre-Sassoulas Oct 6, 2022

Pierre-Sassoulas Oct 6, 2022

DanielNoord commented Nov 9, 2022

ramonsaraiva commented Nov 9, 2022

DanielNoord commented Nov 13, 2022

github-actions bot commented Nov 13, 2022

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

	"Name %r is parameter and nonlocal",
	"Name %r is both parameter and nonlocal",

		@@ -327,6 +333,19 @@ def _check_name_used_prior_global(self, node: nodes.FunctionDef) -> None:
		"used-prior-global-declaration", node=node_name, args=(name,)
		)

Add message name is parameter and nonlocal #7577

Add message name is parameter and nonlocal #7577

Conversation

ramonsaraiva commented Oct 6, 2022 • edited by mbyrnepr2 Loading

Type of Changes

Description

mbyrnepr2 commented Oct 6, 2022 • edited Loading

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbyrnepr2 Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Nov 9, 2022

ramonsaraiva commented Nov 9, 2022

DanielNoord commented Nov 13, 2022

github-actions bot commented Nov 13, 2022

ramonsaraiva commented Oct 6, 2022 •

edited by mbyrnepr2

Loading

mbyrnepr2 commented Oct 6, 2022 •

edited

Loading

mbyrnepr2 Oct 6, 2022 •

edited

Loading