Stream: t-compiler/wg-nll

Topic: #53807 error dedupe


davidtwco (Sep 06 2018 at 13:58, on Zulip):

FYI @nikomatsakis. Submitted #53995. Not sure if the approach is fantastic but the result seems to be fairly decent.

davidtwco (Sep 11 2018 at 13:11, on Zulip):

@nikomatsakis Do you have any thoughts on the issue reported by kennytm from the merge testing? Wasn't sure whether to r= on it or not.

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

I think @kennytm is probably correct in their analysis

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

I'm not sure why this PR is at fault mind you :P

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

have you tried 32 bit linux by any chance?

davidtwco (Sep 11 2018 at 13:13, on Zulip):

I haven't.

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

often these things are due to use of a FxHashMap or something

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

though I'm not sure why that would be the case

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

(maybe it differs per platform?)

davidtwco (Sep 11 2018 at 13:13, on Zulip):

It does use a FxHashMap.

nikomatsakis (Sep 11 2018 at 13:13, on Zulip):

in particular iterating over such a hashmap

nikomatsakis (Sep 11 2018 at 13:14, on Zulip):

let me look over the PR again

davidtwco (Sep 11 2018 at 13:14, on Zulip):

Well, we iterate over the hashmap. But the map contains one error per the move out indices which are then buffered and sorted as before when the buffered errors are emitted.

pnkfelix (Sep 11 2018 at 13:16, on Zulip):

but the sort may not be total?

pnkfelix (Sep 11 2018 at 13:16, on Zulip):

(or rather, the order used in the sort might not be total)

nikomatsakis (Sep 11 2018 at 13:20, on Zulip):

so we do sort the errors?

nikomatsakis (Sep 11 2018 at 13:20, on Zulip):

that are buffered?

nikomatsakis (Sep 11 2018 at 13:20, on Zulip):

(I couldn't remember)

pnkfelix (Sep 11 2018 at 13:21, on Zulip):

I thought we did. By line number at least

pnkfelix (Sep 11 2018 at 13:21, on Zulip):

which of course is not a total order

nikomatsakis (Sep 11 2018 at 13:21, on Zulip):

@davidtwco how do we pick which error to keep? I guess we ought to be producing the errors in the same order

pnkfelix (Sep 11 2018 at 13:21, on Zulip):

but is enough of an order to probably fool most tests

davidtwco (Sep 11 2018 at 13:31, on Zulip):

@nikomatsakis In the order that errors were buffered previously, we check whether or not to keep them based on the last error we saw (in particular, whether the last error is a prefix of the current one), if we opt to keep it, we replace it in the hashmap (against the move out indices), then, before errors are emited, we iterate over the hashmap, adding each error to the error buffer. The error buffer code, as it always has, sorts by primary span.

nikomatsakis (Sep 11 2018 at 13:31, on Zulip):

@davidtwco you might try a BTreeMap instead of a FxHashMap

nikomatsakis (Sep 11 2018 at 13:32, on Zulip):

for catching the duplicates

davidtwco (Sep 11 2018 at 13:32, on Zulip):

I don't know how to test it as I can't reproduce a different order.

nikomatsakis (Sep 11 2018 at 13:32, on Zulip):

alternatively, you could do some local experiments:

nikomatsakis (Sep 11 2018 at 13:32, on Zulip):

e.g., store the hashmap into a vector

nikomatsakis (Sep 11 2018 at 13:32, on Zulip):

sort it randomly

nikomatsakis (Sep 11 2018 at 13:32, on Zulip):

and see if it affects your local output order :)

pnkfelix (Sep 11 2018 at 13:33, on Zulip):

let me see if the sort is obviously non-total (and "obviously" should have been total)

pnkfelix (Sep 11 2018 at 13:34, on Zulip):

Yeah, this is suspect: mbcx.errors_buffer.sort_by_key(|diag| diag.span.primary_span())

davidtwco (Sep 11 2018 at 13:34, on Zulip):

Both errors have the same primary span. So the sort of buffered errors doesn't make a difference in this case.

davidtwco (Sep 11 2018 at 13:35, on Zulip):

What was previously insertion order into the buffer is now the same insertion order as into the map (when they are inserted). So the only point where it can happen is when the map is iterated over to re-add into buffer. I think a BTreeMap'll do it.

nikomatsakis (Sep 11 2018 at 13:36, on Zulip):

another alterantive is to keep a vector

nikomatsakis (Sep 11 2018 at 13:36, on Zulip):

that sort of stores the "canonical order"

nikomatsakis (Sep 11 2018 at 13:36, on Zulip):

or something

nikomatsakis (Sep 11 2018 at 13:36, on Zulip):

anyway, I'm sure it's this somehow

davidtwco (Sep 11 2018 at 13:37, on Zulip):

Yeah, thinking about it with a fresh perspective, it makes sense.

davidtwco (Sep 11 2018 at 14:03, on Zulip):

Pushed a commit for this.

nikomatsakis (Sep 11 2018 at 14:07, on Zulip):

@davidtwco were you able to observe whether it made a difference locally to alter the "buffer order"?

davidtwco (Sep 11 2018 at 14:14, on Zulip):

@nikomatsakis The test that failed that one time changed the output order.

nikomatsakis (Sep 11 2018 at 14:14, on Zulip):

cool

nikomatsakis (Sep 11 2018 at 14:14, on Zulip):

usually I solve these things by a judicious change from FxHashMap to BTreeMap

nikomatsakis (Sep 11 2018 at 14:14, on Zulip):

I wish there were a more automatic way to figure it out

nikomatsakis (Sep 11 2018 at 14:15, on Zulip):

I would almost say I wish that FxHashMap didn't expose an iterator at all

nikomatsakis (Sep 11 2018 at 14:15, on Zulip):

probably what would be better is just if FxHashMap would preserve insertion order

nikomatsakis (Sep 11 2018 at 14:15, on Zulip):

oh well

davidtwco (Sep 11 2018 at 14:15, on Zulip):

I just realised I didn't run all the tests, just that one. Let me double check the rest.

davidtwco (Sep 11 2018 at 14:22, on Zulip):

Alright, I've confirmed they all pass.

nikomatsakis (Sep 11 2018 at 14:24, on Zulip):

nice

nikomatsakis (Sep 11 2018 at 14:25, on Zulip):

left a quick review

davidtwco (Sep 11 2018 at 14:28, on Zulip):

Oops, forgot about that. Fixed.

Last update: Nov 21 2019 at 23:30UTC