Skip to content

#3180. Add tests for ObjectToJSBoxedDartObject #3291

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 29, 2025
Merged

#3180. Add tests for ObjectToJSBoxedDartObject #3291

merged 7 commits into from
Aug 29, 2025

Conversation

sgrekhov
Copy link
Contributor

No description provided.

@sgrekhov sgrekhov requested review from eernstg and srujzs August 27, 2025 14:09
srujzs
srujzs approved these changes Aug 28, 2025
View reviewed changes
Copy link

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

One thing worth checking for: what happens if we call toJSBox on an ExternalDartReference? I suspect we're inconsistent there in some cases. :) dart2wasm will always throw while ddc/dart2js will throw only if the original value in the ExternalDartReference were to throw.

I'm not sure we actually have a good fix for that as we would need to distinguish the result of toExternalReference from the original object, and I don't see a good way of doing that with dart2js/ddc unless we wrap the value (defeating the original purpose of performance).

@sgrekhov
Copy link
Contributor Author

One thing worth checking for: what happens if we call toJSBox on an ExternalDartReference?

https://github.com/dart-lang/sdk/blob/a29e08c72e2ce21813c1edf50cbcdfcac7a7acdd/sdk/lib/js_interop/js_interop.dart#L446-L448

Extension type ExternalDartReference doesn't implement Object and therefore doesn't have toJsBox getter, defined as member of extension on Object.

Descriptions updated. PTAL.

@sgrekhov sgrekhov requested a review from srujzs August 28, 2025 07:48
srujzs
srujzs approved these changes Aug 28, 2025
View reviewed changes
Copy link

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

Extension type ExternalDartReference doesn't implement Object and therefore doesn't have toJsBox getter, defined as member of extension on Object.

Ha! That ended up being convenient. :)

@sgrekhov
Copy link
Contributor Author

@eernstg can we land this PR?

eernstg
eernstg reviewed Aug 29, 2025
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Looks good! I think it would be helpful to add a few words to a description, then land it.

@sgrekhov sgrekhov requested a review from eernstg August 29, 2025 11:07
eernstg
eernstg approved these changes Aug 29, 2025
View reviewed changes
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

LGTM

@eernstg eernstg merged commit 677a491 into dart-lang:master Aug 29, 2025
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 29, 2025
@sgrekhov
[co19] Roll co19 to 677a4913274d00d7e8bdfb60c8dbbe6d64904b3b
385bd8f 
2025-08-29 [email protected] dart-lang/co19#3180. Add tests for `ObjectToJSBoxedDartObject` (dart-lang/co19#3291)
2025-08-29 [email protected] Fixes dart-lang/co19#3292. Ignore some analyzer warnings, add issue numbers (dart-lang/co19#3295)
2025-08-29 [email protected] dart-lang/co19#3180. Fix delete_A01_t06.dart to work in both strict/non-strict modes (dart-lang/co19#3290)
2025-08-28 [email protected] dart-lang/co19#3292. Update assertions for `reachability_switch_*` tests (dart-lang/co19#3293)
2025-08-27 [email protected] dart-lang/co19#3180. Add tests for `JSObjectUnsafeUtilExtension` (dart-lang/co19#3289)
2025-08-26 [email protected] dart-lang/co19#3180. Add JSFunctionUnsafeUtilExtension tests (dart-lang/co19#3285)
2025-08-26 [email protected] dart-lang/co19#3180. Add `callMethod` and `callMethodVarArgs` tests (dart-lang/co19#3288)
2025-08-25 [email protected] dart-lang/co19#3180. Add tests for `createJSInteropWrapper()` function (dart-lang/co19#3283)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,dart2js-minified-linux-d8-try
Change-Id: I8188f94fcf0ffbfbb4c117abb80676f9675fb06e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447721
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Erik Ernst <[email protected]>
Reviewed-by: Ivan Inozemtsev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eernstg eernstg eernstg approved these changes

@srujzs srujzs srujzs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sgrekhov @eernstg @srujzs