-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setup infra for handling auto trait bounds disabled due to perf problems #13112
Conversation
dacfc0c
to
eaaa78d
Compare
Would it be possible to assume that any unresolved type implements all auto traits to fix this? |
Fyi, we put #7637 on our roadmap for the next sprint so we can hopefully make progress in unblocking this great PR assuming Jonas' comment can't be addressed |
☔ The latest upstream changes (presumably #12793) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm sorry I couldn't respond any earlier. We could ignore and filter out the type of unresolved fields, which got the analysis-stats numbers back to those of master when I tested locally iirc. However I can't confidently tell that it wouldn't introduce any false-positives. FWIW, I see #7637 has been put on the table (thanks @Veykril for bringing it up in the steering meeting!). If it seems feasible to solve that issue before too long, I'd rather wait for it and then land this. |
eaaa78d
to
3ebfb45
Compare
We'll see how feasible fixing #7637 is onec we get in touch with the appropriate people, but I am hopeful here. I'll try to do that this week. |
☔ The latest upstream changes (presumably #13494) made this pull request unmergeable. Please resolve the merge conflicts. |
So from the looks of it, fixing #7637 won't happen for quite some time unfortunately, so I'd be inclined to check if Jonas' suggestion would work for us for now, https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/rust-lang.2Frust.2Flibrary.20workspace |
What will happen if we consider unknown type is implementing all auto traits? I think it shouldn't be worse than the current state where we effectively consider everything is implemented every auto trait by ignoring auto trait bounds? |
Oh, it was already discussed in detail and I missed that, sorry.
I think it wouldn't increase false positive type mismatches, as it would only add false positive implementations for auto traits, and those are already in their maximum since ignoring the auto trait bounds is equivalent to assuming everything implements every auto trait. And I would argue that we should do it anyway and implementing auto traits for unknown type is a better default, since there is a higher probability that an unknown type implements auto trait than not. |
Sorry for my unresponsiveness here and there! I might need to rethink about this (it's been months and I don't remember the details quite well 😅), but as I stated in my previous comment, some functions this PR implements are used to prove predicates that are not necessarily about auto traits. I still would like to wait for #7637 to be fully resolved, especially seeing #14599 (comment). |
I would love to see #7637 resolved, but even if it is resolved today by a change in the rustc side, it takes 3 months for it to become available in the stable channel, and even then lots of people are in older toolchains so there is value in making this PR not dependent of that. And I guess by using the "unknown implements every auto trait" as a default (which I think it is needed regardless of #7637) the goods of this PR will surpass its harms (I initially thought the harm will become zero, I'm still interested to see concrete examples of what will break, maybe we can work around that as well) and we can merge it. Then in some point in future (which we should actively move toward it anyway) #7637 becomes resolved and auto trait analysis will become even more accurate. |
3ebfb45
to
232fd82
Compare
232fd82
to
bfb5c7a
Compare
bfb5c7a
to
9d18e19
Compare
Rebased this and skipped fields containing unknown types so now there is no regression here, BUT this increases type checking time by 20+ seconds on my machine... |
Pushed a commit that removes the population of the field vec for now, that effectively disables the implementation here while keeping the changes. I consider the slowdown too much for the benefit of the PR, especially since you only get the benefit if you enable the sysroot metadata config anyways. |
Setup infra for handling auto trait bounds disabled due to perf problems This patch updates some of the partially-implemented functions of `ChalkContext as RustIrDatabase`, namely `adt_datum()` and `impl_provided_for()`. With those, we can now correctly work with auto trait bounds and distinguish methods based on them. Resolves #7856 (the second code; the first one is resolved by #13074) **IMPORTANT**: I don't think we want to merge this until #7637 is resolved. Currently this patch introduces A LOT of unknown types and type mismtaches as shown below. This is because we cannot resolve items like `hashbrown::HashMap` in `std` modules, leading to auto trait bounds on them and their dependents unprovable. |crate (from `rustc-perf@c52ee6` except for r-a)|e3dc5a588f07d6f1d3a0f33051d4af26190abe9e|HEAD of this branch| |---|---|---| |rust-analyzer @ e3dc5a5 |exprs: 417528, ??ty: 907 (0%), ?ty: 114 (0%), !ty: 1|exprs: 417528, ??ty: 1704 (0%), ?ty: 403 (0%), !ty: 20| |ripgrep|exprs: 62120, ??ty: 2 (0%), ?ty: 0 (0%), !ty: 0|exprs: 62120, ??ty: 132 (0%), ?ty: 58 (0%), !ty: 11| |webrender/webrender|exprs: 94355, ??ty: 49 (0%), ?ty: 16 (0%), !ty: 2|exprs: 94355, ??ty: 429 (0%), ?ty: 130 (0%), !ty: 7| |diesel|exprs: 132591, ??ty: 401 (0%), ?ty: 5129 (3%), !ty: 31|exprs: 132591, ??ty: 401 (0%), ?ty: 5129 (3%), !ty: 31|
💔 Test failed - checks-actions |
b788db1
to
d2b27d0
Compare
@bors r+ |
☀️ Test successful - checks-actions |
This patch updates some of the partially-implemented functions of
ChalkContext as RustIrDatabase
, namelyadt_datum()
andimpl_provided_for()
. With those, we can now correctly work with auto trait bounds and distinguish methods based on them.Resolves #7856 (the second code; the first one is resolved by #13074)
IMPORTANT: I don't think we want to merge this until #7637 is resolved. Currently this patch introduces A LOT of unknown types and type mismtaches as shown below. This is because we cannot resolve items like
hashbrown::HashMap
instd
modules, leading to auto trait bounds on them and their dependents unprovable.rustc-perf@c52ee6
except for r-a)