Add rustc_on_unimplemented for Index implementation on slice #33401

GuillaumeGomez · 2016-05-04T16:50:17Z

Reopening of #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

GuillaumeGomez · 2016-05-05T10:14:26Z

I fixed tests breakage (forgot to add Notes lines).

GuillaumeGomez · 2016-05-07T08:18:10Z

cc @nikomatsakis

GuillaumeGomez · 2016-05-09T12:38:31Z

Any news on this?

pnkfelix · 2016-05-10T16:31:33Z

src/librustc/traits/error_reporting.rs

+}
+
+impl<'a> AssociatedWeight for TypeVariants<'a> {
+    // First number is for "global" weight and second number is for bigger precision


what two numbers is this comment referring to? (Did this used to return (usize, usize) or something?

oh, weird; from fn get_weight_diff, I guess you are effectively encoding a pair (x, y), where y < 10, as x * 10 + y?

In which case you should probably say something like "the digit in the ten's place is for "global weight" and the digit in the one's place is for bigger precision."

(Though I haven't read enough yet to understand the phrases "global weight" and "bigger precision" here.)

That seems like a bad idea... if we're into microoptimizations here, a number that can't be larger than two digits can be a u8 :) Otherwise, just use a pair.

(i'm inclined to agree with @durka; it doesn't seem like it would be that onerous to write the match using (u32, u32) or something.)

Then let's go for the tuple!

pnkfelix · 2016-05-10T17:24:04Z

I'm debating about whether I would like to see more tests or not.

On the one hand: It would be good to have tests demonstrating the behavior in cases that aren't just usize/u32/i32

And I might like to also see concrete examples of the kind of scenario I mentioned above, namely where "many small differences accumulating to eventually become larger than one big difference in weight."

On the other hand, this code is by its very nature a mere heuristic. We almost certainly want to allow ourselves plenty of freedom to revise how the "best" candidate impl is chosen, which means that having a large test suite that ties down the particular numbers chosen in fn get_weight could be a bad idea.

pnkfelix · 2016-05-10T17:24:51Z

@GuillaumeGomez overall I'm really happy with this PR (despite the tone of some of my commentary above).

I think I'll just say: r=me after the nits are addressed.

pnkfelix · 2016-05-10T17:33:29Z

One more throw-away thought: I wonder whether there would be any value to the end user to have a compilation mode (selected via some flag, perhaps --verbose ...) where it would present all of the most coarsely matching impls, rather than trying to pick the best of the coarse via the second finer grain pass.

GuillaumeGomez · 2016-05-10T19:10:52Z

One more throw-away thought: I wonder whether there would be any value to the end user to have a compilation mode (selected via some flag, perhaps --verbose ...) where it would present all of the most coarsely matching impls, rather than trying to pick the best of the coarse via the second finer grain pass.

That's a comment I already had and I didn't really know how to address it. So I thought about doing something like:

If you meant to use usize, here's a tip: [usize rustc_on_unimplemented message]
If you meant to use Range, here's a tip: [Range rustc_on_unimplemented message]
...

However, I'm afraid it'll just flood the output in the case there are many potential corresponding types. But maybe I'm wrong, I'd be glad to have a second thought about my thinking.

GuillaumeGomez · 2016-05-10T19:13:57Z

To address this comment: the "big" weight number (so in 12, it's 1) determines a first category. The second number (so 2 in here) only applies to the types of the same categories and doesn't really have any other meaning than differentiating them for the moment. I made this algorithm as easy as possible to maintain and potentially evolve, so I need to add more comments and more clarity.

GuillaumeGomez · 2016-05-10T20:55:04Z

@pnkfelix: I think I had addressed all the nits. Do you see anything else?

pnkfelix · 2016-05-10T23:43:41Z

src/librustc/traits/error_reporting.rs

+
+// Here, we run the "big" filter. Little example:
+//
+// We receive a `String`, an `u32` and in `i32`.


typo: "an i32", not "in i32"

pnkfelix · 2016-05-10T23:44:26Z

@GuillaumeGomez other than the typo I just commented on, I think this looks good.

GuillaumeGomez · 2016-05-11T07:23:01Z

I fixed the typo.

bors · 2016-05-11T10:03:51Z

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

…mpl, this message will be displayed instead

Add more comments for the global understanding

GuillaumeGomez · 2016-05-11T14:53:13Z

All fixed.

pnkfelix · 2016-05-11T18:36:29Z

@bors r+ rollup

bors · 2016-05-11T18:36:31Z

📌 Commit 4a3acfd has been approved by pnkfelix

@pnkfelix

…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

Rollup of 3 pull requests - Successful merges: #33401, #33489, #33558 - Failed merges: #33342, #33475, #33517

pnkfelix · 2016-05-17T15:40:27Z

(since this is tagged with relnotes, I'll just note that a portion of the changes introduced here were subsequently reverted in #33491 , so just take care to double check the state of the actual code in terms of what is written in the release notes...)

rust-highfive assigned pnkfelix May 4, 2016

GuillaumeGomez force-pushed the index_indication branch from 5155d5e to ce3019d Compare May 4, 2016 23:02

pnkfelix reviewed May 10, 2016
View reviewed changes

GuillaumeGomez force-pushed the index_indication branch from af591bb to d21360b Compare May 11, 2016 07:22

GuillaumeGomez and others added 4 commits May 11, 2016 13:18

Extend rustc_on_unimplemented flag: if a message is available at an i…

e9797d4

…mpl, this message will be displayed instead

Improve error message for Index trait on slices

b8fad79

Improve weight algorithm and tests

61e6169

Create Weight enum

e6c0ff6

Add more comments for the global understanding

GuillaumeGomez force-pushed the index_indication branch 3 times, most recently from b5bfb8e to 2c829c1 Compare May 11, 2016 14:52

Update to eddyb's PR

4a3acfd

GuillaumeGomez force-pushed the index_indication branch from 2c829c1 to 4a3acfd Compare May 11, 2016 15:03

GuillaumeGomez mentioned this pull request May 11, 2016

Rollup of 3 pull requests #33562

Merged

bors added a commit that referenced this pull request May 11, 2016

Auto merge of #33562 - GuillaumeGomez:rollup, r=steveklabnik

22ac88f

Rollup of 3 pull requests - Successful merges: #33401, #33489, #33558 - Failed merges: #33342, #33475, #33517

bors merged commit 4a3acfd into rust-lang:master May 12, 2016

durka mentioned this pull request May 12, 2016

provide on_unimplemented hint for ops::Index #31062

Closed

GuillaumeGomez deleted the index_indication branch May 12, 2016 07:19

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label May 13, 2016

GuillaumeGomez restored the index_indication branch May 17, 2016 16:58

GuillaumeGomez mentioned this pull request Sep 22, 2016

Improve documentation and error message for Vec type on erroneous indexing #36645

Closed

pnkfelix mentioned this pull request Jan 3, 2017

Properly ban the negation of unsigned integers in type-checking. #38776

Merged

GuillaumeGomez deleted the index_indication branch August 31, 2017 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rustc_on_unimplemented for Index implementation on slice #33401

Add rustc_on_unimplemented for Index implementation on slice #33401

GuillaumeGomez commented May 4, 2016

GuillaumeGomez commented May 5, 2016

GuillaumeGomez commented May 7, 2016

GuillaumeGomez commented May 9, 2016

pnkfelix May 10, 2016

pnkfelix May 10, 2016

pnkfelix May 10, 2016

durka May 10, 2016

pnkfelix May 10, 2016

GuillaumeGomez May 10, 2016

pnkfelix commented May 10, 2016

pnkfelix commented May 10, 2016

pnkfelix commented May 10, 2016

GuillaumeGomez commented May 10, 2016

GuillaumeGomez commented May 10, 2016

GuillaumeGomez commented May 10, 2016

pnkfelix May 10, 2016

pnkfelix commented May 10, 2016

GuillaumeGomez commented May 11, 2016

bors commented May 11, 2016

GuillaumeGomez commented May 11, 2016

pnkfelix commented May 11, 2016

bors commented May 11, 2016

pnkfelix commented May 17, 2016

Add rustc_on_unimplemented for Index implementation on slice #33401

Add rustc_on_unimplemented for Index implementation on slice #33401

Conversation

GuillaumeGomez commented May 4, 2016

GuillaumeGomez commented May 5, 2016

GuillaumeGomez commented May 7, 2016

GuillaumeGomez commented May 9, 2016

pnkfelix May 10, 2016

Choose a reason for hiding this comment

pnkfelix May 10, 2016

Choose a reason for hiding this comment

pnkfelix May 10, 2016

Choose a reason for hiding this comment

durka May 10, 2016

Choose a reason for hiding this comment

pnkfelix May 10, 2016

Choose a reason for hiding this comment

GuillaumeGomez May 10, 2016

Choose a reason for hiding this comment

pnkfelix commented May 10, 2016

pnkfelix commented May 10, 2016

pnkfelix commented May 10, 2016

GuillaumeGomez commented May 10, 2016

GuillaumeGomez commented May 10, 2016

GuillaumeGomez commented May 10, 2016

pnkfelix May 10, 2016

Choose a reason for hiding this comment

pnkfelix commented May 10, 2016

GuillaumeGomez commented May 11, 2016

bors commented May 11, 2016

GuillaumeGomez commented May 11, 2016

pnkfelix commented May 11, 2016

bors commented May 11, 2016

pnkfelix commented May 17, 2016