Skip to content

Commit

Permalink
#[turbo_tasks::value(transparent)]: Generate docs & fail on invalid c…
Browse files Browse the repository at this point in the history
…allsites (#8087)

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

<details>
<summary>Also checked that it works if there's an existing doc comment, extending rather than replacing the existing documentation.</summary>

![Screenshot 2024-05-02 at 10 25 06 PM](https://github.com/vercel/turbo/assets/180404/0614d96b-f715-4eb2-b555-0fea18d14474)
</details>

**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 vercel/next.js#65337 .

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->


Closes PACK-3038
  • Loading branch information
bgw committed May 8, 2024
1 parent 2ce4f75 commit 3c64d6d
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 29 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/turbo-tasks-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
77 changes: 59 additions & 18 deletions crates/turbo-tasks-macros/src/value_macro.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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<Regex> = 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,
Expand All @@ -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>
Expand All @@ -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! {
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-css/src/module_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-node/src/route_matcher.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-node/src/transforms/postcss.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-node/src/transforms/webpack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
Expand Down

0 comments on commit 3c64d6d

Please sign in to comment.