From 3c64d6df1290e4c932030f6b66aa2a3e5d0b3285 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 8 May 2024 12:30:19 -0700 Subject: [PATCH] #[turbo_tasks::value(transparent)]: Generate docs & fail on invalid callsites (#8087) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description Most `#[turbo_tasks::value(transparent)]` types leave their inner value as private. I think this okay, but because rustdoc hides private fields by default, it makes it hard to understand at a glance the contained value, without scrolling down to the `impl` of `VcValueType`. This PR generates documentation for these. ![Screenshot 2024-05-02 at 10 27 47 PM](https://github.com/vercel/turbo/assets/180404/c3929d1f-7d96-4908-a91a-83b32c8b380a)
Also checked that it works if there's an existing doc comment, extending rather than replacing the existing documentation. ![Screenshot 2024-05-02 at 10 25 06 PM](https://github.com/vercel/turbo/assets/180404/0614d96b-f715-4eb2-b555-0fea18d14474)
**Alternative:** We could fail if the field is non-`pub`, and update the callsites. Let me know if this is preferred. The contained fields are *basically* public anyways, as they can be accessed via `Vc`'s APIs. While modifying this code, I realized that we don't generate an error if `#[turbo_tasks::value(transparent)]` would be a no-op. The second commit in this PR adds an error and updates the callsites. This also exposed some issues in the next.js repository, which are fixed in https://github.com/vercel/next.js/pull/65337 . ### Testing Instructions Closes PACK-3038 --- Cargo.lock | 1 + crates/turbo-tasks-macros/Cargo.toml | 1 + crates/turbo-tasks-macros/src/value_macro.rs | 77 ++++++++++++++----- crates/turbopack-css/src/module_asset.rs | 2 +- .../transform/swc_ecma_transform_plugins.rs | 8 +- crates/turbopack-node/src/route_matcher.rs | 2 +- .../turbopack-node/src/transforms/postcss.rs | 2 +- .../turbopack-node/src/transforms/webpack.rs | 2 +- 8 files changed, 66 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8bbf188fd27ef..d9f3b86f7a47b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10719,6 +10719,7 @@ dependencies = [ "proc-macro-error", "proc-macro2", "quote", + "regex", "syn 1.0.109", "turbo-tasks-macros-shared", ] diff --git a/crates/turbo-tasks-macros/Cargo.toml b/crates/turbo-tasks-macros/Cargo.toml index 984ad25fbe8d3..9d6509310a716 100644 --- a/crates/turbo-tasks-macros/Cargo.toml +++ b/crates/turbo-tasks-macros/Cargo.toml @@ -17,5 +17,6 @@ anyhow = { workspace = true } proc-macro-error = "1.0.4" proc-macro2 = { workspace = true } quote = { workspace = true } +regex = { workspace = true } syn = { workspace = true, features = ["full", "extra-traits", "visit-mut"] } turbo-tasks-macros-shared = { workspace = true } diff --git a/crates/turbo-tasks-macros/src/value_macro.rs b/crates/turbo-tasks-macros/src/value_macro.rs index f7d57a7696f79..c4393b2e73cde 100644 --- a/crates/turbo-tasks-macros/src/value_macro.rs +++ b/crates/turbo-tasks-macros/src/value_macro.rs @@ -1,9 +1,12 @@ +use std::sync::OnceLock; + use proc_macro::TokenStream; use proc_macro2::Ident; -use quote::quote; +use quote::{quote, ToTokens}; +use regex::Regex; use syn::{ parse::{Parse, ParseStream}, - parse_macro_input, + parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned, Error, Fields, FieldsUnnamed, Generics, Item, ItemEnum, ItemStruct, Lit, LitStr, Meta, @@ -189,7 +192,7 @@ impl Parse for ValueArguments { } pub fn value(args: TokenStream, input: TokenStream) -> TokenStream { - let item = parse_macro_input!(input as Item); + let mut item = parse_macro_input!(input as Item); let ValueArguments { serialization_mode, into_mode, @@ -198,6 +201,58 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream { transparent, } = parse_macro_input!(args as ValueArguments); + let mut inner_type = None; + if transparent { + if let Item::Struct(ItemStruct { + attrs, + fields: Fields::Unnamed(FieldsUnnamed { unnamed, .. }), + .. + }) = &mut item + { + if unnamed.len() == 1 { + let field = unnamed.iter().next().unwrap(); + inner_type = Some(field.ty.clone()); + + // generate a type string to add to the docs + let inner_type_string = inner_type.to_token_stream().to_string(); + + // HACK: proc_macro2 inserts whitespace between every token. It's ugly, so + // remove it, assuming these whitespace aren't syntatically important. Using + // prettyplease (or similar) would be more correct, but slower and add another + // dependency. + static WHITESPACE_RE: OnceLock = OnceLock::new(); + // Remove whitespace, as long as there is a non-word character (e.g. `>` or `,`) + // on either side. Try not to remove whitespace between `dyn Trait`. + let whitespace_re = WHITESPACE_RE.get_or_init(|| { + Regex::new(r"\b \B|\B \b|\B \B").expect("WHITESPACE_RE is valid") + }); + let inner_type_string = whitespace_re.replace_all(&inner_type_string, ""); + + // Add a couple blank lines in case there's already a doc comment we're + // effectively appending to. If there's not, rustdoc will strip + // the leading whitespace. + let doc_str = format!( + "\n\nThis is a [transparent value type][::turbo_tasks::value#transparent] \ + wrapping [`{}`].", + inner_type_string, + ); + + attrs.push(parse_quote! { + #[doc = #doc_str] + }); + } + } + if inner_type.is_none() { + item.span() + .unwrap() + .error( + "#[turbo_tasks::value(transparent)] is only valid with single-item unit \ + structs", + ) + .emit(); + } + } + let ident = match &item { Item::Enum(ItemEnum { ident, .. }) => ident, Item::Struct(ItemStruct { ident, .. }) => ident, @@ -211,20 +266,6 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream { } }; - let mut inner_type = None; - if transparent { - if let Item::Struct(ItemStruct { - fields: Fields::Unnamed(FieldsUnnamed { unnamed, .. }), - .. - }) = &item - { - if unnamed.len() == 1 { - let field = unnamed.iter().next().unwrap(); - inner_type = Some(&field.ty); - } - } - } - let cell_mode = match cell_mode { CellMode::New => quote! { turbo_tasks::VcCellNewMode<#ident> @@ -234,7 +275,7 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream { }, }; - let (cell_prefix, cell_access_content, read) = if let Some(inner_type) = inner_type { + let (cell_prefix, cell_access_content, read) = if let Some(inner_type) = &inner_type { ( quote! { pub }, quote! { diff --git a/crates/turbopack-css/src/module_asset.rs b/crates/turbopack-css/src/module_asset.rs index 2830f8e38b551..c622b18c1bd3b 100644 --- a/crates/turbopack-css/src/module_asset.rs +++ b/crates/turbopack-css/src/module_asset.rs @@ -108,7 +108,7 @@ impl Asset for ModuleCssAsset { /// A CSS class that is exported from a CSS module. /// /// See [`ModuleCssClasses`] for more information. -#[turbo_tasks::value(transparent)] +#[turbo_tasks::value] #[derive(Debug, Clone)] enum ModuleCssClass { Local { diff --git a/crates/turbopack-ecmascript-plugins/src/transform/swc_ecma_transform_plugins.rs b/crates/turbopack-ecmascript-plugins/src/transform/swc_ecma_transform_plugins.rs index d7aa68e969446..e75b328267dc0 100644 --- a/crates/turbopack-ecmascript-plugins/src/transform/swc_ecma_transform_plugins.rs +++ b/crates/turbopack-ecmascript-plugins/src/transform/swc_ecma_transform_plugins.rs @@ -11,13 +11,7 @@ use turbopack_ecmascript::{CustomTransformer, TransformContext}; /// Internally this contains a `CompiledPluginModuleBytes`, which points to the /// compiled, serialized wasmer::Module instead of raw file bytes to reduce the /// cost of the compilation. -#[turbo_tasks::value( - transparent, - serialization = "none", - eq = "manual", - into = "new", - cell = "new" -)] +#[turbo_tasks::value(serialization = "none", eq = "manual", into = "new", cell = "new")] pub struct SwcPluginModule( #[turbo_tasks(trace_ignore)] #[cfg(feature = "swc_ecma_transform_plugin")] diff --git a/crates/turbopack-node/src/route_matcher.rs b/crates/turbopack-node/src/route_matcher.rs index 18ef21acd1358..4341da5d97a17 100644 --- a/crates/turbopack-node/src/route_matcher.rs +++ b/crates/turbopack-node/src/route_matcher.rs @@ -1,7 +1,7 @@ use indexmap::IndexMap; use turbo_tasks::Vc; -#[turbo_tasks::value(transparent)] +#[turbo_tasks::value] #[derive(Debug, Clone)] #[serde(untagged)] pub enum Param { diff --git a/crates/turbopack-node/src/transforms/postcss.rs b/crates/turbopack-node/src/transforms/postcss.rs index 31636b701c7a1..8b242611ad2e6 100644 --- a/crates/turbopack-node/src/transforms/postcss.rs +++ b/crates/turbopack-node/src/transforms/postcss.rs @@ -36,7 +36,7 @@ use crate::{ #[derive(Debug, Serialize, Deserialize, Clone)] #[serde(rename_all = "camelCase")] -#[turbo_tasks::value(transparent, serialization = "custom")] +#[turbo_tasks::value(serialization = "custom")] struct PostCssProcessingResult { css: String, map: Option, diff --git a/crates/turbopack-node/src/transforms/webpack.rs b/crates/turbopack-node/src/transforms/webpack.rs index d5518d606a9ae..745ca87cb0cfd 100644 --- a/crates/turbopack-node/src/transforms/webpack.rs +++ b/crates/turbopack-node/src/transforms/webpack.rs @@ -54,7 +54,7 @@ use crate::{ #[derive(Debug, Serialize, Deserialize, Clone)] #[serde(rename_all = "camelCase")] -#[turbo_tasks::value(transparent, serialization = "custom")] +#[turbo_tasks::value(serialization = "custom")] struct WebpackLoadersProcessingResult { source: String, map: Option,