bugfix - `assignment-from-none` False Positive #7853 #7886

orSolocate · 2022-12-01T18:43:34Z

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

See comment about this PR in this comment.

Closes #7853

orSolocate · 2022-12-01T18:52:09Z

That is interesting:

Expected in testdata:
  35: possibly-unused-variable

Unexpected in testdata:
  35: unused-variable
FAILED tests/test_functional.py::test_functional[useless_with_lock] - AssertionError: Wrong results for file "useless_with_lock":

Pierre-Sassoulas

We should definitely not touch safe_infer for something happening in a particular checker, this is going to affect a lot of code. What we can do is make astroid better for a specific piece of code it parse, or make the checker handle a specific piece of code it analyses.

orSolocate · 2022-12-03T00:33:43Z

We should definitely not touch safe_infer for something happening in a particular checker, this is going to affect a lot of code.

I uploaded a fix that refactors the safe_infer method and using its parts to build a safe_infer_multiple function. It took a lot of effort to make all regression tests pass locally.. since there are many logical flows :(
The safe_infer_multiple returns all return nodes and checks all of them, no matter the order whatsoever. The typecheck checker use this fucntion to check for the assignment-from-none and the assignment-no-return messages.

There were some unnecessary things in the original function, like the use of a set that can have a maximum of 1 element, and then verifying that in a condition in the return statement..

github-actions · 2022-12-03T01:28:23Z

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

Effect on sentry:
The following messages are no longer emitted:

assignment-from-no-return:
Assigning result of a function call, where the function has no return
https://github.com/getsentry/sentry/blob/916fe38f45a23d27f2bde851e6521b66c1404299/src/sentry/models/userip.py#L47

This comment was generated for commit b992e12

orSolocate · 2022-12-03T10:07:23Z

EDIT:

Not sure where it fails, when I run test_runner_with_arguments[run_pylint] test locally it passes..

The /home/runner/work/pylint/pylint/tests/test_pylint_runners.py:38:4: E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return) error does not pop out on my machine...

When I run the tox tests I get the error.. is it something that has to do with changing something about the tox environment? Could anybody help solve this mystery?

DanielNoord

I'm still not sure whether I like this approach. It feels like we're complicating the safe_infer util for a very specific use case.
We could also consider doing the double infer call in the checker itself. We would need to duplicate a little bit of safe_infer inside the checker, but at least the safe_infer function remains somewhat legible.

Perhaps other maintainers have an opinion here?

DanielNoord · 2022-12-04T11:37:17Z

.idea/.gitignore

@@ -0,0 +1,3 @@
+# Default ignored files


This shouldn't be committed.

yeah my bad, will remove it in my next commit

DanielNoord · 2022-12-04T11:37:38Z

doc/whatsnew/fragments/7853.bugfix

@@ -0,0 +1,3 @@
+Fix False Positive issue regarding a complex ``assignment-from-none``.


I think this might need some more specific details, but we can figure that out later.

Agree. yeah we just don't know what is the final solution yet.

DanielNoord · 2022-12-04T11:38:07Z

pylint/checkers/typecheck.py

-        function_node = safe_infer(node.value.func)
-        funcs = (nodes.FunctionDef, astroid.UnboundMethod, astroid.BoundMethod)
-        if not isinstance(function_node, funcs):
+        ASTROID_FUNC_TYPES = (


Let's make this a frozen set at either the Module or Class level.

DanielNoord · 2022-12-04T11:38:19Z

pylint/checkers/typecheck.py

+            astroid.BoundMethod,
+        )
+        return_nodes = []
+        function_nodes: Any | None = safe_infer_multiple(node.value.func)


Suggested change

function_nodes: Any | None = safe_infer_multiple(node.value.func)

function_nodes = safe_infer_multiple(node.value.func)

DanielNoord · 2022-12-04T11:38:34Z

pylint/checkers/typecheck.py

+            astroid.UnboundMethod,
+            astroid.BoundMethod,
+        )
+        return_nodes = []


Can we add annotation here?

DanielNoord · 2022-12-04T11:39:04Z

pylint/checkers/typecheck.py

-        if not isinstance(function_node, funcs):
+        ASTROID_FUNC_TYPES = (
+            nodes.FunctionDef,
+            astroid.UnboundMethod,


Ideally these get imported from bases.

DanielNoord · 2022-12-04T11:39:24Z

pylint/checkers/typecheck.py

+            # Make sure that it's a valid function that we can analyze.
+            # Ordered from less expensive to more expensive checks.
+            if (
+                not function_node.is_function


Is this really necessary?

Edit: Ah I see that most of this is just indenting. Hmm, well we could also fix this in a follow up, but it feels unnecessary.

The purpose of refactoring safe_infer was only to separate it to functions. Any logic should not have been modified.

DanielNoord · 2022-12-04T11:42:48Z

pylint/checkers/utils.py

+    context: InferenceContext | None = None,
+    *,
+    compare_constants: bool = False,
+) -> Any | None:


Why is this Any?

I asked myself the same question. But this is what mypy accepted. :S

Yes, that's because mypy doesn't do any checking on astroid. It should be InferenceResult.

Pierre-Sassoulas

We might need to extend safe_infer, but I consider it to be an API. It's used too much in pylint as well as in lib depending on it, to be open to modification.

orSolocate · 2022-12-04T13:36:36Z

I agree that this function is super fragile. I had a really hard time making all the functional tests pass..

The purpose of the change was just to refactor safe_infer to be able to use its parts in a different function to be used by the problematic checker. The only parts that changed is that the inferred_types set is removed, since it can contain a maximum of 1 element, and the ending return else case removed, since it can never be reached, for the same reason.

orSolocate · 2022-12-04T13:41:02Z

We could also consider doing the double infer call in the checker itself.

@DanielNoord - I think I didn't understand the suggestion here, could you explain more on that please?

DanielNoord · 2022-12-04T13:42:02Z

We could also consider doing the double infer call in the checker itself.

@DanielNoord - I think I didn't understand the suggestion here, could you explain more on that please?

The issue is that for some nodes we want to do a double inference right? So not call safe_infer once, but actually twice? Can't we hide that logic inside the checker?

orSolocate · 2022-12-04T14:20:13Z

EDIT: first of all thank you for reviewing the code and for your insights, they've always been very significant. Even though you didn't like the spirit of this PR initially..

The issue is that for some nodes we want to do a double inference right? So not call safe_infer once, but actually twice? Can't we hide that logic inside the checker?

Not necessary twice, for our case which is a function infer, you want safe_infer to retrieve all function declarations, it can theoretically be any number, see this example:

"""

def get_func(param):
    if param is None:
        def func():
            return None
    elif isinstance(param, int):
        def func():
            return str(param)
    else:
        def func():
            return param
    return func

"""

In this case you want to check return nodes for all 3 functionDefs.
The safe_infer returns only the first functionDef, so you can't get around it the way I see it without modifying what it returns..

DanielNoord · 2022-12-04T15:16:21Z

Can't we get away with calling .infer() then? We'd need to catch StopIteration and InferenceError, but we would get access to all inferred nodes.

jacobtylerwalls · 2023-01-16T04:09:58Z

safe_infer()'s docstring should really be updated to say return the first inferred value to avoid confusion.

jacobtylerwalls · 2023-04-17T12:31:11Z

We already have a cached util for getting all inferred values, it's utils.infer_all().

jacobtylerwalls · 2023-07-03T13:48:40Z

Thanks for helping think through these issues. I think changing the assignment-from-none checker to use infer_all() instead of safe_infer() is the right approach. Feel free to reopen or start with a fresh PR if you'd like to continue with the contribution.

Bugfix for False Positive pylint-dev#7853

7f00547

Pierre-Sassoulas added the Work in progress label Dec 1, 2022

Pierre-Sassoulas reviewed Dec 1, 2022

View reviewed changes

jacobtylerwalls changed the title ~~bugfix - assignment-from-none False Positive #7853~~ bugfix - assignment-from-none False Positive #7853 Dec 2, 2022

orSolocate added 2 commits December 3, 2022 01:43

safe_infer refactored and working unchanged

6b6eef0

safe_infer and typecheck.py patch to cover assign-from-none bug

b992e12

orSolocate mentioned this pull request Dec 3, 2022

False Positive - assignment-from-none raised when function not necessarily returns None #7853

Open

DanielNoord self-requested a review December 3, 2022 14:32

DanielNoord reviewed Dec 4, 2022

View reviewed changes

Pierre-Sassoulas requested changes Dec 4, 2022

View reviewed changes

jacobtylerwalls closed this Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix - `assignment-from-none` False Positive #7853 #7886

bugfix - `assignment-from-none` False Positive #7853 #7886

orSolocate commented Dec 1, 2022

orSolocate commented Dec 1, 2022

Pierre-Sassoulas left a comment •

edited

Loading

orSolocate commented Dec 3, 2022

github-actions bot commented Dec 3, 2022

orSolocate commented Dec 3, 2022 •

edited

Loading

DanielNoord left a comment

DanielNoord Dec 4, 2022

orSolocate Dec 4, 2022

DanielNoord Dec 4, 2022

orSolocate Dec 4, 2022

DanielNoord Dec 4, 2022

DanielNoord Dec 4, 2022

DanielNoord Dec 4, 2022

DanielNoord Dec 4, 2022

DanielNoord Dec 4, 2022

orSolocate Dec 4, 2022

DanielNoord Dec 4, 2022

orSolocate Dec 4, 2022

DanielNoord Dec 4, 2022

Pierre-Sassoulas left a comment

orSolocate commented Dec 4, 2022

orSolocate commented Dec 4, 2022

DanielNoord commented Dec 4, 2022

orSolocate commented Dec 4, 2022 •

edited

Loading

DanielNoord commented Dec 4, 2022

jacobtylerwalls commented Jan 16, 2023

jacobtylerwalls commented Apr 17, 2023

jacobtylerwalls commented Jul 3, 2023

		@@ -0,0 +1,3 @@
		Fix False Positive issue regarding a complex ``assignment-from-none``.

	function_nodes: Any \| None = safe_infer_multiple(node.value.func)
	function_nodes = safe_infer_multiple(node.value.func)

bugfix - assignment-from-none False Positive #7853 #7886

bugfix - assignment-from-none False Positive #7853 #7886

Conversation

orSolocate commented Dec 1, 2022

Type of Changes

Description

orSolocate commented Dec 1, 2022

Pierre-Sassoulas left a comment • edited Loading

Choose a reason for hiding this comment

orSolocate commented Dec 3, 2022

github-actions bot commented Dec 3, 2022

orSolocate commented Dec 3, 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

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

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

orSolocate commented Dec 4, 2022

orSolocate commented Dec 4, 2022

DanielNoord commented Dec 4, 2022

orSolocate commented Dec 4, 2022 • edited Loading

DanielNoord commented Dec 4, 2022

jacobtylerwalls commented Jan 16, 2023

jacobtylerwalls commented Apr 17, 2023

jacobtylerwalls commented Jul 3, 2023

bugfix - `assignment-from-none` False Positive #7853 #7886

bugfix - `assignment-from-none` False Positive #7853 #7886

Pierre-Sassoulas left a comment •

edited

Loading

orSolocate commented Dec 3, 2022 •

edited

Loading

orSolocate commented Dec 4, 2022 •

edited

Loading