Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[red-knot] Improve the
unresolved-import
check #13007[red-knot] Improve the
unresolved-import
check #13007Changes from all commits
2efa423
29ab98d
2c1fb1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Here I think describing it as a failed attribute access would actually be much clearer than the vague "Could not resolve" which doesn't really communicate anything! This message also doesn't currently clarify that
'a'
is a module (though it could be taken as implied.) I actually can't think of an error message here that I think would be better than "Modulea
has no attributething
."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.
Do people really think of
from collections import deque
as an "attribute access" ofdeque
from thecollections
module? I certainly don't. Of course that's what's happening under the hood, but even at runtime this detail is hidden -- the import fails withImportError
rather thanAttributeError
, and there's no mention of attempting to access an attribute: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.
I think when we issue the error on the import in
a.py
, we should set the type offoo
ina
toUnknown
, notUnbound
, which should avoid this?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.
Why not just emit the diagnostics inside this function, return an
Option
, and avoid the need forModuleResolutionError
?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.
I still find it odd that we have to do the "unbound" check everywhere where we call
.member
. Do we not always want to emit a diagnostic when failing to resolve a member?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.
We do, but it will need to be different error codes. It would be very counterintuitive for users if
import foo
triggers anunresolved-import
error code andfrom foo import bar
triggers anunresolved-member
error code. Users will expect all import-related failures to be the same error code and all attribute-access-related failures to be a different error code.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 makes sense. We might just want to parameterize the
member
method. But maybe that's not worth it? I don't know. Have you looked at how mypy/pyright do this?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 exactly what you mean; could you clarify?
Mypy has a very different architecture to the one we're building. It resolves all imports eagerly in a first pass before it does any other type checking;
module-not-found
errors are emitted during this first pass here: https://github.com/python/mypy/blob/fe15ee69b9225f808f8ed735671b73c31ae1bed8/mypy/build.py#L2600-L2700.I'm not as familiar with pyright's codebase but I can dig in now
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.
Hmm... it seems both mypy and pyright in fact do what you'd prefer here -- they both use "attribute-access" error codes for
from x import y
imports:Pyright's error code is
reportAttributeAccessIssue
; mypy's isattr-defined
, which both seem equally bad. In terms of error messages, pyright definitely wins, though: pyright has:whereas mypy has
I'm somewhat surprised by this. But given this precedent, I'm okay with emitting the diagnostic from
.member()
, if you'd prefer. Though I hope we still aspire to be more user-friendly in our error messages than either mypy or pyright.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.
Oh, I agree that that makes a lot of sense! I can make that change
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.
Hmmm... if the inferred type of a member access is a union, e.g.
int | Unbound
, would you expect that to be represented as anOk()
or anErr()
variant (if we return anResult
from.member()
?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 might be doable, but after looking at it for a little bit I think it would still be quite a large refactor and design change. I think it's out of the scope of this PR.
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.
I'm not convinced there is any problem with reporting an error on
from foo import bar
(where the modulefoo
does exist, but has no top-level symbolbar
) as an attribute-access error code. If I had to pick between the pyright or mypy error messages above, I would prefer the mypy one (the pyright one gives less useful context). The fact that the attribute access occurred as part of a from-import will be clear from the error location; it doesn't have to be part of the error message or code. I wouldn't want to make design decisions based on the hypothesis that this is confusing to users without clear evidence that it is (e.g. a history of users filing issues on mypy about that error code or message); personally I find it intuitive.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.
I don't think we should return an
Option
orResult
frommember()
, for the reason @AlexWaygood mentions above: we can have a possibly-unbound name (currently represented as a union with Unbound), and it's not clear how that should look in a binary representation.We could design our own representation for "Type plus definitely-bound/maybe-unbound/definitely-unbound state" and use that instead of
Type::Unbound
. This will complicate some APIs and I think be somewhat less efficient, but it would have the advantage that it would force handling Unbound early, which I think is desirable. In general I don't think we want Unbound escaping from the scope where it originated; it should instead result in a diagnostic and be eliminated from the type (replaced withUnknown
if we have no other type information; probably just disappear from the type if we do.) I do think this alternative toType::Unbound
is worth exploring and seeing what kind of impact it will have.I was thinking that we would push diagnostics inside
member()
to simplify the handling of Unbound, and I think I still favor that approach. If we do want to make distinctions in error codes/messages based on context from the caller ofmember()
, I think we can always pass in context to make that possible.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.
I don't think we should be emitting a diagnostic at all if the imported symbol is of type
Unknown
.Unknown
might result from an error in the other module, but in that case the diagnostic should occur in the other module; importing a value of unknown type is not an error.I think just removing this clause would fix the ignored test; what would it break?
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.
Yeah, the only reason we have to check for Unknown here is that we intentionally replaced Unbound with Unknown just a few lines above. If we wait to do it after, the bug in the ignored test goes away, and all tests pass:
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.
Thanks, I think you're right that this fixes the tests I added (and skipped) in this PR. Here's an interesting edge case that you might not have considered, though. What should we do for something like this?
Running
python bar.py
fails withImportError
at runtime becausex
is not defined infoo.py
(due to the iterable being empty), but neither mypy nor pyright catches this. Arguably we should infer the type ofx
after thefor
loop has completed as beingint | Unbound
-- should we flag any union that includesUnbound
as one of the elements with anunresolved-import
diagnostic? Probably not, I don't think -- the import might succeed, it just also might not. So let's say (for argument's sake) we use the error codeimport-possibly-unbound
for that.So then what if we have something like this, where the symbol definitely exists in
foo.py
, but is also definitely unbound? (Mypy and pyright do both catch this one.) Should that have the error codeunresolved-import
, orimport-possibly-unbound
?These are somewhat obscure edge cases, so I think what you propose here is a strict improvement on what I implemented in this PR; I'll make a fixup PR to address your suggestions. But my point is that we may still at some point need some context propagated regarding the cause of the error -- whether that error is represented as
Unknown
orUnbound
.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.
Yeah, great points and great questions! I had the
x: int
case in mind last night when thinking again about representation of unbound-ness in a different comment on this PR.I agree that maybe-unbound is a different error class than definitely-unbound (and arguable whether maybe-unbound should trigger an error on import at all.)
I think we do want a representation of "definitely unbound and typed" that we use for imports, so e.g. we can error if you import from
x: int
(as long as it's not a stub), but we can still type check the importing module treatingx
asint
rather thanUnknown
. (Not sure how important this is, but it seems marginally better.) I think this can be handled by declared vs inferred types.What's less clear to me is if we need a representation of "definitely unbound and typed" for inferred types. It would mean that within a scope we could also check code following
x: int
and emit diagnostics both for "x is not bound" and for use of x that isn't valid for an int. Maybe that's useful?Like I mentioned above in a different comment, I'm pretty open to exploring other representations of unbound to make it orthogonal to types; the main question for me is if we can avoid it regressing in perf, and if not, do we gain enough from it to be worth the regression?
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.
I don't see the advantage of returning a
Result
vs just emitting the diagnostic directly in this method, as the previous code did.