Skip to content
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

Merged
merged 2 commits into from
Feb 3, 2016

Conversation

dirk
Copy link
Contributor

@dirk dirk commented Feb 1, 2016

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.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

r? @nrc

I have a feeling you'll have a much better idea what's going on here than I

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton Feb 1, 2016
@nrc
Copy link
Member

nrc commented Feb 1, 2016

lgtm, what is still left to do? Adding a test or two seems like the only obvious thing to me.

@dirk
Copy link
Contributor Author

dirk commented Feb 1, 2016

@nrc: I'll definitely add some tests! 😉 Overall, though, do you think I took the right approach with adding the additional fields to ImportResolvingError and injecting into the module at error-resolution time?

@nrc
Copy link
Member

nrc commented Feb 1, 2016

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?

@dirk
Copy link
Contributor Author

dirk commented Feb 1, 2016

I'm slightly surprised there isn't an error def in there somewhere.

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.

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'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?

@nrc
Copy link
Member

nrc commented Feb 1, 2016

doesn't the compiler exit after reporting all the name/import/etc. resolution errors

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.

@dirk
Copy link
Contributor Author

dirk commented Feb 2, 2016

@nrc: As of 250ffcb it actually works! 🎉

$ cat test.rs
use foo::bar;

fn main() {
    bar();
}

$ x86_64-apple-darwin/stage1/bin/rustc test.rs
test.rs:1:5: 1:8 error: unresolved import `foo::bar`. Maybe a missing `extern crate foo`? [E0432]
test.rs:1 use foo::bar;
              ^~~
test.rs:1:5: 1:8 help: run `rustc --explain E0432` to see a detailed explanation
error: aborting due to previous error

Now just to write some proper test(s) for it.

@dirk dirk force-pushed the dirk/add-name-bindings-for-bad-imports branch from 88ef9fc to ab0b930 Compare February 2, 2016 05:48
@dirk
Copy link
Contributor Author

dirk commented Feb 2, 2016

@nrc: Tests are updated for the new behavior; I think this is ready for your review.

@dirk dirk force-pushed the dirk/add-name-bindings-for-bad-imports branch 5 times, most recently from c5658d1 to e7f0c95 Compare February 2, 2016 18:48
@@ -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];
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nrc: Fixed! 😄

@dirk dirk force-pushed the dirk/add-name-bindings-for-bad-imports branch from e7f0c95 to 18e0d1a Compare February 3, 2016 04:35
@@ -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) {
Copy link
Member

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

@nrc
Copy link
Member

nrc commented Feb 3, 2016

LGTM. Could you fix the remaining nits and squash the commits please?

@dirk dirk force-pushed the dirk/add-name-bindings-for-bad-imports branch from 18e0d1a to 026bcbf Compare February 3, 2016 04:49
@dirk
Copy link
Contributor Author

dirk commented Feb 3, 2016

@nrc: Fixed and squashed!

@nrc
Copy link
Member

nrc commented Feb 3, 2016

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 3, 2016

📌 Commit 026bcbf has been approved by nrc

@bors
Copy link
Contributor

bors commented Feb 3, 2016

⌛ Testing commit 026bcbf with merge 8c77ffb...

bors added a commit that referenced this pull request Feb 3, 2016
…=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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants