Refactored check::autoderef in librustc_typeck to allow additional behaviour upon reaching the recursion limit

[skeleten]

Fixes bug #19509
In this bug, it relied for the autoderef method to terminate once it reaches a non-deref-able type and collecting all types before selecting the appropiate type.
However, if there were cycles in the deref-types this would never happen and instead the function would hit its recursion limit resulting in an error, even if an apropiate type could've been found within the recursion limit.

This change refactores autoderef into another function autoderef_with_recursion_option and adding an additonal parameter describing the behaviour upon reaching the recursion limit.
The autoderef function calls autoderef_with_recursion_option with the default behaviour (error gracefully) while the problemsome function calls it with an option allowing it to not error, but return the last found type.

Another approach would've been to detect cycles directly by comparing it to a list of already checked types, however this would most likely have added more overhead.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jfager]

This looks like it makes autoderef's non-deterministic (or at least very sensitive to the number of available derefs)?

[skeleten]

In the case of method derefing it does preserve the order in which it is derefeered. If this would cause non deterministic behavior, so would having more than one fitting type in the hierachie, wouldnt it? (actual question)

[eefriedman]

Intentionally hitting the recursion limit seems like a bad idea: it will hurt rustc's performance on code which uses constructs like this, and depending on the recursion limit as part of the non-error control flow is confusing.

[skeleten]

The other option - checking after each iteration if we already found the current type, scales much worse though. While simply relying on the recursion limit terminates after O(n), with n being the recursion limit in case of a cycle, the other option require (n(n+1))/2 checks, which scales with O(n^2), n being the size of the cycle + the head leading into it.
For small cycles it is faster, no doubt, but any cycle of a size > log recursion_limit would be faster with the implemented variant

[brson]

r? @pnkfelix

[skeleten]

To be fair, I tried to implement the cycle-detecting variant as well. And it seems less intrusive, than this one, even though it scales worse. I'd be able to make another PR with that variant, if prefered. (I wouldn't need to change the autoderef function at all, just the closure passed into it)

The performance depends pretty heavily on the size of the cycles, so I'm not really sure which would be the better one.

[pnkfelix]

yay docs!

[pnkfelix]

@skeleten based on niko's feedback on the other bug, I'm hesitant to immediately r+ the strategy here.

In particular, it sounds to me like @nikomatsakis thinks the strategy you've used here is too fragile.

You mentioned that cycle-detection doesn't scale well. (I assume by that you mean that it adds more overhead to the compilation time compared to this change.) Couldn't we cope with that by starting to do the cycle detection only after we've hit some initial work threshold? I.e., once autoderefs hits some fixed threshold (less than the recursion limit), then switch to a different code path that includes cycle detection (restarting the loop if necessary).

That is, as I understand what others are saying: if you can prove that a cycle actually exists, then it might be sound to cut short the candidate expansion. But in the absence of such proof, continue to error upon hitting the recursion limit?

---

Having said that, I'm going to ping @nikomatsakis and ask him what he thinks about the options here.

[skeleten]

The actual performance overhead is very dependent on the recursion_limit itself, as well as on the the amount of deref's until a cycle is complete. Of course, both checking the first n or the last n types is very possible, if we make the autoderef function responsible for detecting those changes. I'd suggest treating a cycle "pretending" it is the end of the chain, meaning it cannot be deref'd any further.
EDIT: Since the work is greater in the later iterations (as pointed out before, scaling with O(n), since we need to check every type that was dereferenced before) dropping the later checks should be more desireable than the early onces, IMO.

[pnkfelix]

handing review off to @nikomatsakis for further discussion/decisions

r? @nikomatsakis

[skeleten]

It seems that triage has decided to detect cycls, making this implementation (and therefore PR) obsolete, so I'll close this here :)