Type _infer of Call, Import and ImportFrom
#1653
Conversation
| """infer an Import node: return the imported module/object""" | ||
| if not context: |
There was a problem hiding this comment.
This will be uncovered but I think it makes more sense to handle the context == None path than see why infer_import and infer_importfrom are never called without a context.
There was a problem hiding this comment.
why infer_import and infer_importfrom are never called without a context
Because they need the lookupname in case the name infers to a wildcard import (or an import with several names defined), I believe. This is one of the astroid design that I don't really like, the context is used to pass crucial informations in an error-prone manner.
| @@ -260,16 +262,22 @@ def infer_call(self, context=None): | |||
| yield from callee.infer_call_result(caller=self, context=callcontext) | |||
| except InferenceError: | |||
| continue | |||
There was a problem hiding this comment.
| continue | |
| yield util.Uninferable | |
| continue |
I'm wondering why we do not yield anything in case of inference error. We're yielding something if call is uninferable itself line 252/254.
There was a problem hiding this comment.
That's what the @decorators.raise_if_nothing_inferred decorator is for I think.
If there is no yielded value it will become Uninferable or raise InferenceError.
Changing this could break a lot of test: if one callee's result is inferable and one isn't currently we get one yielded value without ambiguity. If we would add Uninferable to this because the second callee's result is uninferable we introduce a lot of ambiguity.
I can think of definitions behind sys guards. We handle those pretty poorly so we might struggle to understand what a definition under such a guard returns.
Pull Request Test Coverage Report for Build 2551725132
💛 - Coveralls |
| if not context: | ||
| raise InferenceError(node=self, context=context) |
There was a problem hiding this comment.
Instead of failing out right, couldn't we just create a new InferenceContext and pass that along if for whatever reason none is provided?
context = context or InferenceContext()There was a problem hiding this comment.
That would be a change in behaviour right? As in, currently context == None would crash. Wouldn't it make more sense to do this in a follow up PR and keep this restricted to typing/fixing crashes?
There was a problem hiding this comment.
That would be a change in behaviour right? As in, currently
context == Nonewould crash. Wouldn't it make more sense to do this in a follow up PR and keep this restricted to typing/fixing crashes?
Let's do that here. It isn't used today anyway. If it was, we would need to catch the unexpected error somewhere.
The change is just to satisfy typing and provide a sensible default value.
Scratch that. I guess it's such a small change, we can also do it in a followup PR.
| @@ -260,16 +262,22 @@ def infer_call(self, context=None): | |||
| yield from callee.infer_call_result(caller=self, context=callcontext) | |||
There was a problem hiding this comment.
Can you address the type errors for L260 + L262 here as well? For the later, it's probably adding a type: ignore due to the hasattr call.
There was a problem hiding this comment.
I didn't want to deal with typing infer_call_result so I thought I would leave that to a future PR. Would that be okay with you?
| if not context: | ||
| raise InferenceError(node=self, context=context) |
There was a problem hiding this comment.
| if not context: | |
| raise InferenceError(node=self, context=context) | |
| context = context or InferenceContext() |
Steps
Description
Type of Changes
Related Issue