-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Refer to types using the local identifier #44642
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
c28a79a
to
154d971
Compare
☔ The latest upstream changes (presumably #44634) made this pull request unmergeable. Please resolve the merge conflicts. |
154d971
to
01cbfca
Compare
On type errors, refer to types using the locally available name when they have been imported into scope instead of the fully qualified path. ``` error[E0308]: mismatched types --> file.rs:7:24 | 7 | let y: Option<X> = Ok(()); | ^^^^^^ expected enum `std::option::Option`, found enum `std::result::Result` | = note: expected type `Option<X>` found type `Result<(), _>` = help: here are some functions which might fulfill your needs: - .err() - .unwrap_err() ```
01cbfca
to
6877688
Compare
@@ -1179,6 +1179,10 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
self.all_crate_nums(LOCAL_CRATE) | |||
} | |||
|
|||
pub fn cstore(&self) -> &CrateStore { |
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.
I think @alexcrichton removed this for a reason.
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.
I'm sure. This was a quick fix after rebasing against master to get the code as I had written it working. I fully intend to clean this up before attempting to merge this PR.
@@ -357,7 +359,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |||
// for imported and non-imported crates | |||
if exp_path == found_path | |||
|| exp_abs_path == found_abs_path { | |||
let crate_name = self.tcx.crate_name(did1.krate); | |||
let crate_name = self.tcx.cstore().crate_name_untracked(did1.krate); |
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.
Why can't you use crate_name
here?
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.
This is a spurious change that should not have happened. I introduced it after hastily rebasing to HEAD of master when the moving of cstore
from sess
to tcx
happened. I'll revert.
full_imports: FxHashMap<String, String>, | ||
} | ||
|
||
impl<'a, 'gcx, 'tcx> ImportVisitor<'a, 'gcx, 'tcx> { |
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.
This looks likely to be buggy - you are reimplementing half of name resolution. Could name resolution be made to supply a types_in_scope
map?
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.
cc @jseyfried - is there a way for resolution to be called after it is complete?
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.
I figured that was the case, but it was the closest I've been in any of my approaches to get this working. Once I saw that the suggestion machinery used a visitor it gave me enough context to get this. Having this also allowed me to identify the unit tests that would need to change. It is definitely less than ideal and fully expect there to be a way to do this using existing datastructures, but I feel it is much easier for me to point at this code and saw "this is what I want to do, is there a better way?". :)
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.
There's not a way to do name resolution after lowering to HIR (i.e. after we drop the Resolver
). A naive solution would be to keep the Resolver
for longer, but that would hog memory and isn't ideal since the Resolver has needless complexity used for import resolution and is based on the AST.
@nikomatsakis and I have discussed creating and saving some name resolution structures for this purpose before destroying the Resolver
, but we haven't made detailed designs.
struct ImportVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { | ||
tcx: TyCtxt<'a, 'gcx, 'tcx>, | ||
/// All the `Item`s from other scopes visible from this `TyCtxt`, as well as their local name. | ||
full_imports: FxHashMap<String, String>, |
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.
Why not an FxHashMap<DefId, String>
?
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.
This is an artifact from the exploratory way I wrote this code, as while working on this I didn't have access to DefId
s from other crates. Very sensible suggestion.
} | ||
} | ||
|
||
/// This struct knows how to print out a single type for human consumption, compare two types and |
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.
Can this replace rustc::util::ppaux
?
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.
That is my eventual objective. I have moved the type argument elision code away from TyCtxt
in order to isolate as much from it as possible in order to unify it with the code in ppaux
. Do you think I should strive to do it in this PR?
s.push_str(&self.ty_str(ty_and_mut.ty)); | ||
s | ||
} | ||
_ => format!("{}", ty), |
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.
You really want to replace the entirety of ppaux here. Otherwise e.g. everything within a trait object will go back to ugly.
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.
Yes, that is indeed true. Given how big this PR was getting I thought that it was a good idea to put it out there to get feedback before going further down this rabbit hole. That being said, this covered a big enough percentage of cases that allowed me to test this approach.
} | ||
} else { | ||
// Importing from a different crate. | ||
for child in self.tcx.cstore().item_children_untracked(def_id, |
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.
Ah so the "untracked" here means that it's not tracked by incremental compilation, so this is something that we're going to want to handle as this is all happening after the tcx is created. If you need help turning this into a query though just let me know!
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.
Could you point me in the right direction of a query being performed? I haven't worked in that part of the compiler yet and would appreciate having a thread to pull from to unravel that sweater.
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.
Sure! You can check out #41417 for some more info
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.
TL;DR: Some of the weird code is the result of hastily rebasing against master and pushing the code out without reworking it to fit the current state of the compiler, and some is the result of the exploratory way I was writing the code without going back to clean it up after the reasons for the current state were no longer there.
@@ -357,7 +359,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |||
// for imported and non-imported crates | |||
if exp_path == found_path | |||
|| exp_abs_path == found_abs_path { | |||
let crate_name = self.tcx.crate_name(did1.krate); | |||
let crate_name = self.tcx.cstore().crate_name_untracked(did1.krate); |
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.
This is a spurious change that should not have happened. I introduced it after hastily rebasing to HEAD of master when the moving of cstore
from sess
to tcx
happened. I'll revert.
struct ImportVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { | ||
tcx: TyCtxt<'a, 'gcx, 'tcx>, | ||
/// All the `Item`s from other scopes visible from this `TyCtxt`, as well as their local name. | ||
full_imports: FxHashMap<String, String>, |
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.
This is an artifact from the exploratory way I wrote this code, as while working on this I didn't have access to DefId
s from other crates. Very sensible suggestion.
} | ||
} | ||
|
||
/// This struct knows how to print out a single type for human consumption, compare two types and |
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.
That is my eventual objective. I have moved the type argument elision code away from TyCtxt
in order to isolate as much from it as possible in order to unify it with the code in ppaux
. Do you think I should strive to do it in this PR?
s.push_str(&self.ty_str(ty_and_mut.ty)); | ||
s | ||
} | ||
_ => format!("{}", ty), |
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.
Yes, that is indeed true. Given how big this PR was getting I thought that it was a good idea to put it out there to get feedback before going further down this rabbit hole. That being said, this covered a big enough percentage of cases that allowed me to test this approach.
@@ -1179,6 +1179,10 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
self.all_crate_nums(LOCAL_CRATE) | |||
} | |||
|
|||
pub fn cstore(&self) -> &CrateStore { |
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.
I'm sure. This was a quick fix after rebasing against master to get the code as I had written it working. I fully intend to clean this up before attempting to merge this PR.
--> $DIR/local-ident.rs:16:28 | ||
| | ||
16 | let y: Option<usize> = Ok(2); | ||
| ^^^^^ expected enum `std::option::Option`, found enum `std::result::Result` |
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.
This is an open question: Do we want to show the fully qualified path here? I don't think length is as much of an issue here as this label doesn't print the type arguments, keeping the line relatively short. Alternatively it might be jarring to have two different things being shown here...
| ^^^^^ expected enum `std::option::Option`, found enum `std::result::Result` | ||
| | ||
= note: expected type `Option<usize>` | ||
found type `Result<{integer}, _>` |
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.
Should we add a note
here stating Option: std::option::Option
and Result: std::result::Result
? This could become a very long list for cases like the one put forward in #40186.
} | ||
} else { | ||
// Importing from a different crate. | ||
for child in self.tcx.cstore().item_children_untracked(def_id, |
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.
Could you point me in the right direction of a query being performed? I haven't worked in that part of the compiler yet and would appreciate having a thread to pull from to unravel that sweater.
full_imports: FxHashMap<String, String>, | ||
} | ||
|
||
impl<'a, 'gcx, 'tcx> ImportVisitor<'a, 'gcx, 'tcx> { |
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.
I figured that was the case, but it was the closest I've been in any of my approaches to get this working. Once I saw that the suggestion machinery used a visitor it gave me enough context to get this. Having this also allowed me to identify the unit tests that would need to change. It is definitely less than ideal and fully expect there to be a way to do this using existing datastructures, but I feel it is much easier for me to point at this code and saw "this is what I want to do, is there a better way?". :)
What about shadowing? For example: struct Result;
fn main() {
let res: Result = Ok(());
} This PR produces the following note:
(the full path for |
ping @estebank just want to make sure this doesn't fall off your radar! |
Still waiting on resolution coordination with @jseyfried. |
Not clear what the status here is - is there an issue that triagers can visit to see progress of any blockers? Just wondering who's 'responsible' for pushing this along :) |
☔ The latest upstream changes (presumably #44614) made this pull request unmergeable. Please resolve the merge conflicts. |
@arielb1 / @jseyfried / @estebank - can anyone clarify what/who this PR is waiting on at the moment (aside from a rebase)? I'm not sure how to triage this at the moment as I don't know what's blocking or if there's an issue we can track! |
@aidanhs I need to rework the PR to use queries, but I haven't had time available to do so. If people prefer, I can close this PR and resubmit once I've reworked it. |
@estebank ok great, thanks for clarifying! If you reckon you might get to this in the next couple of weeks then I'd be fine with keeping it open - up to you. |
Here for triage again! I'm going to go ahead and close this now to help clear out the queue, but @estebank of course feel free to resubmit once it's reworked! |
On type errors, refer to types using the locally available name when
they have been imported into scope instead of the fully qualified path.
Re: #21934.