resolve: Try to fix instability in import suggestions #43552
Conversation
|
Even if this becomes deterministic by these changes, I dislike the results:
I still think that it's impossible to produce nice diagnostics with paths during resolve and that we should be caching them until resolve is finished and report them then |
|
I don't see how delaying will help here. The problem with prelude paths is that reexports are shown in candidates as well as true definitions. The intent of the original PR was to filter out the reexports, however the implementation doesn't work correctly and nothing outside of the current crate is considered a reexport and filtered out. |
|
Makes sense. Implementation Lgtm. You'll need a reviewer to sign off the PR though. |
|
r? @jseyfried |
carols10cents
added
the
S-waiting-on-review
|
@bors r+ |
|
@bors r=jseyfried |
|
📌 Commit a6993d6 has been approved by |
|
⌛ Testing commit a6993d6 with merge ca764ee39451f96aa9db27b392b6d1b1974676c3... |
|
💔 Test failed - status-travis |
|
@bors: retry
|
resolve: Try to fix instability in import suggestions cc #42033 `lookup_import_candidates` walks module graph in DFS order and skips modules that were already visited (which is correct because there can be cycles). However it means that if we visited `std::prelude::v1::Result::Ok` first, we will never visit `std::result::Result::Ok` because `Result` will be skipped as already visited (note: enums are also modules here), and otherwise, if we visited `std::result::Result::Ok` first, we will never get to `std::prelude::v1::Result::Ok`. What child module of `std` (`prelude` or `result`) we will visit first, depends on randomized hashing, so we have instability in diagnostics. With this patch modules' children are visited in stable order in `lookup_import_candidates`, this should fix the issue, but let's see what Travis will say. r? @oli-obk
|
☀️ Test successful - status-appveyor, status-travis |
jyn514
added
A-resolve
cc #42033
lookup_import_candidateswalks module graph in DFS order and skips modules that were already visited (which is correct because there can be cycles).However it means that if we visited
std::prelude::v1::Result::Okfirst, we will never visitstd::result::Result::OkbecauseResultwill be skipped as already visited (note: enums are also modules here), and otherwise, if we visitedstd::result::Result::Okfirst, we will never get tostd::prelude::v1::Result::Ok.What child module of
std(preludeorresult) we will visit first, depends on randomized hashing, so we have instability in diagnostics.With this patch modules' children are visited in stable order in
lookup_import_candidates, this should fix the issue, but let's see what Travis will say.r? @oli-obk