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

Fix PostBorrowckAnalysis for old solver #135899

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

branch name is typo'd

This "fixes" the PostBorrowckAnalysis mode for the old solver, and begins to use it in check_opaque_well_formed and check_coroutine_obligations.

There are several curiosities here that result.

r? lcnr

@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. labels Jan 22, 2025
@@ -36,5 +36,21 @@ note: this opaque type is in the signature
LL | type Bar = impl std::fmt::Display;
| ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors
error: the constant `N` is not of type `{type error}`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment:

// FIXME: We should reveal the TAITs that end up in where clauses here, otherwise we
// will not be able to match param-env candidates in the old solver, since we don't
// have eq-modulo-normalization. This is less of a problem than it seems, since this
// only matters if we have TAITs in where where clauses, which isn't achievable with
// RPIT anyways.

@@ -6,5 +6,21 @@ LL | async fn test<const N: crate::Bar>() {
|
= note: the only supported types are integers, `bool`, and `char`

error: aborting due to 1 previous error
error: the constant `N` is not of type `u32`
Copy link
Member Author

@compiler-errors compiler-errors Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment:

// FIXME: We should reveal the TAITs that end up in where clauses here, otherwise we
// will not be able to match param-env candidates in the old solver, since we don't
// have eq-modulo-normalization. This is less of a problem than it seems, since this
// only matters if we have TAITs in where where clauses, which isn't achievable with
// RPIT anyways.

@@ -1,7 +1,5 @@
// ICE failed to resolve instance for ...
// issue: rust-lang/rust#123145
//@ build-fail
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 now cycles in check_opaque_well_formed, since now we actually reveal impl Handler when proving that it satisfies its own item bounds. This seems desirable to me.

let generic_ty = self.cx().type_of(data.def_id);
let mut concrete_ty = generic_ty.instantiate(self.cx(), args);

if concrete_ty == ty {
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 recursiveness check sucks, but it prevents bad error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we move this check to type_of_opaque?

Ty::new_error_with_message(self.cx(), self.cause.span, "recursive opaque type");
}

let concrete_ty = fold_regions(self.cx(), concrete_ty, |re, _dbi| match re.kind() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should consolidate all of these "replace erasing with infer" so that we only need to change it in one place when we start using a real binder...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we probably should 😁

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

💀

I guess I'll minimize that ^

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2025
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits, i'd like to see a crater run here after u've figured out the ci failure

@bors
Copy link
Contributor

bors commented Jan 30, 2025

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

@compiler-errors compiler-errors force-pushed the post-borrowck-new-solver branch from 319fe69 to 9d23763 Compare March 18, 2025 23:25
}

let concrete_ty = fold_regions(self.cx(), concrete_ty, |re, _dbi| match re.kind() {
ty::ReErased => self.selcx.infcx.next_region_var_in_universe(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(((

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn unpeek<'a, F: FnMut(&mut ()) + 'a>(mut peek: F) -> impl FnMut(&mut ()) {
    move |i| peek(i)
}

pub fn test<P>(mut parser: P) -> impl FnMut(&mut ()) {
    unpeek(move |input: &mut ()| {
        let parser = &parser;
    })
}

I'll write up an analysis later.

@compiler-errors
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
…lver, r=<try>

Fix `PostBorrowckAnalysis` for old solver

~~branch name is typo'd~~

This "fixes" the `PostBorrowckAnalysis` mode for the old solver, and begins to use it in `check_opaque_well_formed` and `check_coroutine_obligations`.

There are several curiosities here that result.

r? lcnr
@bors
Copy link
Contributor

bors commented Mar 18, 2025

⌛ Trying commit 9d23763 with merge 51000a3...

@bors
Copy link
Contributor

bors commented Mar 19, 2025

☀️ Try build successful - checks-actions
Build commit: 51000a3 (51000a35bc99aeffdc092bf2226ab562eedb9eff)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-135899 created and queued.
🤖 Automatically detected try build 51000a3
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-135899 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-135899 is completed!
📊 5 regressed and 3 fixed (599834 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 20, 2025
@lcnr
Copy link
Contributor

lcnr commented Mar 21, 2025

does increasing the recursion limit fix it, if so, we should go ahead with this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants