Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark Vcs as must-use #5387

Merged
merged 3 commits into from
Jun 30, 2023
Merged

Mark Vcs as must-use #5387

merged 3 commits into from
Jun 30, 2023

Conversation

wbinnssmith
Copy link
Member

This uses the #[must-use] macro [0] to mark Vcs as needing to be used when created. The compiler creates warnings for each case.

Test Plan: Create a Vc in code and verify a warning is shown when Turbopack is built.

[0] https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute

This uses the `#[must-use]` macro [0] to mark Vcs as needing to be used when created. The compiler creates warnings for each case.

Test Plan: Create a Vc in code and verify a warning is shown when Turbopack is built.

[0] https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
@wbinnssmith wbinnssmith requested a review from a team as a code owner June 26, 2023 22:01
@vercel
Copy link

vercel bot commented Jun 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-cra-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jun 29, 2023 9:21pm
10 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm
turbo-site ⬜️ Ignored (Inspect) Visit Preview Jun 29, 2023 9:21pm

@github-actions
Copy link
Contributor

✅ This change can build next-swc

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2023

🟢 CI successful 🟢

Thanks

@github-actions
Copy link
Contributor

Linux Benchmark for 900d4c9

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9631.00µs ± 91.15µs 9703.63µs ± 60.22µs +0.75%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8890.03µs ± 20.39µs 9009.87µs ± 177.38µs +1.35%
bench_startup/Turbopack CSR/1000 modules 1091.55ms ± 1.15ms 1084.22ms ± 4.95ms -0.67%

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link
Contributor

MacOS Benchmark for 900d4c9

Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 26.99ms ± 0.16ms 27.62ms ± 0.07ms +2.33% +0.66%
bench_startup/Turbopack CSR/1000 modules 4255.80ms ± 475.55ms 9670.84ms ± 1563.38ms +127.24% +25.68%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 26.99ms ± 0.16ms 27.62ms ± 0.07ms +2.33% +0.66%
bench_hmr_to_eval/Turbopack CSR/1000 modules 27.06ms ± 0.24ms 26.64ms ± 0.24ms -1.55%
bench_startup/Turbopack CSR/1000 modules 4255.80ms ± 475.55ms 9670.84ms ± 1563.38ms +127.24% +25.68%

my_transitive_emitting_function("", "a");
my_transitive_emitting_function("", "b");
async fn my_multi_emitting_function() -> Result<ThingVc> {
my_transitive_emitting_function("", "a").await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use let _ = for these two

@@ -435,7 +435,7 @@ impl AssetContext for ModuleAssetContext {
request,
);

result.add_reference(types_reference.into());
result.add_reference(types_reference.into()).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably returns a new result. Maybe rename the function to with_reference

Suggested change
result.add_reference(types_reference.into()).await?;
result = result.add_reference(types_reference.into());

@github-actions
Copy link
Contributor

Linux Benchmark for 70a6270

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 6143.47µs ± 37.43µs 6194.36µs ± 17.02µs +0.83%
bench_hmr_to_eval/Turbopack CSR/1000 modules 5781.66µs ± 38.19µs 6321.06µs ± 617.03µs +9.33%
bench_startup/Turbopack CSR/1000 modules 914.44ms ± 1.32ms 923.20ms ± 6.10ms +0.96%

sig.span()
.unwrap()
.error(
"Cannot return `()` from a turbo_tasks function. Return Result<NothingVc> instead.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Cannot return `()` from a turbo_tasks function. Return Result<NothingVc> instead.",
"Cannot return `()` from a turbo_tasks function. Return a NothingVc instead.",

@github-actions
Copy link
Contributor

Linux Benchmark for 84f15d3

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 7944.58µs ± 28.20µs 7890.60µs ± 26.73µs -0.68%
bench_hmr_to_eval/Turbopack CSR/1000 modules 7336.06µs ± 16.27µs 7704.88µs ± 334.52µs +5.03%
bench_startup/Turbopack CSR/1000 modules 971.05ms ± 1.24ms 977.10ms ± 6.74ms +0.62%

@wbinnssmith wbinnssmith merged commit c51c7ab into main Jun 30, 2023
@wbinnssmith wbinnssmith deleted the wbinnssmith/vc-must-use branch June 30, 2023 22:00
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Jul 3, 2023
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
This uses the `#[must-use]` macro [0] to mark Vcs as needing to be used
when created. The compiler creates warnings for each case.

Test Plan: Create a Vc in code and verify a warning is shown when
Turbopack is built.

[0]
https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
This uses the `#[must-use]` macro [0] to mark Vcs as needing to be used
when created. The compiler creates warnings for each case.

Test Plan: Create a Vc in code and verify a warning is shown when
Turbopack is built.

[0]
https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
This uses the `#[must-use]` macro [0] to mark Vcs as needing to be used
when created. The compiler creates warnings for each case.

Test Plan: Create a Vc in code and verify a warning is shown when
Turbopack is built.

[0]
https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants