-
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
Fix two const_trait_impl issues #100336
Fix two const_trait_impl issues #100336
Conversation
This comment has been minimized.
This comment has been minimized.
fb617e9
to
7de7c9c
Compare
); | ||
} | ||
|
||
implied_bounds.extend(sig.inputs()); | ||
|
||
wfcx.register_wf_obligation(hir_decl.output.span(), None, sig.output().into()); | ||
// override the env when checking the return type. `~const` bounds can be fulfilled with non-const implementations. |
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.
Shouldn't you also be applying this to arg types too?
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.
Also, I guess a deeper question is whether this should be fixed in rustc_trait_selection::traits::wf
instead..?
Or I guess whether we should be doing wfcheck with a Constness::Const
param-env at all? e.g. should we just be doing param_env.without_const
when constructing the wfcx
Like, is it ever required for correctness to register a wf obligation with Constness::Const
? What would break/be unsound/be incorrect if we just made the param-env not const during wfcheck?
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.
whether we should be doing wfcheck with a
Constness::Const
param-env at all?
We should. For an impl
to be well-formed, all super traits must be fulfilled. In the case of const
super traits, we need to ensure that the parent trait has a const
impl
.
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 guess I mean for the nominal obligations of the projection types in the signature, for example. Not necessarily when we verify that a trait's supertraits are satisfied.
Put in a different way, any time we instantiate a WellFormed predicate, is it ever wrong to pass a param env with no constness there instead?
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.
That seems desirable. This is related to #100543. I'm going to look into this further.
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 tend to agree with @compiler-errors' reasoning here (it was my first thought when looking at the code).
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.
This has been done, but only from one producer of wf obligations, to fix this issue. The second fix is unfortunate but is the only solution I can find without breaking the tests.
@oli-obk is currently out. CC @rust-lang/types r? rust-lang/types |
7de7c9c
to
8429cd0
Compare
This comment has been minimized.
This comment has been minimized.
85c785a
to
758b550
Compare
#[const_trait]
return types758b550
to
7f02890
Compare
☔ The latest upstream changes (presumably #100654) made this pull request unmergeable. Please resolve the merge conflicts. |
r? @oli-obk |
#[const_trait] | ||
pub trait IndexMut where Self: Index { | ||
fn foo(&mut self) -> <Self as Index>::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.
So... the problem was that this is not Self: ~const Index
, but it was tested as such? Do we have tests for various combinations of #[const_trait]
being on the super trait or not, and being on the trait itself or not?
7f02890
to
d744f36
Compare
@bors r+ |
r=me with CI passing ^^ |
@bors r=oli-obk |
…it, r=oli-obk Fix two const_trait_impl issues r? `@oli-obk` Fixes rust-lang#100222. Fixes rust-lang#100543.
Rollup of 8 pull requests Successful merges: - rust-lang#98200 (Expand potential inner `Or` pattern for THIR) - rust-lang#99770 (Make some const prop mir-opt tests `unit-test`s) - rust-lang#99957 (Rework Ipv6Addr::is_global to check for global reachability rather than global scope - rebase) - rust-lang#100331 (Guarantee `try_reserve` preserves the contents on error) - rust-lang#100336 (Fix two const_trait_impl issues) - rust-lang#100713 (Convert diagnostics in parser/expr to SessionDiagnostic) - rust-lang#100820 (Use pointer `is_aligned*` methods) - rust-lang#100872 (Add guarantee that Vec::default() does not alloc) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
r? @oli-obk
Fixes #100222.
Fixes #100543.