resolve suggestions should use crate:: when enabled
#51456
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Jun 11, 2018
src/librustc_resolve/lib.rs
Outdated
| @@ -4449,7 +4451,12 @@ fn show_candidates(err: &mut DiagnosticBuilder, | |||
| } else { | |||
| "\n" | |||
| }; | |||
| *candidate = format!("use {};\n{}", candidate, additional_newline); | |||
| let crate_prefix = if crate_in_paths { | |||
There was a problem hiding this comment.
Hmm, now i'm worried that this will apply even when it shouldn't. Consider this testcase:
fn main() {
let x: &Debug = &22;
}This currently prints:
error[E0412]: cannot find type `Debug` in this scope
--> src/main.rs:2:13
|
2 | let x: &Debug = &22;
| ^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
|
1 | use std::fmt::Debug;
|
and I think we do not want it to print use crate::std::fmt::Debug.
There was a problem hiding this comment.
It looks like these are produced by this function:
rust/src/librustc_resolve/lib.rs
Lines 4003 to 4015 in a32e979
I suspect we want to extend this struct
rust/src/librustc_resolve/lib.rs
Lines 87 to 90 in a32e979
with an additional field, indicating whether the first link in the path is an extern crate or not. Notice that the function has the ability to check whether it is an extern crate:
rust/src/librustc_resolve/lib.rs
Lines 4030 to 4031 in a32e979
In fact, we are already threading this in_module_is_extern state around -- that is almost what we want -- it tells us if the path contains an extern crate anywhere. But we just want to know if the path contains an extern crate in the first link.
Actually, reading this code, I think that likely these suggestions will break once we start removing extern crate entries. It may be that the better approach is to fix that problem, basically by iterating over the extern crates that were given on the command line first. The advantage of that is that we would not need any special logic for extern crate. So yeah scratch what I wrote above.
There was a problem hiding this comment.
I left some notes in the issue laying out a possible alternative plan.
kennytm
changed the title
create:: when enabledcrate:: when enabled
pietroalbini
added
S-waiting-on-author
|
Ping from triage @qmx! It's been a while since we heard from you, will you have time to work on this again? |
|
I think @qmx is still on this, just waiting for a bit of free time =) |
|
yep, I'm on it, just got overwhelmed with work :) |
TimNN
added
S-waiting-on-author
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ping from triage! @qmx we haven't heard from you for a while, will you have time to work on this PR? |
|
hey @stokhos - @nikomatsakis asked for more tests to be added before landing this change, and that's where I'm investing time for now |
|
Ping from triage @qmx! It's been a while since we heard from you, will you have time to work on this again? |
|
Here's the catch, it's been worked but there's no actionable "output" yet - I'm trying to figure out how to include suggestions from the prelude also (described with greater detail on the parent issue). Any suggestions on how to surface the work in progress that not necessarily materializes in commits? |
|
Well, don't worry about that! We ping to make sure a PR is not forgotten (and close it if it's inactive), but if the author replies we do nothing. [note for triage: maybe revisit this in two weeks instead of one?] |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Aug 25, 2018
src/librustc_resolve/lib.rs
Outdated
| path_segments.clone() | ||
| }; | ||
| let first_segment_ident = segms[0].ident; | ||
| if first_segment_ident.name != "std" { |
There was a problem hiding this comment.
So I was looking into this special-casing. I think the reason that I was confused is that I thought that if you had a path like use foo::bar, then we would only look for the crate foo amongst the extern-prelude crates. But from skimming the code I no longer quite think that is the case. I think we'll also search the -L directories for that crate -- and this (I believe) is exactly how use std::foo works at all. In that case, we can remove the special case here and make instead the change below.
Still, I somehow that wasn't the desired design... so cc @eddyb and @petrochenkov -- how do we get the full list of crates that can be used with an explicit extern crate?
There was a problem hiding this comment.
@eddyb, @petrochenkov -- for context, what we are trying to do is to search the "available" crates for completion, but avoid duplicates from where there is an extern crate...
src/librustc_resolve/lib.rs
Outdated
| let is_extern_crate_that_also_appears_in_prelude = | ||
| name_binding.is_extern_crate() && | ||
| self.session.rust_2018() && | ||
| self.extern_prelude.contains(&ident.name); |
There was a problem hiding this comment.
if my new understanding is correct, I think we can just remove this self.extern_prelude.contains(&ident.name) line and we don't need to special case std at all.
I missed this comment earlier. I'm a bit confused though -- in Rust 2018, at least, won't |
nikomatsakis
requested changes
Aug 27, 2018
|
|
||
| all: | ||
| $(RUSTC) ep-nested-lib.rs | ||
|
|
There was a problem hiding this comment.
dumb question but I forget -- in Makefiles -- what happens with a blank line like this? I think we should remove it :)
There was a problem hiding this comment.
afaik, nothing, and the other run-make tests do the same for separating the libs from the executables
| all: | ||
| $(RUSTC) ep-nested-lib.rs | ||
|
|
||
| $(RUSTC) use-suggestions.rs --edition=2018 --extern ep_nested_lib=$(TMPDIR)/libep_nested_lib.rlib 2>&1 | $(CGREP) "use ep_nested_lib::foo::bar::Baz" |
nikomatsakis
approved these changes
Aug 27, 2018
|
@bors r+ |
|
📌 Commit d9791d6 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 27, 2018
resolve suggestions should use `crate::` when enabled I couldn't find a way to add a specific assertion for the ui test, so the expected output is living under the `crates-in-path.stderr` ui test. - is this the right place for the test? fixes #51212
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I couldn't find a way to add a specific assertion for the ui test, so the expected output is living under the
crates-in-path.stderrui test.fixes #51212