Stream: git-wasmtime

Topic: wasmtime / PR #8263 cranelift: ISLE printer

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested wasmtime-compiler-reviewers for a review on PR #8263.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested fitzgen for a review on PR #8263.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested wasmtime-default-reviewers for a review on PR #8263.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin opened PR #8263 from mmcloughlin:cranelift-isle-printer to bytecodealliance:main:

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 pretty crate, 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 new printer feature.

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 islefmt tool, but comment representation in the AST would be required before that could be considered.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin submitted PR review.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review 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 https://github.com/rust-lang/cargo/issues/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?

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review 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.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review 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_pos as a public function is a bit distasteful. Previously I had parse_without_pos in the test binary instead, but that requires you to export Parser itself, and that seems worse.
• We can't guard parse_without_pos with #[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.rs binary, which would allow use of conditional compilation for test only.
• We could have an eq_without_pos on 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.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:28):

cfallin requested cfallin for a review on PR #8263.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:29):

cfallin commented on PR #8263:

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.

Wasmtime GitHub notifications bot (Mar 29 2024 at 02:44):

github-actions[bot] commented on PR #8263:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin submitted PR review:

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.)

Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin submitted PR review:

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.)

Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin created PR review 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.)

Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin created PR review 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.

Last updated: Nov 22 2024 at 17:03 UTC