Skip to content

remove EvaluationResult::EvaluatedTo{Unknown,Recur} - #114249

Closed
aliemjay wants to merge 1 commit into
rust-lang:masterfrom
aliemjay:evaluation-result-clean
Closed

remove EvaluationResult::EvaluatedTo{Unknown,Recur}#114249
aliemjay wants to merge 1 commit into
rust-lang:masterfrom
aliemjay:evaluation-result-clean

Conversation

@aliemjay

@aliemjayaliemjay commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

I was confused by them while reviewing #114023.

EvaluatedToRecur was introduced in #42840, seemingly with intent to support inductive cycles in the current solver, but that's not something we plan to have in the old solver. It is broken anyway because it's never converted into EvaluatedToErr in the root predicate, see the FIXME in the docs.

EvaluatedToUnknown is not handled any differently than EvaluatedtoAmbig.

r? @lcnr@compiler-errors

@rustbotrustbot 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 Jul 30, 2023
// Avoid caching results that depend on more than just the trait-ref
// - the stack can create recursion.
if result.is_stack_dependent() {
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is it sound to remove this? Aren't we caching things that should not be cached?

@aliemjay

Copy link
Copy Markdown
ContributorAuthor

as @compiler-errors noted, this example breaks under this PR because fn match_fresh_trait_refs is incomplete. See also rust-lang/trait-system-refactor-initiative#56.
Box<u16>: Trait<_> is cached as ambiguous although it clearly doesn't hold.

traitTrait<J>{}// This impl doesn't holdimpl<J>Trait<J>forBox<u32>whereBox<u16>:Trait<J>,Option<u8>:Trait<J>,{}// This impl holdsimplTrait<()>forBox<u8>{}// --- Trait2traitTrait2<J>{}// This impl doesn't holdimpl<J>Trait2<J>forBox<u32>whereBox<u16>:Trait<J>{}// This impl holdsimplTrait2<()>forBox<u8>{}fnimpls_trait1<T:Trait<J>,J>(){}fnimpls_trait2<T:Trait2<J>,J>(){}fnmain(){impls_trait1::<Box<_>,_>;impls_trait2::<Box<_>,_>;}

@rustbot author

@rustbotrustbot 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 Aug 7, 2023
@aliemjay
aliemjay marked this pull request as draft August 7, 2023 13:40
@bors

bors commented Aug 15, 2023

Copy link
Copy Markdown
Collaborator

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

@Dylan-DPC

Copy link
Copy Markdown
Member

@aliemjay any updates on this?

@Dylan-DPC

Copy link
Copy Markdown
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPCDylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-inactiveStatus: Inactive and waiting on the author. This is often applied to closed PRs.T-compilerRelevant 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

@aliemjay@bors@Dylan-DPC@compiler-errors@lcnr@rustbot