-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Show the used type variable when issuing a "can't use type parameters from outer function" error message #47574
Conversation
@estebank I've got a compile error with my current edits that I don't understand:
Do you have an idea of where that could come from? How could I gather more informations to debug this kind of error, because the error seems very far from the edits that I did? |
Alright some more details: the compilation seems to fail when I copy the span argument of the With: #[derive(Clone, Copy, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub struct ParamTy {
pub idx: u32,
pub name: Name,
pub span: Span,
} This works: pub fn new(index: u32, name: Name, _span: Span) -> ParamTy {
ParamTy { idx: index, name: name, span: DUMMY_SP }
} This doesn't (compilation error: #47574 (comment)): pub fn new(index: u32, name: Name, span: Span) -> ParamTy {
ParamTy { idx: index, name: name, span: span }
} |
You're indeed right that it doesn't seem to be caused by your code, but somehow triggered by the change. It is indeed quite weird. The code you've written looks correct to me. Can you provide the full compiler output? I'm intrigued as to what stage this is happening in. |
It's failing when compiling stage 1 std artifacts. Build log:
|
Thinking about this a bit more, it makes sense that this was introduced by your change. I have to dig a bit deeper to check where, but I'm guessing there's some equality check or similar code somewhere in |
Review ping for you @estebank! Have you checked why the Span field caused the conflicting impl error? |
src/librustc_resolve/lib.rs
Outdated
resolve_error(self, span, | ||
ResolutionError::TypeParametersFromOuterFunction); | ||
ResolutionError::TypeParametersFromOuterFunction(&span)); |
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.
BTW, this line is >100 chars, failing the CI.
[00:06:14] tidy error: /checkout/src/librustc_resolve/lib.rs:3200: line longer than 100 chars
[00:06:16] some tidy checks failed
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.
Thanks! I'll update that before finalizing the PR :) (and run tidy again to check for other errors)
I'm stumped, I haven't figured out why this is happening, although the only working theory I have at the moment is around @rust-lang/compiler, apologies for the wide shout out but can someone help @zilbuz with this issue? This is also blocking @gaurikholkar on a PR that also requires adding spans to |
I'll try to take a look. Assigning review to myself so I remember. Does seem likely to be an unrelated bug, but I've not reviewed the diffs yet. |
Hmm, I wonder if it has to do w/ incremental compilation somehow. |
src/librustc/ty/sty.rs
Outdated
@@ -862,23 +863,24 @@ impl<'tcx> PolyFnSig<'tcx> { | |||
pub struct ParamTy { | |||
pub idx: u32, | |||
pub name: Name, | |||
pub span: Span, |
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.
Looking more closely at this diff, I'm not sure how I feel about adding a span
to ty::ParamTy
in any case. Actually, I would prefer to remove name
too, and just have idx
. I can in any case imagine that adding span information in here might be messing with the equality and hash tests and causing problems with trait selection, though I don't exactly now why that would be true off-hand -- I guess it should be ok as long as we are consistent about what span we use.
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.
We should just add a DefId
if we want to get back the definition, Span
is a very bad idea within anything reachable from Ty
.
(e.g. cross-crate Span
is not that cheap to decode and behaves a bit strangely)
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, definitely not a span. I have been resisting the idea of going back to DefId
, because I'd rather we move towards just an index, but that would require having more context about the currently "in scope" parameters somewhere to give good error messages. Maybe a DefId
is the right step for now.
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 we use DefId
in Region
, don't we?
☔ The latest upstream changes (presumably #47748) made this pull request unmergeable. Please resolve the merge conflicts. |
For information @nikomatsakis, I tried to compile without incremental compilation, but I have the same compilation error. I'll try to make a summary of the situation. ObjectiveThe goal of this PR is to clarify the error message for E0401. I have two examples that trigger this error. First examplepub trait Bar { fn m(&self); }
pub fn foo<T>() {
struct S<T>;
impl Bar for S<T> {
fn m(&self) { println!("Hello World"); }
}
let s = S::<T>;
s.m();
}
pub fn main() {
} Current error message:
Ideal error message:
Second examplestruct A<T> {
inner: T,
}
impl<T> Iterator for A<T> {
type Item = u8;
fn next(&mut self) -> Option<u8> {
fn helper(sel: &Self) -> u8 {
unimplemented!();
}
Some(helper(self))
}
}
fn main() {
} Current error message:
I don't really know what the ideal error message is here. The ImplementationStep one: adjust
|
@nikomatsakis, ping from triage (also pinged on IRC)! |
@zilbuz sorry for not responding earlier. I keep diving into your list and then getting a bit lost myself. =) |
The last time, I got into a discussion with @eddyb about how we really want to be handling the |
@nikomatsakis what's the status on this? |
Sorry for the delay. So, I've finally taken some time to catch up on what the original goal was here. The good news is that I think there is really no need to be changing the I'll try to leave some comments in the code itself, but also, a quick note on your second example:
for this, may I suggest the following error?
The truth is that it is not a type parameter in the context of an impl, not really, more like a type alias, but that may not be worth clarifying. |
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.
OK, I left some comments, hopefully that helps!
src/librustc_resolve/lib.rs
Outdated
resolve_error(self, span, | ||
ResolutionError::TypeParametersFromOuterFunction); | ||
ResolutionError::TypeParametersFromOuterFunction(None)); |
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.
OK, so, here def.def_id()
gives you the DefId
of the thing we want -- annoyingly, I'm not sure how to get the span from that. Normally I'd say to use the hir-map, but it's not available in resolve. Perhaps @eddyb or @petrochenkov can answer that -- what is the best way to get the span where def
was declared here?
src/librustc_resolve/lib.rs
Outdated
@@ -3275,8 +3278,10 @@ impl<'a> Resolver<'a> { | |||
// This was an attempt to use a type parameter outside | |||
// its scope. | |||
if record_used { | |||
debug!("rib: {:?}", rib); | |||
resolve_error(self, span, |
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 we can kill all the diffs that are not part of this file.
src/librustc_resolve/lib.rs
Outdated
@@ -122,7 +122,7 @@ impl Ord for BindingError { | |||
|
|||
enum ResolutionError<'a> { | |||
/// error E0401: can't use type parameters from outer function | |||
TypeParametersFromOuterFunction, | |||
TypeParametersFromOuterFunction(Option<&'a Span>), |
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.
May I suggest, instead of passing the span, that you pass the Def
? (See other comments)
src/librustc_resolve/lib.rs
Outdated
@@ -172,13 +172,16 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver, | |||
resolution_error: ResolutionError<'a>) | |||
-> DiagnosticBuilder<'sess> { | |||
match resolution_error { | |||
ResolutionError::TypeParametersFromOuterFunction => { | |||
ResolutionError::TypeParametersFromOuterFunction(outer_span) => { |
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.
if we have the full Def
here, we can match on it and see whether it is Def::SelfType
or some other kind of def. If it is a self-type, we can customize the error message to highlight the trait/impl. The tricky bit is still getting the span from a Def
.
💔 Test failed - status-travis |
@bors retry
|
⌛ Testing commit 0e68bb9 with merge ac2cc996bd392f81fecba27f69f5588778c956d7... |
💔 Test failed - status-travis |
@bors retry
|
☀️ Test successful - status-appveyor, status-travis |
Some tweaks to "type parameters from outer function" diagnostic Follow up to rust-lang#47574.
Fix #14844
r? @estebank