-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Begin to abstract rustc_type_ir
for rust-analyzer
#116828
Begin to abstract rustc_type_ir
for rust-analyzer
#116828
Conversation
@bors try @rust-timer queue the debug formatting changed to use the non-convenience functions -- shouldn't affect perf but weirder things have happened |
This comment has been minimized.
This comment has been minimized.
…e_ir, r=<try> Begin to abstract `rustc_type_ir` for rust-analyzer Not totally happy with this yet, but if the new trait solver is ever to be used by external users, we need to make rustc-type-ir build without internal deps. This adds the "nightly" feature which is used by the compiler, and falls back to more simple implementations when that is not active. r? `@lcnr` or `@jackh726` -- happy to block this pr on a discussion about design and long-term direction for rustc-type-ir tho
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (26d6e91): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 627.292s -> 625.722s (-0.25%) |
Let's schedule some time to discuss this? I think some of these are okay to do short-term, but I'd like to have some idea of what we want long-term. |
type Never = !; | ||
|
||
#[cfg(not(feature = "nightly"))] | ||
type Never = std::convert::Infallible; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half serious: you could just use <fn() -> !>::Output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to have problems with type inference later on in the crate:'
error[E0284]: type annotations needed
--> compiler/rustc_type_ir/src/fold.rs:223:18
|
223 | type Error = Never;
| ^^^^^ cannot infer type
|
= note: cannot satisfy `<F as FallibleTypeFolder<I>>::Error == _`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the MRE is this: (play).
FallibleTypeFolder<Error = Never>
from TypeFolder
super-traits gets checked against type Error = Never
in the blanket impl of FallibleTypeFolder
for F: TypeFolder
.
First of all, are cycles like this even allowed? I.e. can you satisfy a super-trait bound by a blanket impl that asks for the same trait? I would think that this causes a cycle...
Second of all, can't we just remove the super trait? It seems like all the things that can be proven with the supertrait bound can be proven via the blanket impl (that might be wrong, not sure).
Lastly, why the compiler can't evaluate this projection here? It seems to be fine in other places, and I don't see what can't it infer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why it should cause a cycle... impls (blanket or otherwise) can be provided even where supertraits are not satisfied.
We can't "just" remove the supertrait because the default implementations of the subtrait's provided methods ultimately call into the supertrait — and I don't think the existence of the blanket impl is provable there, within the trait definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so the error arises whenever the associated type doesn't match the supertrait constraint: (play).
Not sure why the compiler is unable to prove that the Never
types are equal, though...
6f43680
to
8006369
Compare
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
8006369
to
9bae00d
Compare
Ending up pulling out the newtype macro and applying the changes from #116830, so I'm slightly happier with the changes now. |
This comment has been minimized.
This comment has been minimized.
f331d63
to
d627468
Compare
I'm interested in understanding the long-term direction here too: could you direct me towards any discussion you end up having over this @compiler-errors and @jackh726 ? Thanks! |
@eggyal: Yeah I haven't had too many conversations about this, but I'll make sure to open a topic on the t-types zulip stream if I do. |
☔ The latest upstream changes (presumably #116951) made this pull request unmergeable. Please resolve the merge conflicts. |
d627468
to
9b2792d
Compare
☔ The latest upstream changes (presumably #117126) made this pull request unmergeable. Please resolve the merge conflicts. |
Would appreciate a review on this, since it's very bitrotty :'( |
9b2792d
to
4506681
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question - but r=me once you resolve that.
#[debug_format = "DebruijnIndex({})"] | ||
#[gate_rustc_only] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, am I confused, or does this mean that DebruijnIndex
(and UniverseIndex
) below are nightly (rustc) only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, why? If there is no reason, this shouldn't be gated.
If not, ignore me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry for the confusion. The attribute just means "emit all the nightly stuff (e.g. Step
and Encodable
/Decodable
) with a cfg(feature = "nightly")
so it can be compiled on stable too". Maybe it should be renamed, but can be later.
@bors r=jackh726 |
☀️ Test successful - checks-actions |
Finished benchmarking commit (4f3da90): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 676.7s -> 678.667s (0.29%) |
This adds the "nightly" feature which is used by the compiler, and falls back to more simple implementations when that is not active.
r? @lcnr or @jackh726