-
-
Notifications
You must be signed in to change notification settings - Fork 306
Support isinstance
constraints in inference
#2846
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2846 +/- ##
==========================================
+ Coverage 93.35% 93.37% +0.02%
==========================================
Files 92 92
Lines 11190 11212 +22
==========================================
+ Hits 10446 10469 +23
+ Misses 744 743 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
π New features to boost your workflow:
|
Seems like test coverage is lacking. Shouldn't be an issue to get it up to 100%, but I would like a review first to see if this implementation makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful!
As we do more of these, it would be nice to add a CI step that installs pylint and runs the primer. I had an experiment (work in progress) at jacobtylerwalls#157 and
pylint-dev/pylint@main...jacobtylerwalls:pylint:run-with-custom-astroid. Something for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobtylerwalls Should this be merged for 4.0.0 considering we already released a RC? Feels like a big change to include after a RC.
I wondered as well, happy to wait for 4.1. Let's release 4.0 final tomorrow in that case. |
Let's do so, especially as we haven't tested this with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat ! Glad to see that the constraint api is getting some love.
b3b9dd4
e5217a2
to
b3b9dd4
Compare
except StopIteration as e: | ||
raise InferenceError(node=node, context=context) from e | ||
# arg2 MUST be a type or a TUPLE of types | ||
# for isinstance | ||
if isinstance(node_infer, nodes.Tuple): | ||
try: | ||
class_container = [ | ||
next(node.infer(context=context)) for node in node_infer.elts | ||
] | ||
except StopIteration as e: | ||
raise InferenceError(node=node, context=context) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StopIteration
handlers are not covered by tests, wondering if it is reasonable to exclude them. From what I understand .infer()
already throws InferenceError
and we won't get to these blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible for node.infer(context=context)
to be inferable but an empty iterable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was what I thought as well, but when we get an empty iterable, this decorator catches StopIteration
and raises InferenceError
? Not sure if there are cases where this decorator is not used.
Lines 75 to 96 in ab119c2
def raise_if_nothing_inferred( | |
func: Callable[_P, Generator[InferenceResult]], | |
) -> Callable[_P, Generator[InferenceResult]]: | |
def inner(*args: _P.args, **kwargs: _P.kwargs) -> Generator[InferenceResult]: | |
generator = func(*args, **kwargs) | |
try: | |
yield next(generator) | |
except StopIteration as error: | |
# generator is empty | |
if error.args: | |
raise InferenceError(**error.args[0]) from error | |
raise InferenceError( | |
"StopIteration raised without any error information." | |
) from error | |
except RecursionError as error: | |
raise InferenceError( | |
f"RecursionError raised with limit {sys.getrecursionlimit()}." | |
) from error | |
yield from generator | |
return inner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there are cases where this decorator is not used.
That worries me also (plugins?)
A pragma to exclude coverage seems reasonable.
matches_checked_types = helpers.object_isinstance(inferred, types) | ||
|
||
if matches_checked_types is util.Uninferable: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Uninferable
check is not covered by tests, it should be unreachable in runtime. I am including it as a safe guard because helpers.object_isinstance()
returns bool | Uninferable
, but I am also fine with removing it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share more details about why this is unreachable? I see that we short-circuit if inferred is uninferable, but it wasn't apparent to me why inferring again wouldn't return uninferable.
b3b9dd4
to
010503c
Compare
Type of Changes
Description
These changes implements the constraint interface to add a new constraint -
TypeConstraint
to improve the accuracy of inference for objects used in type-narrowing guards.Currently, the implementation only supports the
isinstance
pattern, but it should be relatively straightforward to extend it to support the negated case (not isinstance
) in the future.To determine whether an object is an instance of the given types, the implementation reuses existing inference mechanism for
isinstance
inbrains
.Refs pylint-dev/pylint#1162
Refs pylint-dev/pylint#4635
Refs pylint-dev/pylint#10469