FYI @nikomatsakis. Submitted #53995. Not sure if the approach is fantastic but the result seems to be fairly decent.
@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.
I think @kennytm is probably correct in their analysis
I'm not sure why this PR is at fault mind you :P
have you tried 32 bit linux by any chance?
often these things are due to use of a
FxHashMap or something
though I'm not sure why that would be the case
(maybe it differs per platform?)
It does use a
in particular iterating over such a hashmap
let me look over the PR again
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.
but the sort may not be total?
(or rather, the order used in the sort might not be total)
so we do sort the errors?
that are buffered?
(I couldn't remember)
I thought we did. By line number at least
which of course is not a total order
@davidtwco how do we pick which error to keep? I guess we ought to be producing the errors in the same order
but is enough of an order to probably fool most tests
@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.
@davidtwco you might try a
BTreeMap instead of a
for catching the duplicates
I don't know how to test it as I can't reproduce a different order.
alternatively, you could do some local experiments:
e.g., store the hashmap into a vector
sort it randomly
and see if it affects your local output order :)
let me see if the sort is obviously non-total (and "obviously" should have been total)
Yeah, this is suspect:
Both errors have the same primary span. So the sort of buffered errors doesn't make a difference in this case.
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.
another alterantive is to keep a vector
that sort of stores the "canonical order"
anyway, I'm sure it's this somehow
Yeah, thinking about it with a fresh perspective, it makes sense.
Pushed a commit for this.
@davidtwco were you able to observe whether it made a difference locally to alter the "buffer order"?
@nikomatsakis The test that failed that one time changed the output order.
usually I solve these things by a judicious change from
I wish there were a more automatic way to figure it out
I would almost say I wish that
FxHashMap didn't expose an iterator at all
probably what would be better is just if
FxHashMap would preserve insertion order
I just realised I didn't run all the tests, just that one. Let me double check the rest.
Alright, I've confirmed they all pass.
left a quick review
Oops, forgot about that. Fixed.