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

Eagerly instantiate Fn-like obligations in old solver #108918

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 8, 2023

Alternative approach to (actually) fixing #108832. Mostly just up for demonstration, probably needs some cleaning + re-consolidation into a helper fn like the one I removed.

cc @jackh726's comment #108834 (comment) -- was this what you were asking for?

The only problem with this approach is that you can construct higher-ranked Fn obligations with GATs that relies on the "don't normalize GATs with escaping bound vars" behavior to fall back to relating projections via substs. See test tests/ui/generic-associated-types/issue-93340.rs (#93340) which fails to compile, so this would need a types FCP to confirm we're ok with the breakage :)

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2023

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 8, 2023
@compiler-errors
Copy link
Member Author

I guess I could crater run this to see if any GAT usages in production have fallout...

@bors try

@compiler-errors
Copy link
Member Author

r? @jackh726

@rustbot rustbot assigned jackh726 and unassigned eholk Mar 8, 2023
@bors
Copy link
Contributor

bors commented Mar 8, 2023

⌛ Trying commit d2087a9 with merge 34cf026bde0f1d85725f44f69476992693389da0...

@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Try build successful - checks-actions
Build commit: 34cf026bde0f1d85725f44f69476992693389da0 (34cf026bde0f1d85725f44f69476992693389da0)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Try build successful - checks-actions
Build commit: 34cf026bde0f1d85725f44f69476992693389da0 (34cf026bde0f1d85725f44f69476992693389da0)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-108918 created and queued.
🤖 Automatically detected try build 34cf026bde0f1d85725f44f69476992693389da0
🔍 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-108918 is now running

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

@craterbot
Copy link
Collaborator

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

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ 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 10, 2023
@compiler-errors
Copy link
Member Author

Crater results look boring. I'd inclined to say that the only UI test that fails (the GATs example) really shouldn't have ever compiled, since if it didn't have escaping bound vars, we would've normalized it to an inference variable.

@jackh726
Copy link
Member

"Shouldn't have compiled" and "managed to compile because it didn't hit any known bugs" are two different things, and that issue seems to fall in the latter.

I've looked over this, but I need a little bit more time to really think about this.

@compiler-errors
Copy link
Member Author

compiler-errors commented Mar 10, 2023

Yeah, and I think that test shouldn't have compiled.

A corresponding projection test without lifetimes (link) doesn't compile, and I think that, while unfortunate, that is correct behavior. The fact that we eagerly relate projections via substs when they have escaping bound vars is not correct, imo, and this test breaks with the current implementation of lazy norm as well (link).

@jackh726
Copy link
Member

jackh726 commented Apr 9, 2023

@compiler-errors I need to look at this more and think harder about it, but in the meantime, can you add a test that compiles now and fails with this PR?

@apiraino
Copy link
Contributor

apiraino commented May 3, 2023

Switching to waiting on author after the previous comment. Michael, feel free to request a review with @rustbot ready :-) Thanks!

@rustbot author

@rustbot rustbot 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 May 3, 2023
@compiler-errors
Copy link
Member Author

don't have time to continue working on this, sorry 😅

@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 1, 2023
@compiler-errors compiler-errors deleted the eager-instantiate-fn branch August 11, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants