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

#45829 when a renamed import conflict with a previous import #55113

Merged
merged 6 commits into from Oct 23, 2018

Conversation

Projects
None yet
5 participants
@mockersf
Contributor

mockersf commented Oct 16, 2018

Fix the suggestion when a renamed import conflict.

It check if the snipped contains " as ", and if so uses everything before for the suggestion.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 16, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@estebank

Thanks for the fix! I left a few comments inline.

Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
Show resolved Hide resolved src/test/ui/issues/issue-45829/rename-extern.stderr Outdated
Show resolved Hide resolved src/librustc_resolve/lib.rs
Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
suggested_name,
),
(Some(_), true) => format!("{} as {};",
&snippet[..snippet.find(" as ").unwrap()],

This comment has been minimized.

@estebank

estebank Oct 16, 2018

Contributor

Here (and above) this is not going to work if the snippet is "use\tfoo\tas\tbar;".


I'm looking at ImportDirectiveSubclass and what Ident already has a span field, so we can use target.span for the single use case. For extern crate, we should modify ImportDirectiveSubclass::ExternCrate(Option<Name>) to be ImportDirectiveSubclass::ExternCrate(Option<Ident>). By doing that you won't have to peek into the snippet to find the appropriate Span to use in the suggestion, nor peek into the span to be able to tell wether there's an as rename happening. Changing ExternCrate will have some knock down effects, but they should be minor.

subclass: ImportDirectiveSubclass::ExternCrate(orig_name),
subclass: ImportDirectiveSubclass::ExternCrate {
source: orig_name,
target: ident,

This comment has been minimized.

@mockersf

mockersf Oct 20, 2018

Contributor

source is the optional original name if the import is renamed, target is the name it's imported at (so the original name if there was no rename)

if let (
Ok(snippet),
NameBindingKind::Import { directive, ..},
_x @ 1 ... std::u32::MAX,

This comment has been minimized.

@mockersf

mockersf Oct 20, 2018

Contributor

this filter out implicit import, like std

&rename_msg,
match (&directive.subclass, snippet.ends_with(";"), snippet.as_ref()) {
(ImportDirectiveSubclass::SingleImport { .. }, false, "self") =>
format!("self as {}", suggested_name),

This comment has been minimized.

@mockersf

mockersf Oct 20, 2018

Contributor

imports with self are a simple case, as it's only possible in the form import foo::{ self }

help: You can use `as` to change the binding name of the import
|
LL | as other_std// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
| ^^^^^^^^^^^^

This comment has been minimized.

@mockersf

mockersf Oct 20, 2018

Contributor

this was actually another incorrect suggestion that is now not present

@estebank

Looking good!

A couple of nitpicks and then I can approve.

Great work on getting this done!

pub struct B;
}
use foo::{A, A};

This comment has been minimized.

@estebank

estebank Oct 21, 2018

Contributor

This is a good test to add. In this case it might be nice not to suggest renaming, as it is more likely a duplicated import, but adding that is something best left to a follow up PR. This would also ideally affect the case where we import both use foo and use foo::{ self }.

Show resolved Hide resolved src/librustc_resolve/lib.rs Outdated
Show resolved Hide resolved src/librustc_resolve/lib.rs

@zackmdavis zackmdavis removed their assignment Oct 21, 2018

if let (
Ok(snippet),
NameBindingKind::Import { directive, ..},
_x @ 1 ... std::u32::MAX,

This comment has been minimized.

@zackmdavis

zackmdavis Oct 21, 2018

Member

(..= is the new, syntax for inclusive ranges, preferred over ....)

if let (
Ok(snippet),
NameBindingKind::Import { directive, ..},
_dummy @ false,

This comment has been minimized.

@estebank

estebank Oct 22, 2018

Contributor

Side note: This could have been just false :)

@estebank

This comment has been minimized.

Contributor

estebank commented Oct 22, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Oct 22, 2018

📌 Commit 8fe6688 has been approved by estebank

@bors

This comment has been minimized.

Contributor

bors commented Oct 23, 2018

⌛️ Testing commit 8fe6688 with merge 7cfe5de...

bors added a commit that referenced this pull request Oct 23, 2018

Auto merge of #55113 - mockersf:master, r=estebank
#45829 when a renamed import conflict with a previous import

Fix the suggestion when a renamed import conflict.

It check if the snipped contains `" as "`, and if so uses everything before for the suggestion.
@bors

This comment has been minimized.

Contributor

bors commented Oct 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 7cfe5de to master...

@bors bors merged commit 8fe6688 into rust-lang:master Oct 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment