cranelift: ISLE printer #8263
cranelift: ISLE printer #8263
Conversation
| @@ -8,12 +8,17 @@ readme = "../README.md" | |||
| repository = "https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/isle" | |||
| version = "0.107.0" | |||
|
|
|||
| [[test]] | |||
| name = "printer_tests" | |||
| required-features = ["printer"] | |||
There was a problem hiding this comment.
Under this implementation, I believe the printer tests would not be run by default in CI.
It would be nice if cargo supported some kind of "default test features" configuration, but as far as I can tell this isn't possible. Nor is there much appetite for adding it according to rust-lang/cargo#2911.
Perhaps if we cared enough we could edit the Github actions workflows to invoke these tests with the right --feature argument. But having poked around the CI config a little, it's not clear how to do that without causing a mess.
Another option is to have the printer tests in a different crate that can depend on this one with the right feature set.
All that said, I'm not sure it's worth it just for these tests, but I wanted to mention it in case you had a simple suggestion for how to run them?
There was a problem hiding this comment.
I considered the alternative of adding this to the existing run_tests.rs instead. This would have required sprinkling #[cfg(feature = "printer")] everywhere, so I settled on the alternative of a separate test binary with required-features = ["printer"].
Either works though. Please let me know if you have a preference.
| /// Parse without positional information. Provided mainly to support testing, to | ||
| /// enable equality testing on structure alone. | ||
| pub fn parse_without_pos(lexer: Lexer) -> Result<Defs> { | ||
| let mut parser = Parser::new(lexer); | ||
| parser.disable_pos(); | ||
| parser.parse_defs() | ||
| } | ||
|
|
There was a problem hiding this comment.
I really don't like this, but I also couldn't think of a better way. Please let me know if you have an alternative suggestion.
- Having
parse_without_posas a public function is a bit distasteful. Previously I hadparse_without_posin the test binary instead, but that requires you to exportParseritself, and that seems worse. - We can't guard
parse_without_poswith#[cfg(test)]because then it's not visible to an integration test binary. - Potentially the printer tests could be structured as unit tests instead of modeled off the
run_tests.rsbinary, which would allow use of conditional compilation for test only. - We could have an
eq_without_poson AST types instead. However, I assumed that would require a lot of hand- or macro-generated boilerplate.
If parse_without_pos really is the best option, I also wondered if it would be nicer to instead augment the parse method with a Positions enum argument to configure whether positions are populated or dropped.
There was a problem hiding this comment.
A final option: a helper that maps through all AST nodes and sets positions to e.g. zero? That seems easiest to keep up to date and least likely to be subtly incorrect (vs. hand-written equality predicates), especially given compile errors on new AST node kinds.
|
Adding myself as a reviewer; happy to take a look at this at the beginning of next week (thanks!). For context for folks, this comes out of the ISLE-based Cranelift verification project. |
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Thanks -- a few more thoughts below.
After reading through printer.rs itself, I'm not super-convinced of the need for a library crate to help here -- certainly it's nice to use prebuilt helpers, but printing S-expressions is still not too complex to do by hand (even with indenting); if you don't mind, I think it'd be better to avoid the dependency.
(There are also additional steps we have to go through if we want to add a new dep -- vetting, etc -- and especially after recent events I'm personally going to have a higher bar for that too.)
| /// Parse without positional information. Provided mainly to support testing, to | ||
| /// enable equality testing on structure alone. | ||
| pub fn parse_without_pos(lexer: Lexer) -> Result<Defs> { | ||
| let mut parser = Parser::new(lexer); | ||
| parser.disable_pos(); | ||
| parser.parse_defs() | ||
| } | ||
|
|
There was a problem hiding this comment.
A final option: a helper that maps through all AST nodes and sets positions to e.g. zero? That seems easiest to keep up to date and least likely to be subtly incorrect (vs. hand-written equality predicates), especially given compile errors on new AST node kinds.
| } | ||
|
|
||
| /// Dump AST node to standard output. | ||
| pub fn dump<N: Printable>(node: &N) -> Result<(), Errors> { |
There was a problem hiding this comment.
If possible, I'd rather not have an explicit "... to stdout" in the API -- that's something that the caller can easily wire up, and there's otherwise no particular reason to hardcode one special Writer in an API like this. (In general, bad form to reach for "ambient capabilities" rather than those that are passed in, in core libraries, IMHO.)
This PR adds a printer to the ISLE library. This is intended to support tooling which generates ISLE source code. In such cases, this printer library allows applications to work with ISLE's AST types rather than raw string manipulation, and produce more readable output due to the pretty-printing features.
Printing is implemented using the
prettycrate, which in turn is an implementation of Wadler's classic Haskell pretty printer for Rust. In order to avoid required dependencies the printer module is guarded by a newprinterfeature.The printer is tested with a new integration test that verifies the print-parse roundtrip property for provided ISLE files.
In the future, it's possible that an ISLE printer could be used to provide an
islefmttool, but comment representation in the AST would be required before that could be considered.