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

exporter: refactor Clauses #661

Closed
W95Psp opened this issue May 13, 2024 · 3 comments · Fixed by #662
Closed

exporter: refactor Clauses #661

W95Psp opened this issue May 13, 2024 · 3 comments · Fixed by #662
Labels
frontend Issue in the Rust to JSON translation

Comments

@W95Psp
Copy link
Collaborator

W95Psp commented May 13, 2024

While defining Clauses I was a bit aggressive towards binders, and entirely got rid of them. This is a bad design, as pointed out by @Nadrieril in our conversation in #655.

Currently, a Clause is a ClauseKind + an identifier.

We should instead have a Clause being a struct with first a Binder<ClauseKind> and then a ClauseId, which accounts for binders.

@W95Psp W95Psp added the frontend Issue in the Rust to JSON translation label May 13, 2024
@Nadrieril
Copy link
Collaborator

This may cause issues because we don't have access to the binder when recursively converting PredicateKind. The thing I had in mind was:

enum PredicateKind {
  Clause(UnboundClause),
  ...
}

with an API to get from Predicate to Clause by mapping the binder over like rustc does. Maybe there's a simpler option.

@W95Psp
Copy link
Collaborator Author

W95Psp commented May 13, 2024

Yes, that's a good point.

To recap a bit, the issue is that if we go with a Clause object that contains a Binder<ClauseKind> and a clause identifier, then no clause identifier will be accessible from a Predicate.

If we confirm we should always work with Binder<ClauseKind>s and Binder<PredicateKind>s and never work with ClauseKinds and PredicateKinds, then I think we should really have the identifier above the Binder<...>.

Currently Predicate is defined as Binder<PredicateKind>, and we are discussing about adding Clause defined as Binder<PredicateKind>.

What about defining a Predicate (resp. Clause) as structs with both a field kind of type PredicateKind (resp. ClauseKind) and a field clause_id of type ClauseId (using a newtype as you suggest in #626)? (we could also define only one struct for those two with a generic, but that would be more confusing than anything else)

Edit: and ClauseId might be better named PredicateId

@W95Psp
Copy link
Collaborator Author

W95Psp commented May 15, 2024

Regarding our conversation in your PR @Nadrieril, I'm not crazy: there was a renaming from Clause to ClauseKind in Rustc... Clauses used not to have binders at all!

In commit fca56a8, the type Clause has been renamed as ClauseKind, then commit 21226e makes Clause a Binder<ClauseKind>, essentially (with an indirection via PredicateKind and an invariant where the
predicate kind has to be a ty::PredicateKind::Clause(ClauseKind)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issue in the Rust to JSON translation
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants