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

Separate clauses and goals from the Predicate type #107250

Open
5 of 10 tasks
oli-obk opened this issue Jan 24, 2023 · 8 comments
Open
5 of 10 tasks

Separate clauses and goals from the Predicate type #107250

oli-obk opened this issue Jan 24, 2023 · 8 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jan 24, 2023

Split up the PredicateKind type into Goal and Clauses similar (or exactly :D) like what chalk does. The TLDR is that ParamEnv only contains a small set of PrecicateKind variants and we want to encode this in the type system by only returning Clauses from it.

This projection has been accepted as an MCP: rust-lang/compiler-team#531

Implementation steps

  • [SP] Split out Trait, TypeOutlives, RegionOutlives and Projection into Clause enum: #104846
  • [SP] #104911 inferred_outlives_crate returns predicates, but they are always only TypeOutlives or RegionOutlives
    • may kill perf, so let's see if it works to just return ty::Clause where it now returns ty::Predicate and build the predicates at the caller site of inferred_outlives_of
  • doing the same thing for explicit_predicates_of is gonna be a bit more involved
  • Improve perf of #104846
  • start with gather_explicit_predicates_of and convert at the caller sites
  • Bounds::predicates should return an iterator over Clauses (Make Bound::predicates use Clause #112734)

Follow-up work after #112938:

  • Investigate making TypeFoldable<TyCtxt<'tcx>> for ty::Clause<'tcx> implementation less weird:
    .expect("no sensible folder would do this"))
  • Clean up the elaborator since it should only be emitting child clauses, not predicates
  • Rename identifiers like pred and predicates to clause if they're actually clauses around the codebase
  • Validate that all of the ToPredicate impls are acutally still needed, or prune them if they're not

leave a comment when you start working on one of these steps so that no one else duplicates work.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Jan 24, 2023
@obeis
Copy link
Contributor

obeis commented Jan 24, 2023

@rustbot claim

@spastorino
Copy link
Member

spastorino commented Jan 24, 2023

@oli-obk thanks for opening this, I'd be happy to review and/or help/mentor if needed.

@obeis feel free to reach out on Zulip.

@spastorino spastorino changed the title Seperate clauses and goals from the Predicate type Separate clauses and goals from the Predicate type Jan 24, 2023
@obeis obeis removed their assignment Jan 29, 2023
@GentBinaku
Copy link

@rustbot claim

@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 1, 2023

PredicateKind::ConstEvaluatable and PredicateKind::WellFormed can all appear in the param_env/predicates_of so should be moved to Clause as well (PredicateKind::ConstEquate might too although we might also end up removing that so 🤷‍♀️)

@spastorino
Copy link
Member

@GentBinaku are you still actively working on this?

@Almugra Almugra removed their assignment May 29, 2023
@dswij
Copy link
Member

dswij commented May 31, 2023

@rustbot claim

@compiler-errors compiler-errors removed the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jun 15, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 18, 2023
…compiler-errors

Make `Bound::predicates`  use `Clause`

Part of rust-lang#107250

`Bound::predicates` returns an iterator over `Binder<_, Clause>` instead of `Predicate`.

I tried updating `explicit_predicates_of` as well, but it seems that it needs a lot more change than I thought. Will do it in a separate PR instead.
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 18, 2023
…compiler-errors

Make `Bound::predicates`  use `Clause`

Part of rust-lang#107250

`Bound::predicates` returns an iterator over `Binder<_, Clause>` instead of `Predicate`.

I tried updating `explicit_predicates_of` as well, but it seems that it needs a lot more change than I thought. Will do it in a separate PR instead.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 18, 2023
…compiler-errors

Make `Bound::predicates`  use `Clause`

Part of rust-lang#107250

`Bound::predicates` returns an iterator over `Binder<_, Clause>` instead of `Predicate`.

I tried updating `explicit_predicates_of` as well, but it seems that it needs a lot more change than I thought. Will do it in a separate PR instead.
@compiler-errors compiler-errors removed E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Jun 19, 2023
@compiler-errors compiler-errors removed their assignment Aug 11, 2023
@zirconium-n
Copy link
Contributor

@rustbot claim

@zirconium-n
Copy link
Contributor

I think I'm a little bit unsure about the task.

Validate that all of the ToPredicate impls are acutally still needed, or prune them if they're not.

What count as an unneeded impl? Some impls can be removed without error, but theres one gray area I'm not sure. That is the impl is not there to satisfy a trait bound but just a helper function. Does this count the impl needed or not? We can just inline the impl body?

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 20, 2023
…unused-to-predicate, r=oli-obk

clean up unneeded `ToPredicate` impls

Part of rust-lang#107250.
Removed all totally unused impls. And inlined two impls not need to satisify trait bound.

r? `@oli-obk`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2023
Rollup merge of rust-lang#115566 - zirconium-n:issue-107250-clean-up-unused-to-predicate, r=oli-obk

clean up unneeded `ToPredicate` impls

Part of rust-lang#107250.
Removed all totally unused impls. And inlined two impls not need to satisify trait bound.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

9 participants