feat(op2): remove async attribute and make FromV8/ToV8 the default behaviour
#1262
Conversation
WalkthroughThe PR removes explicit Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
# Conflicts: # core/ops_builtin_v8.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ops/op2/dispatch_slow.rs (4)
338-369: Fake-async should likely be treated as async for cppgc rooting.
Ifgenerator_state.is_fake_asyncproduces a future boundary, then the non-rooting branch can leaveself_(and similar cppgc refs) unrooted across that boundary.pub(crate) fn with_self( generator_state: &mut GeneratorState, ret_val: &RetVal, ) -> TokenStream { @@ - if ret_val.is_async() { + let is_async = ret_val.is_async() || generator_state.is_fake_async; + if is_async { let tokens = gs_quote!(generator_state(self_ty, fn_args, scope, try_unwrap_cppgc) => { let Some(mut self_) = deno_core::_ops::#try_unwrap_cppgc::<#self_ty>(&mut #scope, #fn_args.this().into()) else { #throw_exception; }; self_.root(); });
786-857: Same fake-async rooting concern for cppgc args.
CppGcResource/OptionCppGcResourceshould likely root whenret_val.is_async() || generator_state.is_fake_async.- if ret_val.is_async() { + if ret_val.is_async() || generator_state.is_fake_async { let tokens = quote! { let Some(mut #arg_ident) = deno_core::_ops::#try_unwrap_cppgc::<#ty>(&mut #scope, #from_ident) else { #throw_exception; }; #arg_ident.root(); }; @@ - if ret_val.is_async() { + if ret_val.is_async() || generator_state.is_fake_async { let tokens = quote! { let #arg_ident = if #arg_ident.is_null_or_undefined() { None } else if let Some(mut #arg_ident) = deno_core::_ops::#try_unwrap_cppgc::<#ty>(&mut #scope, #arg_ident) { #arg_ident.root(); Some(#arg_ident) } else { #throw_exception; }; };
1015-1051:call()should handlemovesfor fake-async too (if you root in fake-async).
Once you root in fake-async branches, you’ll want thosemovesto execute inside the future as well.- if ret_val.is_async() && !generator_state.moves.is_empty() { + if (ret_val.is_async() || generator_state.is_fake_async) && !generator_state.moves.is_empty() { let mut moves = TokenStream::new(); for m in &generator_state.moves { moves.extend(m.clone()); } - quote!(async move { + let maybe_await = if ret_val.is_async() { quote!(.await) } else { quote!() }; + quote!(async move { #moves - #call.await + #call #maybe_await }) } else if generator_state.is_fake_async { quote!(std::future::ready(#call)) } else { call }
659-693: Remove unusedFromV8Fastimport and use UFCS instead.In the
Arg::FromV8(ty, true)branch,use deno_core::FromV8Fast;is never used and will trigger warnings under#![deny(warnings)]. Use UFCS syntax to avoid the unused import:Arg::FromV8(ty, true) => { *needs_scope = true; let ty = syn::parse_str::<syn::Type>(ty).expect("Failed to reparse state type"); let scope = scope.clone(); let err = format_ident!("{}_err", arg_ident); let throw_exception = throw_type_error_string(generator_state, &err); quote! { - let #arg_ident = { - use deno_core::FromV8; - use deno_core::FromV8Fast; - - match <#ty>::from_v8(&mut #scope, #arg_ident) { + let #arg_ident = match <#ty as deno_core::FromV8>::from_v8(&mut #scope, #arg_ident) { Ok(t) => t, Err(#err) => { #throw_exception; } - } - }; + }; }; }The semantics are correct:
truerepresents the slow path (requiring scope andFromV8trait), whilefalserepresents the fast path (FromV8Fastwithout scope).core/convert.rs (1)
675-714: Fix null pointer dereference inabview_to_vecbefore callingadd().The
data()pointer can be null for zero-length arrays (V8 fastcalls behavior), butadd()is called unconditionally on line 703. This is undefined behavior. UseNonNull::new()to safely handle the null case before any pointer arithmetic, similar to the pattern already used incore/runtime/ops.rsline 460. Apply the fix to both the explicitUint8Arrayimpl and all macro-generated typed array implementations.
🧹 Nitpick comments (3)
ops/op2/dispatch_async.rs (1)
30-51: Consider improving the error message.The error string
"an async return"at line 50 is incomplete. Looking at the error handling at lines 82-89, this string becomes part ofV8SignatureMappingError::NoRetValMapping. The current message doesn't clearly indicate what was expected.Consider making it more descriptive:
} else { - return Err("an async return"); + return Err("a Future return type for async dispatch"); };ops/op2/signature.rs (2)
650-658: Consider boxing the Type in CUnknown variant.The clippy warning suppression indicates
Typecan be large. Boxing it (Box<Type>) would reduce the overall enum size, though the current approach works.
1598-1598: Extract repeated V8Slow check into helper method.The pattern
matches!(attrs.primary, Some(AttributeModifier::V8Slow))appears multiple times (lines 1387, 1409, 1429, 1439, 1759, 1763). Consider adding a helper method likeattrs.is_v8_slow()for clarity.+impl Attributes { + fn is_v8_slow(&self) -> bool { + matches!(self.primary, Some(AttributeModifier::V8Slow)) + } +}Also applies to: 1626-1626, 1689-1690, 1756-1765
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
ops/op2/test_cases/async/async_v8_global.outis excluded by!**/*.outops/op2/test_cases/sync/op_state_ref.outis excluded by!**/*.outops/op2/test_cases/sync/v8_global.outis excluded by!**/*.out
📒 Files selected for processing (50)
core/benches/ops/async.rs(1 hunks)core/benches/ops/sync.rs(0 hunks)core/convert.rs(22 hunks)core/examples/op2.rs(1 hunks)core/lib.rs(1 hunks)core/ops_builtin.rs(9 hunks)core/ops_builtin_v8.rs(9 hunks)core/runtime/ops.rs(14 hunks)core/runtime/tests/jsrealm.rs(1 hunks)core/runtime/tests/misc.rs(6 hunks)core/runtime/tests/mod.rs(1 hunks)core/runtime/tests/ops.rs(6 hunks)ops/op2/README.md(0 hunks)ops/op2/config.rs(8 hunks)ops/op2/dispatch_async.rs(3 hunks)ops/op2/dispatch_fast.rs(3 hunks)ops/op2/dispatch_shared.rs(0 hunks)ops/op2/dispatch_slow.rs(9 hunks)ops/op2/generator_state.rs(1 hunks)ops/op2/mod.rs(9 hunks)ops/op2/object_wrap.rs(1 hunks)ops/op2/signature.rs(34 hunks)ops/op2/signature_retval.rs(2 hunks)ops/op2/test_cases/async/async_arg_return.rs(1 hunks)ops/op2/test_cases/async/async_arg_return_result.rs(1 hunks)ops/op2/test_cases/async/async_cppgc.rs(1 hunks)ops/op2/test_cases/async/async_jsbuffer.rs(1 hunks)ops/op2/test_cases/async/async_op_metadata.rs(1 hunks)ops/op2/test_cases/async/async_opstate.rs(1 hunks)ops/op2/test_cases/async/async_precise_capture.rs(1 hunks)ops/op2/test_cases/async/async_result.rs(1 hunks)ops/op2/test_cases/async/async_result_impl.rs(1 hunks)ops/op2/test_cases/async/async_result_smi.rs(1 hunks)ops/op2/test_cases/async/async_stack_trace.rs(1 hunks)ops/op2/test_cases/async/async_v8_global.rs(0 hunks)ops/op2/test_cases/async/async_void.rs(1 hunks)ops/op2/test_cases/compiler_pass/async.rs(1 hunks)ops/op2/test_cases/compiler_pass/sync.rs(0 hunks)ops/op2/test_cases/sync/from_v8.rs(1 hunks)ops/op2/test_cases/sync/op_state_ref.rs(0 hunks)ops/op2/test_cases/sync/to_v8.rs(0 hunks)ops/op2/test_cases/sync/v8_global.rs(0 hunks)ops/op2/valid_args.md(0 hunks)ops/op2/valid_retvals.md(1 hunks)testing/checkin/runner/ops.rs(0 hunks)testing/checkin/runner/ops_async.rs(6 hunks)testing/checkin/runner/ops_error.rs(1 hunks)testing/checkin/runner/ops_io.rs(1 hunks)testing/checkin/runner/ops_worker.rs(2 hunks)testing/checkin/runner/testing.rs(1 hunks)
💤 Files with no reviewable changes (10)
- ops/op2/test_cases/async/async_v8_global.rs
- ops/op2/dispatch_shared.rs
- ops/op2/valid_args.md
- ops/op2/test_cases/sync/op_state_ref.rs
- ops/op2/test_cases/sync/v8_global.rs
- ops/op2/test_cases/compiler_pass/sync.rs
- testing/checkin/runner/ops.rs
- ops/op2/test_cases/sync/to_v8.rs
- core/benches/ops/sync.rs
- ops/op2/README.md
🧰 Additional context used
🧬 Code graph analysis (27)
ops/op2/test_cases/async/async_void.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_precise_capture.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_opstate.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_result_smi.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_jsbuffer.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
core/benches/ops/async.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/lib.rs (1)
op2(16-18)core/runtime/ops.rs (1)
op_async_void(2098-2098)
testing/checkin/runner/ops_error.rs (2)
ops/op2/mod.rs (1)
op2(76-96)ops/lib.rs (1)
op2(16-18)
core/runtime/tests/misc.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/compile_test_runner/lib.rs (1)
op2(20-25)ops/lib.rs (1)
op2(16-18)
ops/op2/test_cases/async/async_stack_trace.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_arg_return_result.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_op_metadata.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/signature_retval.rs (1)
ops/op2/signature.rs (1)
parse_type(1590-1865)
ops/op2/test_cases/compiler_pass/async.rs (2)
ops/op2/mod.rs (1)
op2(76-96)ops/op2/test_cases/async/async_opstate.rs (2)
op_async_opstate(11-15)state(14-14)
ops/op2/test_cases/async/async_result_impl.rs (2)
ops/op2/mod.rs (1)
op2(76-96)ops/lib.rs (1)
op2(16-18)
testing/checkin/runner/ops_io.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/compile_test_runner/lib.rs (1)
op2(20-25)ops/lib.rs (1)
op2(16-18)
testing/checkin/runner/ops_worker.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/compile_test_runner/lib.rs (1)
op2(20-25)ops/lib.rs (1)
op2(16-18)
core/runtime/tests/mod.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/compile_test_runner/lib.rs (1)
op2(20-25)ops/lib.rs (1)
op2(16-18)
core/runtime/tests/jsrealm.rs (3)
ops/op2/mod.rs (1)
op2(76-96)ops/compile_test_runner/lib.rs (1)
op2(20-25)ops/lib.rs (1)
op2(16-18)
ops/op2/test_cases/async/async_cppgc.rs (3)
ops/op2/mod.rs (1)
op2(76-96)core/webidl.rs (1)
cppgc(871-871)ops/op2/test_cases_fail/lifetimes.rs (1)
op_use_cppgc_object(18-18)
ops/op2/test_cases/async/async_arg_return.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/test_cases/async/async_result.rs (1)
ops/op2/mod.rs (1)
op2(76-96)
ops/op2/mod.rs (1)
ops/op2/signature.rs (3)
parse_signature(1008-1069)name(805-823)sig(1936-1936)
core/runtime/ops.rs (2)
core/core.d.ts (1)
Uint8Array(1275-1275)core/convert.rs (2)
value(355-356)value(690-690)
ops/op2/config.rs (1)
ops/webidl/mod.rs (1)
list(80-80)
ops/op2/signature.rs (2)
ops/op2/signature_retval.rs (2)
arg(122-128)try_parse(132-161)ops/op2/mod.rs (4)
signature(279-283)signature(284-288)signature(290-290)signature(291-291)
core/ops_builtin.rs (2)
ops/op2/mod.rs (1)
op2(76-96)ops/lib.rs (1)
op2(16-18)
core/ops_builtin_v8.rs (2)
core/core.d.ts (2)
Uint8Array(1275-1275)Function(924-924)core/error.rs (1)
f(1807-1807)
🔇 Additional comments (82)
ops/op2/test_cases/async/async_op_metadata.rs (1)
6-20: LGTM! Async inference from function signature.The removal of explicit
asyncfrom the macro attributes is correct—theop2macro now infers async behavior from theasync fnsignatures. The metadata attributes are preserved as expected.ops/op2/test_cases/sync/from_v8.rs (1)
20-22: LGTM!The removal of the
#[from_v8]attribute aligns with the PR objective whereFromV8behavior is now the default for arguments. TheFootype correctly implementsFromV8Trait, so the conversion will work implicitly.ops/op2/valid_retvals.md (1)
33-35: LGTM!The documentation correctly indicates that
#[buffer] V8Slice<u32>is supported whileVec<u32>andBox<[u32]>variants are not (missing the "X" in the Supported column). This provides clear guidance on which buffer types can be used as return values.testing/checkin/runner/ops_error.rs (1)
6-9: LGTM!The removal of
asyncfrom the attribute is correct per the PR objectives. The async behavior will now be inferred from theasync fnsignature. Thedeferredandlazyvariants on other ops correctly remain explicit as they specify non-default async behavior.ops/op2/test_cases/async/async_opstate.rs (1)
10-14: LGTM!Correctly updated test case - async behavior is now inferred from the
async fnsignature rather than the macro attribute.testing/checkin/runner/testing.rs (1)
44-54: LGTM!The
#[global]→#[v8_slow]change correctly aligns with the PR objectives. Thev8::Global<v8::Function>type requires the slow-pathFromV8conversion, which is now explicitly requested via#[v8_slow].testing/checkin/runner/ops_io.rs (1)
103-126: LGTM!The removal of
asyncfrom the attribute follows the PR pattern. The async behavior is correctly inferred from theasync fnsignature.testing/checkin/runner/ops_worker.rs (2)
221-236: Same as above; macro-only change, function remains async.
194-198: Async inference confirmed; pattern matches the rest of the codebase.The op2 macro correctly infers async from the
async fnsignature. This is already the established pattern across the codebase—multiple async operations inops/op2/test_cases/use#[op2]without an explicitasyncparameter on async functions. No lingering#[op2(async)]remains in the file. Changes are good to go.ops/op2/dispatch_slow.rs (4)
29-29: RetVal import is fine.
1053-1066: RetVal matching change looks consistent with the refactor.
Just ensure non-Value/Result variants are handled elsewhere (thetodo!()remains a footgun if reachable).
1123-1125: MappingToV8FastlikeToV8in slow path is sensible.
1206-1208: Same:ToV8Fasttreated asToV8for v8::Value production is fine.core/lib.rs (1)
71-75: Public re-exports look good; validate intended public API + docs.ops/op2/test_cases/async/async_void.rs (1)
6-7: Good migration test for async inference; should keep catching regressions.core/runtime/tests/mod.rs (1)
34-64: Looks correct; relies on op2 async inference—worth double-checking the test still exercises async dispatch paths.ops/op2/test_cases/async/async_precise_capture.rs (1)
8-13: LGTM - async inference working as intended.The removal of the explicit
asyncflag aligns with the PR objective. The macro now infers async behavior from theimpl Futurereturn type.ops/op2/generator_state.rs (1)
13-13: LGTM - new field for fake-async tracking.The
is_fake_asyncfield addition supports the async behavior refactoring mentioned in the PR objectives.core/runtime/tests/jsrealm.rs (1)
85-88: LGTM - async inference from function signature.The macro correctly infers async behavior from the
async fndeclaration.core/runtime/tests/misc.rs (6)
90-96: LGTM - async inference working correctly.All test ops in this file have been updated consistently to remove explicit async flags while maintaining async function signatures.
714-719: LGTM - consistent with other async op updates.
1036-1040: LGTM - async behavior correctly inferred.
1084-1087: LGTM - consistent async op conversion.
1190-1193: LGTM - async op attribute updated correctly.
1328-1332: LGTM - final async op conversion in this file.ops/op2/test_cases/async/async_result.rs (1)
6-9: LGTM - async inference from async fn.ops/op2/test_cases/async/async_jsbuffer.rs (1)
8-12: LGTM - async inference with buffer attribute.The
#[buffer]attribute is correctly retained while removing the explicit async flag.ops/op2/test_cases/async/async_result_impl.rs (1)
9-14: LGTM - async inference from Future return type.The macro correctly identifies async behavior from the
impl Futurereturn type, even when wrapped in a Result.ops/op2/test_cases/async/async_arg_return.rs (1)
6-9: LGTM - async inference from async fn.ops/op2/test_cases/async/async_result_smi.rs (1)
8-11: LGTM!The removal of the explicit
asyncflag from#[op2]is correct—async behavior is now inferred from theasync fnsignature, aligning with the PR objectives.ops/op2/test_cases/async/async_arg_return_result.rs (1)
6-9: LGTM!Attribute change is consistent with the PR's approach of inferring async behavior from the function signature.
core/benches/ops/async.rs (1)
54-60: LGTM!Both
op_async_voidandop_async_yieldcorrectly updated to use#[op2]without the explicitasyncflag. Theasync(lazy)andasync(deferred)variants are appropriately preserved on other ops, consistent with the PR description stating those alternatives remain unchanged.ops/op2/test_cases/async/async_cppgc.rs (1)
18-28: LGTM!All three CPPGC-related ops correctly updated to use
#[op2]without the explicitasyncflag while preserving the#[cppgc]attributes for garbage-collected object handling.ops/op2/test_cases/async/async_stack_trace.rs (1)
6-7: LGTM!The
asyncflag is correctly removed while preserving thestack_traceattribute. This demonstrates proper attribute composition under the new inference model.core/examples/op2.rs (1)
7-14: LGTM!The migration from
#[global]to#[v8_slow]is correct per the PR objectives. Sincev8::Global<v8::Function>requires the slow conversion path (not the new fast-path default), the#[v8_slow]attribute appropriately signals this requirement.ops/op2/test_cases/compiler_pass/async.rs (1)
15-62: LGTM!All nine async ops correctly updated. Notable coverage includes:
- Standard
async fnvariantsResult<impl Future<Output = T>>patterns (op_async4, op_async5) which should be correctly inferred as async- Preservation of auxiliary attributes (
#[buffer],#[string])ops/op2/dispatch_async.rs (2)
21-21: LGTM!Import updated to reflect
RetValrelocation tosignature_retvalmodule.
98-111: LGTM!The added
!generator_state.is_fake_asynccondition correctly prevents eager error unwrapping for fake-async ops. In fake-async scenarios, theResultis part of the future's output type rather than an outer wrapper, so attempting eager unwrap would be incorrect.ops/op2/dispatch_fast.rs (3)
401-407: Verify the operator precedence in this condition.The condition
signature.ret_val.is_async() && !config.async_lazy && !config.async_deferred || config.fake_asynchas ambiguous precedence. Due to&&binding tighter than||, this evaluates as:(is_async && !async_lazy && !async_deferred) || fake_asyncIf
fake_asyncistrue, the function returnsOk(None), meaning no fast path is generated. However, the AI summary states "fake_async enables fast path." Please verify this matches the intended behavior.If you want fake_async ops to skip the fast path, this is correct. If you want them to have a fast path, parentheses are needed.
912-914: Pattern updated to match newFromV8arity.The
Arg::FromV8(_, _)now accepts two parameters, consistent with the broader refactor adding type information. This correctly returnsOk(None)to indicate no fast-call support forFromV8arguments.
964-965: Pattern updated to match newToV8arity.The
Arg::ToV8(_, _)pattern correctly excludes these return types from fast-call support, returningOk(None).ops/op2/signature_retval.rs (3)
85-129: Clean design for nested return value representation.The
RetValenum withValue,Result, andFuturevariants cleanly models the nested return type structures. The recursive helper methods (is_async,get_future,unwrap_result,arg) correctly traverse the structure.
131-161: Recursive parsing with async wrapping looks correct.The
handle_typeinner function recursively unwrapsResultandFuture(viaimpl Future<Output = ...>) types, andtry_parsewraps the final result inFuturewhen the function is declaredasync. This correctly handles both explicitimpl Futurereturns andasync fnsyntax.
174-199: Test coverage for return value parsing.Tests cover the key combinations: plain values,
Result<_>,impl Future<Output = _>, and nested combinations likeResult<impl Future<...>>andFuture<Result<...>>. Good coverage for the new structure.core/runtime/tests/ops.rs (3)
23-24: Async inference from function signature.The
asynckeyword removal from#[op2]is correct—async behavior is now inferred from theasync fnsignature. This aligns with the PR's goal of simplifying op declarations.
104-105: Consistent migration to inferred async.All these async ops correctly have
#[op2]without the explicitasyncflag, while retaining theirasync fnsignatures. The macro will infer async behavior automatically.Also applies to: 336-338, 485-486, 540-543, 545-551, 553-557, 559-564, 566-570, 597-600
572-580: Async variants (deferred,lazy) retained correctly.The
async(deferred)andasync(lazy)attributes remain onop_async_deferredandop_async_lazysince these are explicit scheduling behaviors that cannot be inferred from the function signature alone.core/ops_builtin.rs (4)
168-171: Async ops simplified.
op_add_async,op_void_async, andop_error_asynccorrectly have theasyncflag removed from#[op2]while keeping theirasync fnsignatures.
281-282: Consistent async removal.
op_wasm_streaming_stream_feedfollows the same pattern—async inferred from signature.
341-342: Promise ID ops retain that attribute.
op_read,op_read_all, andop_writecorrectly keep#[op2(promise_id)]sincepromise_idis a distinct configuration from async. Theasyncportion is inferred from the function signature.Also applies to: 354-356, 390-391
433-434: Remaining async ops simplified.
op_write_all,op_write_type_error, andop_shutdownall correctly use#[op2]with async inferred from their signatures.Also applies to: 449-450, 464-465
ops/op2/mod.rs (7)
30-30: Import path updated for module restructure.
RetValis now imported fromsignature_retvalmodule, reflecting the extraction of return value parsing into its own module.
130-138: Simplifiedparse_signaturecall.The
parse_signaturefunction no longer requires an explicitis_asyncflag—it now readsasyncnessdirectly from theSignature. Usingident.clone()preserves span information better thanformat_ident!.
168-172: Self type handling improved.Using
ty.clone()instead of string formatting preserves the original token's span, which improves error message locations.
211-212:is_fake_asyncpropagated to generator state.This flag enables the dispatch generators to know when fake async mode is active, affecting code generation paths.
222-228: Dispatch path selection includesfake_async.When
config.fake_asyncis true, the async dispatch path is used even if the function isn't actually async. TheIdent::newwithorig_name.span()preserves error location accuracy.
246-250:fake_asyncexcluded from fast-path enforcement.Fake async ops use the async dispatch path, so they shouldn't trigger the
ShouldBeFasterror even if they're technically fast-compatible.
512-513: Test helper calls updated.Test invocations of
parse_signatureupdated to passfunction.attrsandfunction.sig.clone()matching the new API signature.Also applies to: 578-579, 594-596
testing/checkin/runner/ops_async.rs (3)
16-26:#[v8_slow]onv8::Global<v8::Function>looks correct (forces slow conversion path).
This matches the new “fast conversion by default” direction while still letting you store the callback beyond the current scope.
28-31: Async inference changes look consistent (async fn / impl Future drive async-ness now).
No functional concerns in these ops given the new macro behavior.Also applies to: 43-54, 56-65, 79-96, 103-106
43-54: No action needed. The repo uses Rust 1.91.1 with edition = "2024" configured, which exceeds the minimum requirement (Rust 1.85) forimpl Trait + use<>syntax. This code will compile without issues.core/runtime/ops.rs (6)
543-544:Uint8Arrayimport is the right follow-up for the updated buffer return types.
1948-1955:#[op2]+async fnforop_test_get_cppgc_resourcematches the new async inference model.
2097-2190: Bulk#[op2(async)]→#[op2]conversions look consistent with “async inferred from signature”.Also applies to: 2249-2266, 2288-2296, 2353-2380
2314-2319:op_async_buffer_vecreturningUint8Arrayis a good tightening of the JS-facing type.
output.into()keeps the implementation straightforward.
2394-2416: Dropping explicit#[from_v8]/#[to_v8]onSmi/Numbermatches the new defaults.
2471-2474:#[v8_slow]onBoolis appropriate unless/untilBool: FromV8Fastexists.ops/op2/config.rs (2)
454-501: Tests updated toformat_ident!correctly reflect the newIdentstorage.
248-268: Async flag parsing is stricter now (requiresasync(lazy|fake|deferred)), which matches the PR intent.
Just make sure you’re happy with the error UX for plainasync(it’ll be a parse error now).In syn/proc-macro attribute parsing, what is the best practice for producing friendly errors vs proc-macro panics when parsing keywords like `async`?core/ops_builtin_v8.rs (2)
623-718:op_serializereturningUint8Arrayis a good API improvement; dummy return on thrown exception is fine.
Minor: the dummyOk(vec![].into())could beOk(Uint8Array(Vec::new()))for clarity, but current code is fine.
28-40: Local-in, Global-stored callback pattern is correct and safe.
Acceptingv8::Local<v8::Function>at the boundary and wrapping intov8::Globalfor storage follows rusty_v8 best practices. Creating the Global from the Local while an activePinScopeis present satisfies the key requirement that persistent handles must be created within a valid HandleScope. Storing the Global in module state for later use is the intended pattern—no issues with scope or lifetime.Also applies to: 42-50, 128-177, 1050-1092, 1236-1249
core/convert.rs (1)
84-88: Fast-trait delegation pattern for Smi/Number/bool looks good.
This keeps the slow signature stable while letting op2 use fast conversions by default.Also applies to: 136-140, 191-213, 270-291, 341-360
ops/op2/signature.rs (10)
404-407: Position-aware conversion looks correct.The use of position to determine FromV8 vs ToV8 is appropriate.
564-565: Marker logic correctly implements fast/slow path distinction.
670-671: Attribute requirements correctly enforce fast-by-default behavior.
727-727: Good improvement using Ident instead of String.Preserves span information and improves type safety.
1261-1261: V8Slow attribute parsing integrated correctly.
1014-1030: Explicit name extraction improves error reporting.The refactored code is clearer about how argument names are extracted for diagnostics.
1383-1391: Vec handling logic supports both buffer and trait-based conversion paths.The conditional logic correctly distinguishes between buffer usage and generic FromV8/ToV8 conversion based on attributes.
1409-1409: v8::Global now handled through generic conversion traits.Consistent with the PR objective of removing special Global handling in favor of trait-based conversion.
1568-1578: Updated validation correctly handles new CUnknown variant.
1917-1919: Test updates consistently reflect new signature format.All tests properly updated for the removal of
is_fake_asyncparameter and new return value structure.Also applies to: 1971-1971, 1980-2141
crowlKats
changed the title
async attribute and make FromV8/ToV8 the default behaviour
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/convert.rs (1)
394-505: FixByteString::from(&str)– current conversion does not compile
impl From<&str> for ByteStringstill useslet v: Vec<u8> = s.into();, but&strdoes not implementInto<Vec<u8>>in the standard library, so this will fail to compile here (unlike in serde_v8 where additional context may exist).USIZE2X is now correctly using
std::mem::size_of, but thisFrom<&str>needs to be adjusted.Suggested fix reusing the existing
From<&[u8]>impl:impl From<&str> for ByteString { fn from(s: &str) -> Self { - let v: Vec<u8> = s.into(); - ByteString::from(v) + ByteString::from(s.as_bytes()) } }
🧹 Nitpick comments (1)
core/convert.rs (1)
559-570:Option/OptionNull/OptionUndefinedfast-paths match existing semanticsThe new
FromV8Fastimpls for:
OptionNull<T>(mappingnull→None, else delegate toT::from_v8),OptionUndefined<T>(mappingundefined→None, else delegate), and- plain
Option<T>(sameundefinedsemantics),mirror the existing
FromV8behavior and correctly reuse the underlyingFromV8FastofT.If you want to avoid future drift between slow/fast paths, you could optionally implement the
FromV8versions in terms of the new fast variants (as done for numbers), but the current duplication is correct and clear.Also applies to: 624-635, 1068-1079
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/convert.rs(23 hunks)ops/op2/config.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/convert.rs (3)
core/runtime/ops_rust_to_v8.rs (9)
to_v8(37-40)to_v8(46-46)to_v8(77-85)to_v8(449-456)to_v8(463-470)to_v8(478-483)to_v8(575-580)to_v8(592-594)from(149-151)ops/op2/test_cases/sync/from_v8.rs (1)
from_v8(12-17)core/error.rs (6)
v(1663-1663)v(1666-1666)v(1669-1669)error(298-311)error(1801-1801)from(244-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-linux-arm / linux-arm test
- GitHub Check: build-windows / windows lint
- GitHub Check: build-linux-arm / linux-arm test-miri
- GitHub Check: build-windows / windows test
- GitHub Check: build-linux / linux coverage
- GitHub Check: build-linux / linux test
- GitHub Check: build-linux / linux test-miri
- GitHub Check: build-macos / macos lint
- GitHub Check: build-linux / linux lint-deps
- GitHub Check: build-macos / macos test
- GitHub Check: build-linux / linux test-ops
🔇 Additional comments (9)
ops/op2/config.rs (5)
35-37: Field types improved to useIdent.Changing from
Option<String>toOption<Ident>is a good improvement for type safety. Note that these fields aren't populated from attribute parsing anymore—they're likely derived from the function signature elsewhere.
101-105: Async mode handling looks correct.The change from
Async(Option<AsyncMode>)toAsync(AsyncMode)is consistent with the updated parsing, and the match arms properly set the corresponding config flags.
234-254: Async parsing requires explicit mode specification.The parsing now requires arguments via
require_list(), meaning#[op2(async)]alone will error—you must specify the mode like#[op2(async(lazy))]. This aligns with the PR's goal of inferring async from the function signature, making explicit async attributes mode-specific only.
442-447: Test case looks good.The
promise_idparsing test is straightforward and correct.
223-230:fast(...)parsing convertsTypeto string intentionally, with roundtrip validation.The pattern is tested and intentional—it stores generic types like
op_generic::<T>as strings for later use as alternative op references. The string is reparsed back toTypeindispatch_fast.rsline 392 with.expect(), meaning invalid alternatives will panic at compile time. This is already documented with a TODO comment atdispatch_fast.rsline 389 noting that alternatives are not validated. No changes needed here.core/convert.rs (4)
84-88: Fast-path traits +v8::Local/v8::Globalconversions look coherentThe introduction of
ToV8Fast/FromV8Fastand their use in the newV8ConvertError+v8::Local/v8::Globalimpls is consistent:
V8ConvertErrorcleanly wraps bothInfallibleandDataError.- The
TryInto-based impls forv8::Local<T>andv8::Global<T>correctly bridge between specific V8 handle types andv8::Valuewithout extra scope requirements.ToV8delegating toToV8FastandFromV8delegating toFromV8Fastfor these handles avoids duplication and matches the intended “fast-path” design.I don't see correctness issues or surprising behavior in this layering.
Also applies to: 136-140, 1081-1179
191-213: Numeric/boolFromV8refactor to fast paths is fine; confirmError = DataErrorimpactThe refactor of
Smi<T>,Number<T>, primitive numeric types, andboolso that:
FromV8forwards toFromV8Fast, and- their
Errorassociated type isDataErroris internally consistent and simplifies the implementation. Fast paths now own the actual conversion logic, with
FromV8just acting as a thin wrapper.However, using
DataErroras theErrortype for these publicFromV8impls is a behavioral API change compared to using a boxed error type (e.g.,JsErrorBox) and can affect downstream crates that mention<T as FromV8>::Errorin bounds or pattern-match on it. Please double-check that this change is acceptable from a semver/API-compat perspective fordeno_core.Also applies to: 270-291, 293-329, 341-360
675-699: Typed array andArrayBufferViewconversions look correct and efficientThe additions around typed arrays and
ArrayBufferViewlook solid:
Uint8Arrayand the macro-generated{Int16,Uint16,Int32,Uint32,BigInt64,BigUint64}Arraynow haveFromV8delegating toFromV8Fast, with fast paths gated by the appropriateis_*_arraychecks and clearBadTypeerrors.abview_to_vecplusmaybe_uninit_vec/transmute_vecis used in a standard way for zero-copy-ish extraction fromArrayBufferView, with size/alignment assertions guarding the unsafe transmute.ArrayBufferView’sFromV8Fastdispatches cleanly across all supported typed-array kinds and returns a preciseBadTypeerror when the value is not anArrayBufferView.Overall, this gives a good, fast typed-array story without obvious safety or semantic issues.
Also applies to: 716-785, 788-794, 795-872
1294-1302: Tests exercise the new option/typed-array conversions appropriatelyThe updated tests:
- For
OptionUndefined/OptionNull, verify both the JS→Rust (FromV8) and Rust→JS (ToV8) directions forundefined/nulland concrete numeric payloads.- For all typed array wrappers and
BigInt*variants, assert round-tripping throughFromV8andToV8, including empty arrays and theDataErrorshape in failure cases.This coverage is well aligned with the new conversion paths and should catch regressions in the option and typed-array handling.
Also applies to: 1329-1335, 1489-1497, 1514-1522, 1539-1547, 1564-1572, 1592-1594, 1614-1616, 1622-1624
|
what's the purpose behind this? |
|
This is for things we had discussed during the offsite |
nathanwhit
left a comment
nathanwhit
left a comment
There was a problem hiding this comment.
Looks good overall, just a bit confused on the FromV8Fast and ToV8Fast traits
core/convert.rs
Outdated
| ) -> Result<Self, Self::Error>; | ||
| } | ||
|
|
||
| pub trait FromV8Fast<'a>: Sized + FromV8<'a> { |
There was a problem hiding this comment.
Can you add a comment on this trait and why it exists? And like what requirement is there to impl FromV8Fast. Likewise for ToV8Fast
I'm guessing it's like "available in fast api" but it's not super clear. There's also nothing preventing anyone from implementing FromV8Fast for a random type, though not sure if that would be problematic in practice
There was a problem hiding this comment.
so it doesnt actually have much to do with v8 fast api, so maybe needs to be renamed.
We have ops which do not need to create a scope, FromV8Fast is what plays into that since FromV8 requires a scope. so maybe FromV8Fast should be named to FromV8Scopeless or something similar.
ToV8Fast is in a similar situation, however it is only implemented for Local and also is not handled by op2 in this PR, so maybe not worth including in this PR?
littledivy
left a comment
littledivy
left a comment
There was a problem hiding this comment.
Looks good. Let's land in Deno and run benchy to measure perf
This PR is split into multiple commits; I would recommend to review them separately.
This PR does the following changes:
asynckeyword on the macro declaration; it is fully inferred from the function definition. Theasyncalternatives (fake,lazy, &deferred) remain unchanged.globalattribute for arguments and returns types.FromV8Scopelesstrait, which is the default behaviour for arguments now, and requires ascopedattribute on arguments to use theFromV8trait.ToV8trait.from_v8andto_v8attributes are removed as these are now the default behaviour.FomV8/ToV8.There is denoland/deno#31607 ready for updating in CLI.
The
v8_slowattribute is kinda not greatly named, so maybe some bikeshedding on that is necessary