[Merged by Bors] - chore: replace removed typfify::TypeSpace::to_string() with prettyplease

[ahl]

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:

@@ -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

[CLAassistant]

All committers have signed the CLA.

[ahl]

I see that CI has failed, but I'm not sure how to fix it.

[mbrobbel]

Thanks for this!

Regarding the failed check: https://github.com/substrait-io/substrait-rs/blob/main/CONTRIBUTING.md#pull-requests

[mbrobbel]

We just need the parsing feature of syn, and it seems version 2 was just released:

Suggested change

	syn = "1.0.109"
	syn = { version = "2.0.2", default-features = false, features = ["parsing"] }

[ahl]

prettyplease uses syn 1 so I didn't think this would work, and indeed it didn't:

error[E0308]: `?` operator has incompatible types
  --> build.rs:74:36
   |
74 |             prettyplease::unparse(&syn::parse2::<syn::File>(type_space.to_stream())?),
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `syn::file::File`, found struct `syn::File`
   |
   = note: `?` operator cannot convert from `syn::File` to `syn::file::File`
   = note: struct `syn::File` and struct `syn::file::File` have similar names, but are actually distinct types
note: struct `syn::File` is defined in crate `syn`
  --> /Users/ahl/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-2.0.2/src/file.rs:3:1
   |
3  | / ast_struct! {
4  | |     /// A complete file of Rust source code.
5  | |     ///
6  | |     /// Typically `File` objects are created with [`parse_file`].
...  |
85 | |     }
86 | | }
   | |_^
note: struct `syn::file::File` is defined in crate `syn`
  --> /Users/ahl/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.109/src/file.rs:3:1
   |
3  | / ast_struct! {
4  | |     /// A complete file of Rust source code.
5  | |     ///
6  | |     /// *This type is available only if Syn is built with the `"full"` feature.*
...  |
85 | |     }
86 | | }
   | |_^
   = note: perhaps two different versions of crate `syn` are being used?
   = note: this error originates in the macro `strip_attrs_pub` which comes from the expansion of the macro `ast_struct` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0308`.
error: could not compile `substrait` due to previous error

I'll change syn to only use the parsing feature... but prettyplease already uses full so I don't think there will be a material change in build times.

[ahl]

Actually, it turns out that syn::File requires "full" which is the default. "parsing" is insufficient.

https://docs.rs/syn/1.0.109/syn/struct.File.html

[ahl]

Let me know if there's something you need me to do to get this merged

[mbrobbel]

Let me know if there's something you need me to do to get this merged

To fix the failing check can you please update the PR title to follow Conventional Commits. Maybe something like:

chore: replace removed `typfify::TypeSpace::to_string()` with `prettyplease`

@westonpace can you please help review this?

[westonpace]

bors try

[bors]

try

Build succeeded:

• Package
• Test (pbjson)
• Check (default)
• Test (semver,serde)
• Test (default)
• SPDX License Header
• Formatting
• Check (pbjson)
• Rustfmt
• Clippy (pbjson)
• Check (protoc)
• Check (semver,serde)
• Clippy (default)
• Clippy (semver,serde)
• Conventional Commits
• Rustdoc

[westonpace]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• Check (default)
• Check (pbjson)
• Check (protoc)
• Check (semver,serde)
• Clippy (default)
• Clippy (pbjson)
• Clippy (semver,serde)
• Conventional Commits
• Formatting
• Package
• Rustdoc
• Rustfmt
• SPDX License Header
• Test (default)
• Test (pbjson)
• Test (semver,serde)