Fix strong references to mutable objects in context.clone #927
Conversation
| @@ -1703,7 +1703,8 @@ def __init__(self): | |||
| """ | |||
| ast = extract_node(code, __name__) | |||
| expr = ast.func.expr | |||
| self.assertIs(next(expr.infer()), util.Uninferable) | |||
| with pytest.raises(exceptions.InferenceError): | |||
| next(expr.infer()) | |||
There was a problem hiding this comment.
This test has changed behaviour. I assume that it's okay for it to raise InferenceError instead of returning Uninferable as long as it still doesn't leak StopIteration
There was a problem hiding this comment.
@cdce8p what is your opinion?
There was a problem hiding this comment.
I'm not too familiar with that part of the code base. If the pylint tests continue to pass, I would guess it's fine.
There was a problem hiding this comment.
Potentially this could cause new crash if an inference error is raised in calling code ? I don't think pylint tests cover all possible edge case.
nelfin
force-pushed
the
fix/926-strong-mutable-ref-in-context-clone
branch
from
b09297c
to
573e018
Compare
|
Updated and rebased to appease code formatting overlords |
nelfin
force-pushed
the
fix/926-strong-mutable-ref-in-context-clone
branch
from
573e018
to
04822ab
Compare
There was a problem hiding this comment.
@nelfin it is a great improvment. In fact you succeeded where i failed with #884. Congrats!
I just left a bunch of comments.
I also tried the tests of pylint with your astroid branch and one test is failing. It deals with logging module and it is possible that the warnings that are now emitted are correct. Could you please have a look to it? @Pierre-Sassoulas can you also have a look? I saw you just reintroduced the concerned lines, so you will be much pertinent than me.
| @@ -1703,7 +1703,8 @@ def __init__(self): | |||
| """ | |||
| ast = extract_node(code, __name__) | |||
| expr = ast.func.expr | |||
| self.assertIs(next(expr.infer()), util.Uninferable) | |||
| with pytest.raises(exceptions.InferenceError): | |||
| next(expr.infer()) | |||
nelfin
force-pushed
the
fix/926-strong-mutable-ref-in-context-clone
branch
from
04822ab
to
c30fd42
Compare
I think the new messages are okay? There wasn't any warning previously raised for the following: class Logger(renamed_logging.Logger):
pass
custom_logger = Logger('three')
custom_logger.info('testing {0}'.format('info')) # here
custom_logger.info('testing %s' % 'info') # and herebut those newly raised warning match the behaviour you would expect if |
|
@nelfin i will wait for @Pierre-Sassoulas remarks concerning the pylint test before merging this. Thanks again. |
No problem.
Thank you! |
|
@hippo91 I just looked at pylint-dev/pylint#4325 and also ran the other functional tests. The error with the logging tests is indeed a false-negative currently. Since all seems to work as expected then, I would say this is good to go. |
There was a problem hiding this comment.
@Pierre-Sassoulas mentioned here that the change from Uninferable to StopIteration might have unintended consequences. After I saw it, I ran the changes against one of my repos and indeed saw multiple errors. Namely: not-callable, unsupported-membership-test, and invalid-sequence-index. I haven't investigated what code exactly is causing these, but it's safe to assume this shouldn't be merged just yet.
The Github Actions run: https://github.com/cdce8p/ha-core/runs/2308362444?check_suite_focus=true#step:7:58
|
I had some time to look at two of them and reduced it down to code snippets. Maybe that helps. unsupported-membership-test # pylint: disable=missing-docstring,too-few-public-methods,invalid-name
from enum import Enum
class MyEnum(Enum):
CONST1 = "const"
def name_in_enum(name):
# false-positive: unsupported-membership-test
if name in MyEnum.__members__:
returnnot-callable pip install aiohttp python-telnet-vlc# pylint: disable=missing-docstring,unused-import,protected-access
from aiohttp import web
from python_telnet_vlc import VLCTelnet
def func():
app = web.Application()
app._router.freeze = lambda: None
class VlcDevice:
def __init__(self):
self._vlc = VLCTelnet("VLC-TELNET", "pwd", "0000")
def media_previous_track(self):
# False-positive: not-callable
self._vlc.prev()
def media_next_track_abc(self):
# False-positive: not-callable
self._vlc.next() |
Thanks for that, it helps a lot. I spent some time trying to replicate these messages from your bulid log and debugging the causes. I picked up these errors from the log in the link: The The The class DynamicGetitem:
def __getitem__(self, key):
if key == 'attributes':
return []
return {'world': 123}
ex = DynamicGetitem()
ex['hello']['world']
# E1126: Sequence index is not an int, slice, or instance with __index__ (invalid-sequence-index) |
This wasn't quite correct. So it looks like this has uncovered an existing bug: before the patch the value of # enum_repro.py
class BaseMeta(type):
def __new__(metacls, cls, bases, classdict):
classdict['__members__'] = metacls.__members__
return super().__new__(metacls, cls, bases, classdict)
@property
def __members__(cls):
return {'hello', 'world'}
class Parent(metaclass=BaseMeta):
pass
class Derived(Parent):
pass
assert 'hello' in Parent.__members__
assert 'world' in Derived.__members__And example output from astroid: # enum_repro.py
import astroid
mod = astroid.MANAGER.ast_from_file('enum_repro_mod.py', 'mod', source=True)
Parent, Derived = mod.body[1], mod.body[2]
print(list(Parent.igetattr('__members__')))
# [<Set.set l.10 at 0x...>]
print(list(Derived.igetattr('__members__')))
# [<Property.__members__ l.8 at 0x...>] |
Ref pylint-dev#927. `metaclass` tests seem related, `property` tests possibly related. pylint-dev#927 (comment)
|
@cdce8p: class Example:
def prev(self):
pass
def next(self):
pass
def other(self):
pass
ex = Example()
ex.other() # no warning
ex.prev() # no warning
ex.next() # no warning
import typing
ex.other() # no warning
ex.prev() # false-positive: not-callable
ex.next() # false-positive: not-callable |
Ref pylint-dev#926. Since InferenceContext.clone passes self.path when constructing a new instance, these two objects will hold references to the same underlying set object. This caused modifications to context.path further down in the stack to be reflected in "earlier" objects and this would incorrectly detect a loop if the same node was used twice in the inference of a some node (instead of only if an inference result was used in inferring itself). As an illustration: [stmt1 = something.one, stmt1 = something.two] -> infer stmt1 <-+ -> -> infer something -+ writes back to parent context.path -+ -> <- <returns> | -> infer stmt2 affecting this context.path <-+ -> -> infer something -> sees something in context.path, decorators.path_wrapper returns None
Now raises InferenceError instead of returning Uninferable. Still no StopIteration leak.
This reverts commit b699ebb. The fixes to context.path also appears to stop returning Uninferable for numpy ufuncs.
nelfin
force-pushed
the
fix/926-strong-mutable-ref-in-context-clone
branch
from
c30fd42
to
0e5cd49
Compare
for more information, see https://pre-commit.ci
Steps
Description
Since
InferenceContext.clonepassesself.pathwhenconstructing a new instance, these two objects will hold references to
the same underlying set object. This caused modifications to
context.pathfurther down in the stack to be reflected in "earlier"objects and this would incorrectly detect a loop if the same node was
used twice in the inference of a some node (instead of only if an
inference result was used in inferring itself).
Type of Changes
Related Issue
Closes #926