detect missing trait methods at compile time #3657

ForsakenHarmony · 2023-02-06T20:16:16Z

Leaves the default implementations of a trait "in place" and implements the "cross-trait" resolving differently

we get back the normal rust error messages when you don't have every method implemented 🎉

Fixes WEB-569

ForsakenHarmony · 2023-02-06T20:16:29Z

Current dependencies on/for this PR:

main
- PR detect missing trait methods at compile time #3657 👈

This comment was auto-generated by Graphite.

ForsakenHarmony · 2023-02-06T20:18:43Z

crates/turbo-tasks-macros/src/value_trait_macro.rs

+            #ref_ident: turbo_tasks::FromSubTrait<T>,
+            turbo_tasks::TaskInput: for<'a> std::convert::From<&'a T>
+            #(, #supertrait_refs: turbo_tasks::FromSubTrait<T>)* {
+            #(#trait_fns)*
        }


This is the main change with the new turbo_tasks::FromSubTrait (just a copy paste of std::convert::From)

no more default fn impls

github-actions · 2023-02-06T20:18:57Z

⚠️ CI failed ⚠️

The following steps have failed in CI:

Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

ForsakenHarmony · 2023-02-06T20:19:09Z

crates/turbo-tasks-macros/src/value_trait_macro.rs

@@ -233,8 +243,6 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream {
                let raw_vc: turbo_tasks::RawVc = super_trait_vc.into();
                #ref_ident { node: raw_vc }
            }
-
-            #(pub #trait_fns)*


Could probably add this back to reduce the imports, though this is more "rusty" now

ForsakenHarmony · 2023-02-06T20:19:29Z

crates/turbo-tasks-macros/src/value_trait_macro.rs

@@ -138,6 +138,10 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream {
                });

                *sig = external_sig;
+                *default = Some(parse_quote! {{


We also keep the default methods around, unsure if this needs to be trait call or not

ForsakenHarmony · 2023-02-06T20:20:21Z

crates/turbo-tasks/src/value_type.rs

@@ -51,6 +51,41 @@ pub trait ValueTraitVc:
    fn get_trait_type_id() -> TraitTypeId;
 }

+pub trait IntoSuperTrait<T>: ValueTraitVc


here are the definitions for the new traits

Why ain't the normal From and Into traits working for that?

FWIW I like having the separation here. The semantics are clearer.

I think the reason the default fn was needed because non trait types already implement Into<TraitVc> which was the reason it never complained about missing functions

ForsakenHarmony · 2023-02-06T20:21:00Z

crates/turbopack-ecmascript/src/chunk/loader.rs

+    #[turbo_tasks::function]
+    fn related_path(&self) -> FileSystemPathVc {
+        self.manifest.path()
+    }


this was missing

ForsakenHarmony · 2023-02-06T20:21:05Z

crates/turbopack-ecmascript/src/chunk/loader.rs

+    #[turbo_tasks::function]
+    fn related_path(&self) -> FileSystemPathVc {
+        self.manifest.path()
+    }


ForsakenHarmony · 2023-02-06T20:21:14Z

crates/turbopack-json/src/lib.rs

+    #[turbo_tasks::function]
+    fn get_exports(&self) -> EcmascriptExportsVc {
+        EcmascriptExports::Value.cell()
+    }


ForsakenHarmony · 2023-02-06T20:21:19Z

crates/turbopack-mdx/src/lib.rs

+    #[turbo_tasks::function]
+    fn get_exports(&self) -> EcmascriptExportsVc {
+        EcmascriptExports::Value.cell()
+    }


github-actions · 2023-02-06T21:49:48Z

Benchmark for `fcded0d`

Click to view benchmark

Test	Base	PR	%
bench_hmr_to_commit/Turbopack CSR/1000 modules	10.05ms ± 0.10ms	9950.59µs ± 86.52µs	-0.96%
bench_hmr_to_commit/Turbopack RCC/1000 modules	10.31ms ± 0.09ms	10.21ms ± 0.08ms	-0.95%
bench_hmr_to_commit/Turbopack RSC/1000 modules	506.05ms ± 1.49ms	508.73ms ± 2.75ms	+0.53%
bench_hmr_to_commit/Turbopack SSR/1000 modules	10.19ms ± 0.09ms	10.10ms ± 0.10ms	-0.90%
bench_hmr_to_eval/Turbopack CSR/1000 modules	8954.98µs ± 134.70µs	9123.45µs ± 87.90µs	+1.88%
bench_hmr_to_eval/Turbopack RCC/1000 modules	9210.66µs ± 86.94µs	9237.92µs ± 103.60µs	+0.30%
bench_hmr_to_eval/Turbopack SSR/1000 modules	9083.23µs ± 74.42µs	9054.24µs ± 83.08µs	-0.32%
bench_hydration/Turbopack RCC/1000 modules	4317.31ms ± 9.60ms	4336.33ms ± 18.16ms	+0.44%
bench_hydration/Turbopack RSC/1000 modules	3890.78ms ± 15.94ms	3915.26ms ± 11.81ms	+0.63%
bench_hydration/Turbopack SSR/1000 modules	3818.57ms ± 15.65ms	3786.79ms ± 9.40ms	-0.83%
bench_startup/Turbopack CSR/1000 modules	2826.30ms ± 11.47ms	2798.25ms ± 11.11ms	-0.99%
bench_startup/Turbopack RCC/1000 modules	2624.54ms ± 8.11ms	2610.61ms ± 7.92ms	-0.53%
bench_startup/Turbopack RSC/1000 modules	2495.92ms ± 8.70ms	2516.36ms ± 8.05ms	+0.82%
bench_startup/Turbopack SSR/1000 modules	2148.89ms ± 2.89ms	2143.87ms ± 2.55ms	-0.23%

alexkirsz

This is great.

alexkirsz · 2023-02-07T09:12:18Z

crates/turbo-tasks-macros/src/value_trait_macro.rs

@@ -265,6 +278,12 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream {
                }
            }

+            impl turbo_tasks::FromSubTrait<#ref_ident> for #supertrait_refs {


In my Vc<T> work, I called this trait Upcast (for both the Vc<dyn Sub> -> Vc<dyn Super> and the Vc<Val> -> Vc<dyn Trait> cases).

alexkirsz · 2023-02-07T09:14:43Z

crates/turbo-tasks/src/value_type.rs

@@ -51,6 +51,41 @@ pub trait ValueTraitVc:
    fn get_trait_type_id() -> TraitTypeId;
 }

+pub trait IntoSuperTrait<T>: ValueTraitVc


FWIW I like having the separation here. The semantics are clearer.

@ForsakenHarmony

That's extracted from @ForsakenHarmony's PR #3657 and fixes the workspace members

…kspace

github-actions · 2023-02-07T16:44:10Z

Benchmark for `153f62c`

Test	Base	PR	%	Significant %
bench_hydration/Turbopack SSR/1000 modules	3893.93ms ± 9.60ms	3816.66ms ± 24.47ms	-1.98%	-0.24%
bench_startup/Turbopack RCC/1000 modules	2662.29ms ± 10.78ms	2712.89ms ± 10.52ms	+1.90%	+0.30%

Click to view full benchmark

Test	Base	PR	%	Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules	10.10ms ± 0.10ms	10.27ms ± 0.09ms	+1.70%
bench_hmr_to_commit/Turbopack RCC/1000 modules	10.35ms ± 0.07ms	10.33ms ± 0.08ms	-0.21%
bench_hmr_to_commit/Turbopack RSC/1000 modules	518.44ms ± 2.18ms	520.71ms ± 3.42ms	+0.44%
bench_hmr_to_commit/Turbopack SSR/1000 modules	10.41ms ± 0.09ms	10.23ms ± 0.09ms	-1.75%
bench_hmr_to_eval/Turbopack CSR/1000 modules	9360.17µs ± 121.69µs	9236.25µs ± 42.73µs	-1.32%
bench_hmr_to_eval/Turbopack RCC/1000 modules	9283.16µs ± 93.71µs	9346.54µs ± 106.45µs	+0.68%
bench_hmr_to_eval/Turbopack SSR/1000 modules	9397.31µs ± 101.35µs	9238.58µs ± 68.79µs	-1.69%
bench_hydration/Turbopack RCC/1000 modules	4394.88ms ± 20.69ms	4426.63ms ± 15.27ms	+0.72%
bench_hydration/Turbopack RSC/1000 modules	4022.77ms ± 12.56ms	4018.95ms ± 24.65ms	-0.09%
bench_hydration/Turbopack SSR/1000 modules	3893.93ms ± 9.60ms	3816.66ms ± 24.47ms	-1.98%	-0.24%
bench_startup/Turbopack CSR/1000 modules	2869.35ms ± 10.10ms	2879.03ms ± 17.09ms	+0.34%
bench_startup/Turbopack RCC/1000 modules	2662.29ms ± 10.78ms	2712.89ms ± 10.52ms	+1.90%	+0.30%
bench_startup/Turbopack RSC/1000 modules	2588.71ms ± 9.13ms	2591.74ms ± 10.03ms	+0.12%
bench_startup/Turbopack SSR/1000 modules	2218.85ms ± 3.81ms	2200.52ms ± 8.37ms	-0.83%

Leaves the default implementations of a trait "in place" and implements the "cross-trait" resolving differently we get back the normal rust error messages when you don't have every method implemented 🎉

ForsakenHarmony requested a review from a team as a code owner February 6, 2023 20:16

github-actions bot added the area: turbopack label Feb 6, 2023

vercel bot deployed to Preview – turbo-site February 6, 2023 20:17 View deployment

ForsakenHarmony commented Feb 6, 2023

View reviewed changes

ForsakenHarmony changed the title ~~detect missing trait methods~~ Detect missing trait methods at compile time Feb 6, 2023

ForsakenHarmony changed the title ~~Detect missing trait methods at compile time~~ detect missing trait methods at compile time Feb 6, 2023

ForsakenHarmony force-pushed the hrmny/detect-missing-trait-methods branch from 6eb4320 to bbf4c76 Compare February 6, 2023 20:31

ForsakenHarmony requested a review from a team as a code owner February 6, 2023 20:31

ForsakenHarmony requested review from nathanhammond and NicholasLYang February 6, 2023 20:31

sokra mentioned this pull request Feb 7, 2023

add missing workspace members and default members #3668

Merged

alexkirsz approved these changes Feb 7, 2023

View reviewed changes

kodiakhq bot pushed a commit that referenced this pull request Feb 7, 2023

add missing workspace members and default members (#3668)

e794df4

That's extracted from @ForsakenHarmony's PR #3657 and fixes the workspace members

ForsakenHarmony added 5 commits February 7, 2023 16:22

detect missing trait methods

4ebfbab

add missing imports

5523ce9

implement missing methods

223fb4b

missed an import because the crate wasn't a default member of the wor…

d99e732

…kspace

remove outdated build script glob

2c25b89

ForsakenHarmony force-pushed the hrmny/detect-missing-trait-methods branch from bbf4c76 to 2c25b89 Compare February 7, 2023 15:26

vercel bot deployed to Preview – turbo-docs February 7, 2023 15:27 View deployment

vercel bot deployed to Preview – turbo-site February 7, 2023 15:27 View deployment

vercel bot deployed to Preview – turbo-docs-at18 February 7, 2023 15:27 View deployment

vercel bot had a problem deploying to Preview – turbo-js February 7, 2023 15:28 Failure

vercel bot had a problem deploying to Preview – turbo-turbo-tracing-next-plugin-ek5c February 7, 2023 15:29 Failure

vercel bot had a problem deploying to Preview – turbo-tests February 7, 2023 15:30 Failure

vercel bot had a problem deploying to Preview – turbo-tests-7fjh February 7, 2023 15:31 Failure

vercel bot had a problem deploying to Preview – turbo-benchmark February 7, 2023 15:32 Failure

vercel bot had a problem deploying to Preview – turbo-cli February 7, 2023 15:33 Failure

vercel bot had a problem deploying to Preview – turbo-turbo-tracing-next-plugin February 7, 2023 15:33 Failure

vercel bot had a problem deploying to Preview – turbo-with-mongodb-mongoose-8o41 February 7, 2023 15:34 Failure

vercel bot had a problem deploying to Preview – turbo-buildcontainer February 7, 2023 15:35 Failure

vercel bot had a problem deploying to Preview – turbo-with-mongodb-mongoose February 7, 2023 15:36 Failure

vercel bot had a problem deploying to Preview – turbo-js-q5o2 February 7, 2023 15:37 Failure

vercel bot had a problem deploying to Preview – turbo-crates February 7, 2023 15:38 Failure

vercel deleted a comment from vercel bot Feb 7, 2023

ForsakenHarmony merged commit bf0980e into main Feb 7, 2023

ForsakenHarmony deleted the hrmny/detect-missing-trait-methods branch February 7, 2023 16:06

DuCanhGH mentioned this pull request Mar 15, 2023

Fix(typedRoutes): fixed files from directories other than pages and app getting into Route vercel/next.js#47184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect missing trait methods at compile time #3657

detect missing trait methods at compile time #3657

ForsakenHarmony commented Feb 6, 2023 •

edited

ForsakenHarmony commented Feb 6, 2023

ForsakenHarmony Feb 6, 2023 •

edited

github-actions bot commented Feb 6, 2023 •

edited

ForsakenHarmony Feb 6, 2023

ForsakenHarmony Feb 6, 2023 •

edited

ForsakenHarmony Feb 6, 2023

sokra Feb 7, 2023

alexkirsz Feb 7, 2023

ForsakenHarmony Feb 7, 2023

ForsakenHarmony Feb 6, 2023

ForsakenHarmony Feb 6, 2023

ForsakenHarmony Feb 6, 2023

ForsakenHarmony Feb 6, 2023

github-actions bot commented Feb 6, 2023

alexkirsz left a comment

alexkirsz Feb 7, 2023

alexkirsz Feb 7, 2023

github-actions bot commented Feb 7, 2023

detect missing trait methods at compile time #3657

detect missing trait methods at compile time #3657

Conversation

ForsakenHarmony commented Feb 6, 2023 • edited

ForsakenHarmony commented Feb 6, 2023

ForsakenHarmony Feb 6, 2023 • edited

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2023 • edited

⚠️ CI failed ⚠️

Choose a reason for hiding this comment

ForsakenHarmony Feb 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2023

Benchmark for fcded0d

alexkirsz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2023

Benchmark for 153f62c

ForsakenHarmony commented Feb 6, 2023 •

edited

ForsakenHarmony Feb 6, 2023 •

edited

github-actions bot commented Feb 6, 2023 •

edited

ForsakenHarmony Feb 6, 2023 •

edited

Benchmark for `fcded0d`

Benchmark for `153f62c`