-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Add name bindings for bad imports #31338
Add name bindings for bad imports #31338
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
r? @nrc I have a feeling you'll have a much better idea what's going on here than I |
lgtm, what is still left to do? Adding a test or two seems like the only obvious thing to me. |
@nrc: I'll definitely add some tests! 😉 Overall, though, do you think I took the right approach with adding the additional fields to |
Yeah, I think it is a fine approach. I'm slightly surprised there isn't an error def in there somewhere. In particular if a name ends up mapping to one of these dummy imports, how do we prevent type checking issuing further errors about it? |
I was surprised about that myself. It may turn out we need to make one. I'll write up tests for this tonight and we can see how those turn out.
I'm still getting familiar with the compiler internals so this may be completely wrong, but doesn't the compiler exit after reporting all the name/import/etc. resolution errors and not progress on to the type-checking stage? |
It used to, but I've been working on this recently and now will get all the way to type checking before bailing out most of the time. |
@nrc: As of 250ffcb it actually works! 🎉
Now just to write some proper test(s) for it. |
88ef9fc
to
ab0b930
Compare
@nrc: Tests are updated for the new behavior; I think this is ready for your review. |
c5658d1
to
e7f0c95
Compare
@@ -248,15 +293,15 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { | |||
let import_index = module.resolved_import_count.get(); | |||
match self.resolve_import_for_module(module, &imports[import_index]) { | |||
ResolveResult::Failed(err) => { | |||
let import_directive = &imports[import_index]; | |||
let ref import_directive = imports[import_index]; |
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.
nit: the previous style is more idiomatic
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.
@nrc: Fixed! 😄
e7f0c95
to
18e0d1a
Compare
@@ -200,11 +201,55 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { | |||
} | |||
} | |||
|
|||
/// Resolves an `ImportResolvingError` into the correct enum discriminant | |||
/// and passes that on to `resolve_error`. | |||
fn resolve_import_resolving_error(&self, e: ImportResolvingError) { |
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.
s/resolve_import_resolving_error/import_resolving_error
LGTM. Could you fix the remaining nits and squash the commits please? |
18e0d1a
to
026bcbf
Compare
@nrc: Fixed and squashed! |
Thanks! @bors: r+ |
📌 Commit 026bcbf has been approved by |
…=nrc WIP implementation of #31209. The goal is to insert fake/dummy definitions for names that we failed to import so that later resolver stages won't complain about them.
WIP implementation of #31209.
The goal is to insert fake/dummy definitions for names that we failed to import so that later resolver stages won't complain about them.