Skip to content

#3180. Add tests for NullableUndefineableJSAnyExtension. #3205

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgrekhov
Copy link
Contributor

No description provided.

@sgrekhov sgrekhov marked this pull request as draft May 27, 2025 13:06
@sgrekhov sgrekhov marked this pull request as ready for review May 27, 2025 13:16
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.

@srujzs, it would be great if you could take a look at this as well!


Expect.isTrue(globalContext["n"].isNull);
Expect.isFalse(42.toJS.isNull);
Expect.isFalse(globalContext["u"].isNull);
Copy link
Member

Choose a reason for hiding this comment

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

This is the most interesting case. I don't know whether or not isNull should see any difference between null and undefined.

The documentation says

Currently, there is no way to distinguish between JavaScript undefined
and JavaScript null when compiling to Wasm. Therefore, this getter
should only be used for code that compiles to JavaScript and will throw
when compiling to Wasm.

.. so there may be a need to treat compilation to Wasm and compilation to JS differently.

Copy link

Choose a reason for hiding this comment

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

Thanks for the cc, Erik! And thanks for adding tests, Sergey!

Yes, when compiling to Wasm, we're forced to convert to Wasm's null ref regardless of whether the JS value is undefined or null in order to maintain the same nullability semantics for interop across all compilers, so there's no way of distinguishing.

In some future, we may provide an opt-in annotation users to preserve undefined separately, but that doesn't exist yet.

So, both isNull and isUndefined need to be handled differently based on the platforms.

var s = "s";
''');

Expect.isFalse(globalContext["n"].isUndefined);
Copy link
Member

Choose a reason for hiding this comment

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

This would again rely on the ability for Dart code to see a difference between JS null and JS undefined. Similarly for line 27.

@eernstg
Copy link
Member

eernstg commented May 28, 2025

Thanks, @srujzs!

@sgrekhov, do we use status files to enable a test for specific platforms?

@sgrekhov
Copy link
Contributor Author

Yes, we use status files for these purposes. But in this particular case I think that it's simpler to use if(isJS) {/* Code, not supported by Wasm*/}

@eernstg
Copy link
Member

eernstg commented May 28, 2025

it's simpler to use if(isJS) {/* Code, not supported by Wasm*/}

OK. I'm just wondering, if this test is executed on any platform where the generated code isn't JavaScript, presumably the eval thing wouldn't work. So how can we even use isJS if part of the test will be a compile-time error when isJS is false?

@srujzs
Copy link

srujzs commented May 28, 2025

if this test is executed on any platform where the generated code isn't JavaScript, presumably the eval thing wouldn't work

eval here is an interop method that interfaces https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval, so it should work even when compiling to Wasm. We use it often in the SDK tests for a quick way to set up the JS environment.

@sgrekhov
Copy link
Contributor Author

After rereading the docs... @srujzs is using of isNull/isUndefined a compile-time error on Wasm?

@srujzs
Copy link

srujzs commented May 28, 2025

It's a runtime-error (you specifically get an UnimplementedError) when compiling to Wasm.

@sgrekhov
Copy link
Contributor Author

Thank you. I thought that it throws on undefined values only. Ok, if so, then I'll switch off isNull and isUndefined tests in status files. @eernstg PTAL.

@sgrekhov sgrekhov requested a review from eernstg May 29, 2025 07:08
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.

3 participants