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

Modular canonicalizer #1058

Merged
merged 9 commits into from Nov 30, 2021
Merged

Conversation

WardBrian
Copy link
Member

@WardBrian WardBrian commented Nov 24, 2021

Resolves #1053. I think this will be good for #1049 as we can run only the deprecations setting on the test suite so we don't over-regularize.

This also lays the ground work for #1042 but I ran into some issues after fixing a bug in how we handle source positions that I need more time to run down, so I'm submitting this without that work.

Submission Checklist

Release notes

The canonicalizer can now have each part enabled seperately. This can be done with commands like stanc --auto-format --canonicalize=braces,deprecations <model>. Current options are deprecations, braces, and parenthesis. The --print-canonical flag still works, but is simply synonymous for --auto-format --canonicalize=<all possible options>

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

src/frontend/Canonicalize.ml Outdated Show resolved Hide resolved
Comment on lines +202 to +207
let repair_syntax program settings =
if settings.deprecations then
program
|> map_program
(repair_syntax_stmt (userdef_distributions program.functionblock))
else program
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm inclined to say that repair_syntax should be enabled for all formatting, not just deprecations. It's not about syntax that's going to be deprecated but syntax that has already been deprecated.. A program that "needs" repair_syntax isn't even going to typecheck without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, those programs do fail to typecheck without --print-canonical. If we don't like that we could run it if and only if the formatter is activated, I suppose.

@WardBrian
Copy link
Member Author

As @rok-cesnovar brought up in #577, it would be useful if this was also available in stancjs. Any suggestions on how that should be implemented? I think the js interface can only pass string flags, correct? So each part would need a seperate flag like "canonicalize-deprecations"?

@nhuurre
Copy link
Collaborator

nhuurre commented Nov 29, 2021

The changes to the canonicalizer look good. I'm not familiar with cram tests so maybe someone else should review those.

@rok-cesnovar
Copy link
Member

I think the js interface can only pass string flags, correct? So each part would need a seperate flag like "canonicalize-deprecations"?

It also handles "name=val" https://github.com/stan-dev/stanc3/blob/master/src/stancjs/stancjs.ml#L114

@WardBrian
Copy link
Member Author

Cram tests are described a little in the dune manual. They basically show a command (the line starting with $), its output, and the exit code (in [``]). They're great for testing command line arguments, which is all they're doing here

stanc.opam Outdated Show resolved Hide resolved
@WardBrian WardBrian merged commit 45fad64 into stan-dev:master Nov 30, 2021
@WardBrian WardBrian deleted the modular-canonicalizer branch November 30, 2021 18:57
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.

Make Canonicalizer modular
3 participants