Create ResolvePath abstraction in librustc_resolve #50315
Closed
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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (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. |
rust-highfive
added
the
S-waiting-on-review
|
I don't think one-off abstractions like this bring more improvement than harm and would personally close #50258 as not an issue. |
|
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 |
|
It could be nice to rename |
Manishearth
requested changes
Apr 29, 2018
src/librustc_resolve/lib.rs
Outdated
| @@ -84,6 +84,13 @@ mod check_unused; | |||
| mod build_reduced_graph; | |||
| mod resolve_imports; | |||
|
|
|||
| pub struct ResolvePath<'a> | |||
| { | |||
There was a problem hiding this comment.
nit: standard style is the brace is on the same line
There was a problem hiding this comment.
Also no space before the colon
src/librustc_resolve/lib.rs
Outdated
| @@ -1654,12 +1661,18 @@ impl<'a> Resolver<'a> { | |||
| let path: Vec<Ident> = segments.iter() | |||
| .map(|seg| Ident::new(seg.name, span)) | |||
| .collect(); | |||
|
|
|||
| let resolve_path = ResolvePath { | |||
There was a problem hiding this comment.
nit: standard style is
let resolve_path = ResolvePath {
ident: &path,
source: None,
speculative: true,
};
(run against rustfmt to see)
There was a problem hiding this comment.
also, i think it's fine if we just call this variable path everywhere, the intent is to reduce paths that we pass around as slices
src/librustc_resolve/lib.rs
Outdated
| { | ||
| ident : &'a [Ident], | ||
| source : Option<NodeId>, // None if this path is speculative | ||
| speculative : bool, |
There was a problem hiding this comment.
There currently isn't a case where this is true and source is None, let's remove this field. Instead, document the source as
"NodeId of the path that we are attempting to resolve. When None, this path is being speculatively resolved and we should not emit errors or lints about it."
src/librustc_resolve/macros.rs
Outdated
| } else { | ||
| Ok(def) | ||
| let def = { | ||
| let resolve_path = ResolvePath { |
There was a problem hiding this comment.
this should be constructed immediately after we construct path
There was a problem hiding this comment.
The problem I had with this is, written like this, we need the borrow to end on the Vec so we can do push((path.into_boxed_slice(), span)) later on in the function. I couldn't find a way to turn a &[T] directly into a Box<[T]>, is there a way to do that?
|
Yes, the intent is to roll record_used into this as well, as we use it more pervasively. Another thing I'd like to pass down here is whether or not the path started with I think this is somewhat useful: We pass around paths a bunch and strip them of most of their info, but we need to persist a couple things and it's getting unweildy. @eddyb what do you think? |
eddyb
reviewed
Apr 30, 2018
src/librustc_resolve/lib.rs
Outdated
| @@ -84,6 +84,13 @@ mod check_unused; | |||
| mod build_reduced_graph; | |||
| mod resolve_imports; | |||
|
|
|||
| pub struct ResolvePath<'a> { | |||
| ident: &'a [Ident], | |||
|
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 |
|
Thank you for your reviews so far @Manishearth and @eddyb; I think I've fixed everything mentioned apart from one: let segments: Vec<_> = segments.iter().map(|seg| seg.ident).collect();
let path = ResolvePath {
segments: &segments,
source: None,
};However, before we were then turning the vector (which is now borrowed in self.current_module.nearest_item_scope().macro_resolutions.borrow_mut()
.push((segments.into_boxed_slice(), span));Afaik, there's no way to turn the |
|
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, @Manishearth ! You have some pending questions. |
|
@IsaacWoods That's fine. @eddyb do we want to land this? |
|
@Manishearth I defer any decisions on this PR to @petrochenkov and/or @jseyfried. |
|
Ping from triage @petrochenkov @jseyfried! This PR needs your decision. |
|
@bors r+ |
|
📌 Commit bfd2a75 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit bfd2a75 with merge 15d308e83cab8a2fe602e872f220b8f5c472a669... |
|
💔 Test failed - status-travis |
bors
added
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 |
1 similar comment
|
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 |
petrochenkov
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 |
|
This now compiles locally; Travis looks unrelated? Should I squash the commits? |
|
Yeah, squash. Those errors do look like they could possibly be the result of a broken resolution, though. |
| let path = ResolvePath { | ||
| segments: &path, | ||
| source: None, | ||
| }; |
There was a problem hiding this comment.
Nit: could you use one-line formatting for all these ResolvePath { ... } constructors?
|
☔ The latest upstream changes (presumably #50665) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage @IsaacWoods! It's been a while since we heard from you, will you have time to work on this again soon? |
|
Still not sure what I've broken, and very busy with school atm, so I'll try to get some time in but can't promise anything. Sorry :/ |
|
Don't worry if you don't have time right now, just wanted to check in to make sure the PR is still on your radar As far as the broken things, maybe @Manishearth or @petrochenkov can give you some hints on how to debug them? |
Manishearth
reviewed
May 21, 2018
| )); | ||
| } | ||
|
|
||
| let result = match self.resolve_path(&path, Some(ns), true, span, Some(id)) { | ||
| let result = match self.resolve_path(&path, Some(ns), true, span) { |
There was a problem hiding this comment.
the path here is incorrect and probably is the cause of the bug
There was a problem hiding this comment.
Because the source is incorrect? Redefining the path aslet path = ResolvePath { segments: path.segments, source: Some(id) }; before that line has no effect, however.
There was a problem hiding this comment.
oh sorry, I didn't notice the scopes here
|
This is your weekly triage ping to make sure you are still aware of this! Still planning on getting back to this? |
|
Thank you for this PR @IsaacWoods! Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! |
pietroalbini
added
S-inactive
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.
Fixes #50258
This creates a struct called
ResolvePathto replace the use of&[Ident]inlibrustc_resolve.However, this currently doesn't build because we don't use the
speculativeflag. The only place it could be used atm (as far as I can see), is instead now written like this (librustc_resolve::3370):I can make the change to testing
speculativeand then unwrapping it if that's better?Overall, I don't know if this is what was intended from the issue, but I thought I'd give it a shot.
r? @Manishearth