Remove dependency on `rustfmt` installation for consumers #221

mbrobbel · 2023-03-15T10:15:16Z

This PR adds two features to allow controlling pretty printing in the TypeSpace::ToString impl.

rustfmt: use the existing behavior with rustfmt-wrapper
prettyplease: use prettyplease to pretty print the token stream

This is a breaking change because it changes the default behavior (no pretty printing). This makes using this crate easier in environments where no rustfmt is available (e.g. apache/datafusion-python#261).

ahl · 2023-03-15T16:21:58Z

Thanks for filing this; this is an interesting interface to consider.

In general, I like that the default for typify (and progenitor) is to use rustfmt for its code output that I see. I recognize, however, that 1. imposes a transitive dependency and 2. isn't important for code emitted, say, by a build.rs. I think, perhaps, that the right answer is for typify to not format code. We could remove the impl ToString entirely and require that consumers use ToTokens::to_tokens(). We could update the build.rs examples to do something like this:

let contents = prettyplease::unparse(parse2::<syn::File>(type_space.to_tokens()).unwrap());

That would probably make the most sense for the use case in the substrait build.rs file. I like this approach because it lets the consumer choose the formatting that makes the most sense for their use case.

The cargo-typify command (#204) can use rustfmt by default, but could grow some additional options (--prettyplease and --unformatted). I'd like the various test cases to render code with rustfmt, but that can just be a dev-dependency.

Please let me know what you think of this alternative direction. If you're interested in working on it, please adapt this PR. If not, please close it and I'll implement it this week.

mbrobbel · 2023-03-15T20:13:38Z

Yes I agree that's a better approach. I removed the ToString impl for TypeSpace and removed the rustfmt-wrapper dependency. Is that fine or should it be deprecated first?

New dependency tree

cargo tree
example-build v0.0.0 (typify/example-build)
├── serde v1.0.155
│   └── serde_derive v1.0.155 (proc-macro)
│       ├── proc-macro2 v1.0.52
│       │   └── unicode-ident v1.0.8
│       ├── quote v1.0.24
│       │   └── proc-macro2 v1.0.52 (*)
│       └── syn v1.0.109
│           ├── proc-macro2 v1.0.52 (*)
│           ├── quote v1.0.24 (*)
│           └── unicode-ident v1.0.8
└── serde_json v1.0.94
    ├── itoa v1.0.1
    ├── ryu v1.0.9
    └── serde v1.0.155 (*)
[build-dependencies]
├── prettyplease v0.1.25
│   ├── proc-macro2 v1.0.52 (*)
│   └── syn v1.0.109 (*)
├── schemars v0.8.12
│   ├── dyn-clone v1.0.5
│   ├── schemars_derive v0.8.12 (proc-macro)
│   │   ├── proc-macro2 v1.0.52 (*)
│   │   ├── quote v1.0.24 (*)
│   │   ├── serde_derive_internals v0.26.0
│   │   │   ├── proc-macro2 v1.0.52 (*)
│   │   │   ├── quote v1.0.24 (*)
│   │   │   └── syn v1.0.109 (*)
│   │   └── syn v1.0.109 (*)
│   ├── serde v1.0.155 (*)
│   ├── serde_json v1.0.94 (*)
│   └── uuid v1.3.0
│       └── serde v1.0.155 (*)
├── serde_json v1.0.94 (*)
├── syn v1.0.109 (*)
└── typify v0.0.11-dev (typify/typify)
    ├── typify-impl v0.0.11-dev (typify/typify-impl)
    │   ├── heck v0.4.1
    │   ├── log v0.4.17
    │   │   └── cfg-if v1.0.0
    │   ├── proc-macro2 v1.0.52 (*)
    │   ├── quote v1.0.24 (*)
    │   ├── regress v0.5.0
    │   │   ├── hashbrown v0.13.2
    │   │   │   └── ahash v0.8.3
    │   │   │       ├── cfg-if v1.0.0
    │   │   │       └── once_cell v1.17.1
    │   │   │       [build-dependencies]
    │   │   │       └── version_check v0.9.4
    │   │   └── memchr v2.4.1
    │   ├── schemars v0.8.12 (*)
    │   ├── serde_json v1.0.94 (*)
    │   ├── syn v1.0.109 (*)
    │   ├── thiserror v1.0.39
    │   │   └── thiserror-impl v1.0.39 (proc-macro)
    │   │       ├── proc-macro2 v1.0.52 (*)
    │   │       ├── quote v1.0.24 (*)
    │   │       └── syn v1.0.109 (*)
    │   └── unicode-ident v1.0.8
    │   [dev-dependencies]
    │   ├── expectorate v1.0.6
    │   │   ├── console v0.15.3
    │   │   │   ├── lazy_static v1.4.0
    │   │   │   ├── libc v0.2.124
    │   │   │   └── unicode-width v0.1.9
    │   │   ├── newline-converter v0.2.2
    │   │   │   └── unicode-segmentation v1.10.0
    │   │   └── similar v2.2.1
    │   ├── paste v1.0.12 (proc-macro)
    │   ├── prettyplease v0.1.25 (*)
    │   ├── rustfmt-wrapper v0.2.0
    │   │   ├── serde v1.0.155 (*)
    │   │   ├── tempfile v3.3.0
    │   │   │   ├── cfg-if v1.0.0
    │   │   │   ├── fastrand v1.7.0
    │   │   │   ├── libc v0.2.124
    │   │   │   └── remove_dir_all v0.5.3
    │   │   ├── thiserror v1.0.39 (*)
    │   │   ├── toml v0.5.9
    │   │   │   └── serde v1.0.155 (*)
    │   │   └── toolchain_find v0.2.0
    │   │       ├── home v0.5.3
    │   │       ├── lazy_static v1.4.0
    │   │       ├── regex v1.6.0
    │   │       │   ├── aho-corasick v0.7.18
    │   │       │   │   └── memchr v2.4.1
    │   │       │   ├── memchr v2.4.1
    │   │       │   └── regex-syntax v0.6.27
    │   │       ├── semver v0.11.0
    │   │       │   └── semver-parser v0.10.2
    │   │       │       └── pest v2.1.3
    │   │       │           └── ucd-trie v0.1.3
    │   │       └── walkdir v2.3.2
    │   │           └── same-file v1.0.6
    │   ├── schema v0.0.1
    │   │   ├── proc-macro2 v1.0.52 (*)
    │   │   ├── quote v1.0.24 (*)
    │   │   ├── schema-derive v0.0.1 (proc-macro)
    │   │   │   ├── paste v1.0.12 (proc-macro)
    │   │   │   ├── proc-macro2 v1.0.52 (*)
    │   │   │   ├── quote v1.0.24 (*)
    │   │   │   └── syn v1.0.109 (*)
    │   │   └── syn v1.0.109 (*)
    │   ├── schemars v0.8.12 (*)
    │   ├── serde v1.0.155 (*)
    │   └── uuid v1.3.0 (*)
    └── typify-macro v0.0.11-dev (proc-macro) (typify/typify-macro)
        ├── proc-macro2 v1.0.52 (*)
        ├── quote v1.0.24 (*)
        ├── schemars v0.8.12 (*)
        ├── serde v1.0.155 (*)
        ├── serde_json v1.0.94 (*)
        ├── serde_tokenstream v0.1.7
        │   ├── proc-macro2 v1.0.52 (*)
        │   ├── serde v1.0.155 (*)
        │   └── syn v1.0.109 (*)
        ├── syn v1.0.109 (*)
        └── typify-impl v0.0.11-dev (typify/typify-impl) (*)
    [dev-dependencies]
    ├── expectorate v1.0.6 (*)
    ├── glob v0.3.1
    ├── quote v1.0.24 (*)
    ├── regress v0.5.0 (*)
    ├── rustfmt-wrapper v0.2.0 (*)
    ├── schemars v0.8.12 (*)
    ├── serde v1.0.155 (*)
    ├── serde_json v1.0.94 (*)
    ├── trybuild v1.0.79
    │   ├── basic-toml v0.1.1
    │   │   └── serde v1.0.155 (*)
    │   ├── glob v0.3.1
    │   ├── once_cell v1.17.1
    │   ├── serde v1.0.155 (*)
    │   ├── serde_derive v1.0.155 (proc-macro) (*)
    │   ├── serde_json v1.0.94 (*)
    │   └── termcolor v1.1.3
    └── uuid v1.3.0 (*)

example-macro v0.0.0 (typify/example-macro)
├── serde v1.0.155 (*)
├── serde_json v1.0.94 (*)
└── typify v0.0.11-dev (typify/typify) (*)

typify v0.0.11-dev (typify/typify) (*)

typify-impl v0.0.11-dev (typify/typify-impl) (*)

typify-macro v0.0.11-dev (proc-macro) (typify/typify-macro) (*)

typify-test v0.0.0 (typify/typify-test)
├── regress v0.5.0 (*)
├── serde v1.0.155 (*)
└── serde_json v1.0.94 (*)
[build-dependencies]
├── ipnetwork v0.20.0
│   ├── schemars v0.8.12 (*)
│   └── serde v1.0.155 (*)
├── prettyplease v0.1.25 (*)
├── schemars v0.8.12 (*)
├── serde v1.0.155 (*)
├── syn v1.0.109 (*)
└── typify v0.0.11-dev (typify/typify) (*)

ahl

This is not a strongly held opinion, but I'd prefer that test_generation only use the rustfmt method, because I think that's the right approach for generators (such as progenitor) when they emit code into files.

Can you please add a note to the CHANGELOG.adoc? I think this may warrant a section on what to do if you're using to_string(). In particular, I'd suggest people use to_stream() and then to_string if no one is going to see the output, prettyplease for build.rs output or code generation that's ephemeral, but that humans might want to look at to debug, and rustfmt-wrapper for persistent generation such as code that's going to live in a crate.

Thanks so much for doing this.

jayvdb · 2023-03-16T00:56:35Z

Fwiw, I use prettyplease followed by rustfmt to clean up the generated code. rustfmt does a really bad job for formatting generated code - it ignores lots of it - there are some tips to see how bad at rust-lang/rustfmt#5683 .
After prettyplease has reformatted the generated code, rustfmt does a much better job.

ahl

I'm happy to merge this as-in, but if you're willing to make those prettyplease changes to the build.rs, add a section to the README about formatting, and update the changelog to point to it, that would be great. If you're done... just let me know and I'll merge this.

Thanks again!

CHANGELOG.adoc

example-build/Cargo.toml

example-build/build.rs

typify-test/build.rs

ahl

So good. Thank you!!

…please` (#73) The use of `typify` assumed that `rustfmt` was installed... which turned out not to be a great assumption. We've modified `typify` to remove the dependency on `rustfmt-wrapper` and have removed the interface that used it `ToString::to_string()`. Instead we recommend that consumers use `prettyplease` for `build.rs` uses such as the one in this crate. See oxidecomputer/typify#221 Alternatively, the `build.rs` could just emit the tokens unformatted (to remove the build-time dependency on `prettyplease` and `syn`), but that seems annoying if and when you need to look at the generated code. FWIW `syn` is an existing dependency; `prettyplease` is the only new new crate I see in `Cargo.lock`. I can share the full diff between the old and new versions of the `substrait_text.rs`, but here's a sample: ```diff @@ -1593,22 +1831,27 @@ T: std::convert::TryInto<Option<super::SessionDependent>>, T::Error: std::fmt::Display, { - self.session_dependent = value.try_into().map_err(|e| { - format!( - "error converting supplied value for session_dependent: {}", - e - ) - }); self + .session_dependent = value + .try_into() + .map_err(|e| { + format!( + "error converting supplied value for session_dependent: {}", e + ) + }); + self } pub fn variadic<T>(mut self, value: T) -> Self where T: std::convert::TryInto<Option<super::VariadicBehavior>>, T::Error: std::fmt::Display, { - self.variadic = value + self + .variadic = value .try_into() - .map_err(|e| format!("error converting supplied value for variadic: {}", e)); + .map_err(|e| { + format!("error converting supplied value for variadic: {}", e) + }); self } pub fn window_type<T>(mut self, value: T) -> Self ```

Add rustfmt and prettyplease features

8be8037

mbrobbel mentioned this pull request Mar 15, 2023

Add conda.yaml baseline workflow file apache/datafusion-python#281

Merged

jdye64 mentioned this pull request Mar 15, 2023

Add arrow datafusion python dependency dask-contrib/dask-sql#1085

Merged

ahl mentioned this pull request Mar 15, 2023

reconsider the use of rustfmt-wrapper ahl/codegen-template#1

Open

Remove impl ToString for TypeSpace and rustfmt-wrapper dependency

cced301

ahl requested changes Mar 16, 2023

View reviewed changes

mbrobbel added 7 commits March 16, 2023 09:17

Use rustfmt instead of prettyplease

103506e

Add changelog entry

c1944da

Fix code blocks

e7e2bdb

Fix path

721a5f1

Merge branch 'main' into rustfmt

9702468

Reflect removal of ToString in cargo-typify

11e7f58

Remove syn from typify-test build deps

5af168c

ahl reviewed Mar 17, 2023

View reviewed changes

CHANGELOG.adoc Show resolved Hide resolved

example-build/Cargo.toml Outdated Show resolved Hide resolved

example-build/build.rs Outdated Show resolved Hide resolved

typify-test/build.rs Outdated Show resolved Hide resolved

ahl changed the title ~~Add rustfmt and prettyplease features to control TypeSpace::ToString impl~~ Remove dependency on rustfmt installation for consumers Mar 17, 2023

Use prettyplease in examples and add Formatting section to README

75c64fe

mbrobbel requested a review from ahl March 17, 2023 10:01

ahl approved these changes Mar 17, 2023

View reviewed changes

ahl merged commit 33832b9 into oxidecomputer:main Mar 17, 2023

mbrobbel deleted the rustfmt branch March 17, 2023 17:40

This was referenced Mar 17, 2023

remove implicit dependency for transitive consumers on rustfmt installation oxidecomputer/progenitor#362

Closed

[Merged by Bors] - chore: replace removed typfify::TypeSpace::to_string() with prettyplease substrait-io/substrait-rs#73

Closed

mbrobbel mentioned this pull request Apr 11, 2023

Build fails: error: failed to run custom build command for substrait v0.4.2 substrait-io/substrait-rs#92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on `rustfmt` installation for consumers #221

Remove dependency on `rustfmt` installation for consumers #221

mbrobbel commented Mar 15, 2023

ahl commented Mar 15, 2023

mbrobbel commented Mar 15, 2023

ahl left a comment

jayvdb commented Mar 16, 2023

ahl left a comment

ahl left a comment

Remove dependency on rustfmt installation for consumers #221

Remove dependency on rustfmt installation for consumers #221

Conversation

mbrobbel commented Mar 15, 2023

ahl commented Mar 15, 2023

mbrobbel commented Mar 15, 2023

ahl left a comment

Choose a reason for hiding this comment

jayvdb commented Mar 16, 2023

ahl left a comment

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

Remove dependency on `rustfmt` installation for consumers #221

Remove dependency on `rustfmt` installation for consumers #221