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

Add synterp phase support for derive plugins. #597

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

rlepigre
Copy link
Contributor

@rlepigre rlepigre commented Feb 15, 2024

This was developed and tested against our own derive commands (included in this branch). It would probably make sense to test this in the repo somehow though, maybe by adding a new deriver that creates some kind of module?

CC @Janno.

Edit:

We had to require a synterp phase for all derivers since we need to know about all of them upfront (the dep1 arguments are not enough to compute the full list, since independent derivers have no dep1). This does not seem too bad though.

We tweaked the module wrapping behaviour of derive in the following way:

  • Using derive Inductive ty := ... does not automatically create a module ty anymore.
    • The original behaviour can be recovered with #[module] derive Inductive ty := ....
    • A module name can be specified with #[module="Ty"] derive Inductive ty := ....
  • A custom prefix can be configured with the prefix (string) attribute.
  • When a wrapper module is generated, the no_alias (boolean) attribute can be used to avoid generating alias notations.

apps/derive/elpi/derive.elpi Outdated Show resolved Hide resolved
@gares
Copy link
Contributor

gares commented Feb 19, 2024

  • Using derive Inductive ty := ... does not automatically create a module ty anymore.

    • The original behaviour can be recovered with #[module] derive Inductive ty := ....
    • A module name can be specified with #[module="Ty"] derive Inductive ty := ....
  • A custom prefix can be configured with the prefix (string) attribute.

please put this text in the README.md (and fix the example there).

apps/derive/elpi/derive.elpi Outdated Show resolved Hide resolved
apps/derive/elpi/derive.elpi Outdated Show resolved Hide resolved
Comment on lines +50 to +56
topo [] _ [] :- !.
topo L Deps SL :-
std.partition L (not-a-src Deps) LNoDeps Other,
if (LNoDeps = []) (coq.error "derive: no topological order:" L Deps) true,
std.filter Deps (tgt-is-not-in LNoDeps) NewDeps,
topo Other NewDeps SOther,
std.append LNoDeps SOther SL.
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO (for me): there is a coq.elpi.toposort but for a full graph. Fix/adapt to this use case.

@rlepigre
Copy link
Contributor Author

Note: I fixed the generation of the alias for the type in the case where there is a wrapper module. It was named after the wrapper module name, instead of the type name.

I also took this opportunity to add a no_alias attribute that disables the generation of the aliases.

@rlepigre
Copy link
Contributor Author

please put this text in the README.md (and fix the example there).

@gares I have given it a quick try, and it seems to me like the README.md file is not in a terribly good shape right now. I'd be surprised if the code it contains fully works on master (I found a few typos). I'm not sure how best to fix this.

@gares
Copy link
Contributor

gares commented Feb 23, 2024

Thanks. The code should be copy/pasted from readme.v which you edited to add the #[module] attribute and that edit prompted for an update. The README says it generates a module, that now is optional.

NOTE: the [derivation] predicate now has an extra boolean argument
indicating whether the derivation has a non-trivial synterp phase
(it is only relevant for recursive derivation). There is no such
boolean in the synterp-level counterpart of [derivation].
@rlepigre
Copy link
Contributor Author

rlepigre commented Mar 6, 2024

I took care of adding a bit of documentation to apps/derive/README.md, and also squashed commits since they did not really make sense anymore on their own with all the bug fixes we pushed.

@gares gares merged commit f911a33 into LPCIC:master Mar 11, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants