Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix two const_trait_impl issues #100336

Merged
merged 4 commits into from
Aug 22, 2022
Merged

Fix two const_trait_impl issues #100336

merged 4 commits into from
Aug 22, 2022

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Aug 9, 2022 •
edited
Loading

r? @oli-obk

Fixes #100222.
Fixes #100543.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 9, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
compiler-errors
compiler-errors reviewed Aug 14, 2022
View reviewed changes
compiler/rustc_typeck/src/check/wfcheck.rs Outdated
);
}

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.
Copy link
Member

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?

Copy link
Member

@compiler-errors compiler-errors Aug 14, 2022 •
edited
Loading

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Contributor

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).

Copy link
Member Author

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.

@estebank
Copy link
Contributor

@oli-obk is currently out. CC @rust-lang/types

r? rust-lang/types

@rust-highfive rust-highfive assigned lcnr and unassigned oli-obk Aug 15, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fee1-dead fee1-dead force-pushed the fix-wf-const-trait branch 2 times, most recently from 85c785a to 758b550 Compare August 16, 2022 06:28
@fee1-dead fee1-dead changed the title Fix wf check on #[const_trait] return types Fix two const_trait_impl issues Aug 16, 2022
@bors
Copy link
Contributor

bors commented Aug 22, 2022

☔ The latest upstream changes (presumably #100654) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned lcnr Aug 22, 2022
oli-obk
oli-obk reviewed Aug 22, 2022
View reviewed changes
src/test/ui/rfc-2632-const-trait-impl/issue-100222.rs Outdated
Comment on lines 9 to 12
#[const_trait]
pub trait IndexMut where Self: Index {
fn foo(&mut self) -> <Self as Index>::Output;
}
Copy link
Contributor

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?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2022

📌 Commit f019b6c has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

r=me with CI passing ^^

@fee1-dead
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 22, 2022

📌 Commit f1db3be has been approved by oli-obk

It is now in the queue for this repository.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 22, 2022
@Dylan-DPC
Rollup merge of rust-lang#100336 - fee1-dead-contrib:fix-wf-const-tra…
1a92d96 
…it, r=oli-obk

Fix two const_trait_impl issues

r? `@oli-obk`

Fixes rust-lang#100222.
Fixes rust-lang#100543.
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 22, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2022
@bors
Auto merge of rust-lang#100881 - Dylan-DPC:rollup-q9rr658, r=Dylan-DPC
a785176 
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
@bors bors merged commit 14c8a68 into rust-lang:master Aug 22, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 22, 2022
@fee1-dead fee1-dead deleted the fix-wf-const-trait branch August 23, 2022 01:09
@fee1-dead fee1-dead mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@estebank estebank estebank left review comments

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

regression: can't compare X with _ in const contexts #[const_trait] and super trait error
9 participants
@fee1-dead @rust-log-analyzer @estebank @bors @oli-obk @compiler-errors @rust-highfive @lcnr @rustbot