Skip to content
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

Add rustc_on_unimplemented for Index implementation on slice #31071

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #31062

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2016

is this error message testable?

@GuillaumeGomez
Copy link
Member Author

Oh right, I'll add test. Thanks for notifying me @oli-obk!

@GuillaumeGomez
Copy link
Member Author

It's getting longer and more difficult because the rustc_on_unimplemented flag needs to be improved in order to handle impl as well (so a trait has this flag and an implementation of this trait redefines it, the impl's one will be displayed instead).

@GuillaumeGomez GuillaumeGomez force-pushed the index_indication branch 3 times, most recently from a8d346c to 9cf0aea Compare January 24, 2016 20:03
@GuillaumeGomez
Copy link
Member Author

I'm facing an issue here. For example, for the following code:

#![feature(on_unimplemented)]

fn main() {
    let x : &[i32] = &[1, 2];
    x[1u32];
}

I cannot determine the impl trait used between:

  • Index<usize>
  • Index<RangeTo<usize>>
  • Index<RangeFrom<usize>>
  • Index<RangeFull>

How can I determine the good one?

cc @pnkfelix, @nikomatsakis, @arielb1, @eddyb

@GuillaumeGomez GuillaumeGomez force-pushed the index_indication branch 2 times, most recently from 218bf8b to b3900ff Compare January 25, 2016 00:08
@nikomatsakis
Copy link
Contributor

@GuillaumeGomez

It's getting longer and more difficult because the rustc_on_unimplemented flag needs to be improved in order to handle impl as well (so a trait has this flag and an implementation of this trait redefines it, the impl's one will be displayed instead).

Well, the whole point is that this message is displayed in cases where is no impl to apply, so that's a bit tricky. But that said the new obligation forest does, in principle, keep some backtraces that might give us the "most likely" impl that was supposed to apply, so I guess we could find the best message to use by walking the backtrace. I know @arielb1 was working on a PR to improve error message that would make use of the backtrace, not sure of the status of that work though -- seems like it would be a precursor that would do some of the required refactoring for you.

@GuillaumeGomez
Copy link
Member Author

@nikomatsakis: That's a great news! Now, just have to wait that this functionality lands.

@nikomatsakis
Copy link
Contributor

@GuillaumeGomez for the record, the issue I was referring to is #30976

@GuillaumeGomez
Copy link
Member Author

Still waiting to see how it'll be solved. Then I'll take their function.

@bors
Copy link
Contributor

bors commented Mar 3, 2016

☔ The latest upstream changes (presumably #31979) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

I talked with @pnkfelix this evening and he proposed (correct me if I'm wrong) that since there wasn't that much movement on the function I need, instead of doing a global one, it'll be a more specific one. So what I have in mind is the following:

  • Checking type size.
  • Compare type's type (struct vs struct, enum vs enum, primitive vs primitive, etc...).
  • Checking name (it'll be the last resort, and I'm actually not sure if it's needed).

I'll wait a few days then, if no ones has objection, I'll start to work on this.

@nikomatsakis
Copy link
Contributor

@GuillaumeGomez I don't quite understand what you are proposing -- what is this type whose size etc you are checking and so forth? I do agree though that it seems fine to add a bit of special-case logic for the error messages around indexing.

@GuillaumeGomez
Copy link
Member Author

Oh sorry, my message is a bit incomplete. So I was tried to explain was to "guess" what was the trait the user tried to use. So for example:

let s = &[0, 1];
s[0i32];

will span an error because index is implemented for usize, not i32. Now we have to look what trait implementation could have been used, and we have the choice between:

  • Index<usize>
  • Index<RangeTo<usize>>
  • Index<RangeFrom<usize>>
  • Index<RangeFull>

It seems obvious for humans that is Index<usize> but not for a computer, so we need to add the function. To guess the good type, I thought to first compare type's size (i32 is close to usize after all), type's type (i32 is primitive, usize is primitive, others aren't), and in last resort the name.

Now I guess I added the missing part of my previous explanation. Very sorry about this.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2016

@GuillaumeGomez I've edited your comment to add the ... necessary to avoid having <...> interpreted as HTML.

@GuillaumeGomez
Copy link
Member Author

@eddyb: Thanks! :)

@eddyb
Copy link
Member

eddyb commented Mar 22, 2016

@GuillaumeGomez I think the size and "is primitive" checks could be replaced by the "flavor" of type - i32 and usize are both integers, whereas the Range types are structs.

@GuillaumeGomez
Copy link
Member Author

So remove my first point and keep the two others? Fine by me.

@bors
Copy link
Contributor

bors commented Mar 27, 2016

☔ The latest upstream changes (presumably #32432) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

Yes, I agree with @eddyb -- checking the size seems like it may not be
that helpful. Probably we just want to specialize this message around
integer types in general, no? But otherwise, creating some rough
categories and checking for those is probably good.

On Tue, Mar 22, 2016 at 11:01:06AM -0700, Guillaume Gomez wrote:

So remove my first point and keep the two others? Fine by me.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#31071 (comment)

@nikomatsakis
Copy link
Contributor

@GuillaumeGomez I'm going to close this PR for the time being. Please feel free to open another at some later point :)

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 11, 2016
…pnkfelix

Add rustc_on_unimplemented for Index implementation on slice

Reopening of rust-lang#31071.

It also extends the possibility of `#[rustc_on_unimplemented]` by providing a small type filter in order to find the ones which corresponds the most.

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants