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.

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

Do not try to reveal hidden types when trying to prove Freeze in the defining scope #122192

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 8, 2024

fixes #99793

this avoids the cycle error by just causing a selection error, which is not fatal. We pessimistically assume that freeze does not hold, which is always a safe assumption.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Mar 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@oli-obk oli-obk added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Mar 8, 2024
@oli-obk oli-obk assigned lcnr and unassigned estebank Mar 8, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 8, 2024

Reassigning reviewer because it allows more code on stable and needs trait system reviews

@oli-obk oli-obk added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 8, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 11, 2024

happy with this change in general, unless this blocks the stabilization of ATPIT I would wait until #122077 is merged for an in-depth review

@lcnr lcnr added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 11, 2024

This PR is low priority and deserves some mir_const_qualifs cleanups and reduplications before landing anyway. The other two PRs this one is based on are needed for ATPIT

@oli-obk oli-obk removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 19, 2024
@oli-obk oli-obk force-pushed the type_of_opaque_for_const_checks branch from 69500f9 to 8de5565 Compare April 19, 2024 15:07
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2024

I would like to make more code compile by avoiding unnecessary (and false) query cycles in Freeze bound checking during const checks. Specifically, the following code snippet should compile (it doesn't before this PR):

const fn f() -> impl Eq {
    g()
}
const fn g() {}

The reason is that when checking whether the return type implements Freeze we end up trying to reveal the opaque type. Since we're also defining the opaque type, this causes a cycle. What this PR instead does (which is what the new solver already does), is to never reveal opaque types for auto traits if we're in the defining scope. Instead, we just bail out with a non-fatal selection error. This means we do not know the answer, but if the code asking cares only about knowing for sure a trait is implemented, but is fine pessimistically assuming it is not, then we can just do that. This means that even if the hidden type is Freeze, we assume it is not, which is always a safe decision, even if it means some code won't compile even though it could.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 25, 2024

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 25, 2024
@lcnr
Copy link
Contributor

lcnr commented Apr 25, 2024

As a quick note: this differs from the new solver in that the new solver instead normalizes the opaque type by looking at the opaque type storage, so the new solver may instead return ambiguity or success instead of NoSolution. The important part is that the new solver new gets query cycles here, so moving to the new solver will not cause any breakage.

Theoretically the incompleteness of the old solver may guide type inference, but as there are no trait implementations with Freeze bounds this should not happen. Even if it could, it would still be merely a theoretical concern.

@rfcbot reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False-positive "cycle detected" in const fn with RPIT
5 participants