#3180. Add tests for JsAnyOperation.equals()

[sgrekhov]

If this PR approach is OK, I'll rewrite the earlier tests this way.

[eernstg]

I can see that this PR uses the same approach as #3216 where I asked why some tests had two eval invocations and executed initialization of the global environment multiple times. So the questions there would be relevant for this PR as well.

[eernstg]

LGTM, with a couple of questions about the fact that certain declarations are evaluated more than once.

@srujzs, here's another one. Do tell us if we need to find other interop expert reviewers!

[sgrekhov]

I can see that this PR uses the same approach as #3216 where I asked why some tests had two eval invocations and executed initialization of the global environment multiple times. So the questions there would be relevant for this PR as well.

Answered in #3216 as well. A raw string doesn't support $... interpolation. Therefore we need the first eval to obtain underTest value from the function argument.

[srujzs]

If this PR approach is OK, I'll rewrite the earlier tests this way.

Oh, how funny - I didn't see this PR first and wrote comments about how I'd prefer this way in the other PRs. This makes more sense imo!

[eernstg]

@osa1, could you take a look at this? It looks like the WASM behavior differs from the expected behavior, so we need to know whether it's the expectations or the WASM implementation that should be adjusted. ;-)

[osa1]

It looks like the WASM behavior differs from the expected behavior

@eernstg which lines/tests do you mean? I run all of the tests here and they pass with dart2wasm.

[osa1]

The test cases LGTM, but similar @srujzs in #3216 (comment), I also wonder if this might be overkill. JSAny is just a reference to a JS object, and operator == simply call JS ===: https://github.com/dart-lang/sdk/blob/b67482d6bc21fd7462c98122086eae56450d7d11/sdk/lib/_internal/wasm/lib/js_helper.dart#L158-L159

I suspect it should be the same in dart2js as well.

So if Dart to JS and JS to Dart conversions are correct, then operator == cannot be wrong.

[sgrekhov]

It looks like the WASM behavior differs from the expected behavior

@eernstg which lines/tests do you mean? I run all of the tests here and they pass with dart2wasm.

I think it's just a wrong PR. @osa1 please, take a look at #3221 (comment)
Let's do what @srujzs suggests in a file LibTest/js_interop/JSAnyOperatorExtension/divide_t01.dart on the strings 39-40. Let's remove dartify() from them. Then the test starts passing with JS and fail with WASM. Is this expected?

For me it's strange that it passes on JS. {} / 2 produces NaN in JS and without dartify() we are comparing two JS objects but NaN == NaN is false in JS. So, the test should fail. Is this an issue or I'm missing anything?

Another example.

main() {
  Expect.isTrue(true.toJS.isTruthy);
}

This works with JS but fails with WASM. Adding .dartify() fixes it. But is this as designed and expected?

[osa1]

@sgrekhov thanks for the smaller repro. I think the output is as expected. isTruthy returns a JSBoolean, not bool:

https://api.dart.dev/dart-js_interop/JSAnyOperatorExtension/isTruthy.html

and operator == on JS types always check right-hand side type. So x == y where x is JSBoolean and y is bool returns false as the types are different.

Edit: to elaborate: dart:js_interop types are all extensions of a JS value reference type (JSValue in dart2wasm). Because they're extensions, the type check in operator == for the right-hand side type cannot distinguish JSBoolean from e.g. JSArrayBuffer, but it will distinguish JSBoolean from bool.

[eernstg]

About tests expecting not-a-number and failing because n != n when n is one of the special not-a-number values: It should be possible to use isNaN in JavaScript or in Dart, whichever is the correct place for that comparison.

[eernstg]

@osa1 wrote:

I think the output is as expected ...

Thanks, @osa1, that's very helpful!

[sgrekhov]

Hmm... According to @srujzs JSBoolean is extension type on bool and this explains why Expect.isTrue(true.toJS.isTruthy); works on JS without additional .dartify(). Probably it makes sense to update documentation. Please see https://api.dart.dev/dart-js_interop/JSBoolean-extension-type.html

JSBoolean extension type
A JavaScript boolean.

on
JSBooleanRepType
Implemented types
JSAny

And there's no any information about JSBooleanRepType which is extension type on Dart bool I guess (probably not directly).

[osa1]

@sgrekhov do you have the link to @srujzs's comment on this?

This behavior is the same in JSNumber as well, i.e. if you do x == y where x is JSNumber and y is double, they will never be equal in dart2wasm but they will be in dart2js. Example:

import 'dart:js_interop';
import 'expect.dart';

@JS('eval')
external void eval(String s);

@JS('foo')
external JSNumber foo();

main() {
  eval('''
    globalThis.foo = function() {
      return 123;
    };
  ''');

  print(foo());
  print(foo() == 123.0); // true in dart2js, false in dart2wasm
  print(foo() == 123);  // true in dart2js, false in dart2wasm
}

The reason is because in dart2wasm we have to box the JS references returned from JS calls, and once you box the references you can implement your own operator == where you check that the types are compatible.

In dart2js you don't have to box, and boxing would cost you performance. So if we make dart2js work like dart2wasm here, js_interop in dart2js will get slower, and we probably cannot afford that.

So we have three options:

1. Accept this difference in js_interop semantics.

2. Make dart2js work like dart2wasm: this would cost dart2js performance as it would have to box returned JS references to be able to implement a operator == that checks types and distinguishes JSBoolean from bool (and JSNumber from double).

   This would cost dart2js performance when using js_interop: every return value would need to be boxed. I suspect this would be unacceptable.

3. Make dart2wasm work like dart2js: this would require dartifying the reference when comparing it with a non-js-interop type.

   This would cost dart2wasm some performance, but I suspect it may be acceptable. Depending on the RHS type, we would call different dartify functions that can fail, so that if you compare with a bool you do dartifyBool, and if the receiver isn't a bool it fails and you return false. Same for other types.

   Because dart2wasm already needs to box all js_interop return values, this performance hit here is only in operator ==.

I don't see any other options.

@srujzs have you considered this case already? Do you have any opinions on the options above?

[sgrekhov]

@osa1 is there a way to detect wasm somehow? Currently we detect JS as final bool isJS = identical(1.0, 1);. Probably, depending on the above we'll need test like:

Expect.isTrue(true.toJS.isTruthy.dartify());
if (isJS) {
  Expect.isTrue(true.toJS.isTruthy);
}
if (isWasm) {
  Expect.isFalse(true.toJS.isTruthy);
}

[osa1]

@sgrekhov identical(1.0, 1) is the only way to do this in runtime that I'm aware of.

[srujzs]

And there's no any information about JSBooleanRepType which is extension type on Dart bool I guess (probably not directly).

It is an extension type on Dart bool directly in the JS compilers. Re: documentation, I agree it might be helpful to be clearer about the representation type differences, but I don't want to specify that the representation type is a specific type, because that may and should be allow to change. https://api.dart.dev/dart-js_interop/ specifies the runtime type differences and that identical checks won't work, but perhaps it should also mention that one shouldn't do == checks (or anything that calls that like Expect.equals) between a JS value and a Dart value.

My comment here about not using dartify is meant for the case where the two values are JS values, in which case == is okay and will do the sensible thing of checking the underlying values and not the boxes.

Do you have any opinions on the options above?

We could lint as a fourth option. We already lint for is and as in cases we think may result in platform differences. We could consider linting == and Expect.equals when the static types are different. With some of the more dynamic expectation testing in package:test (like passing a JS value as a "matcher" argument), it may be harder to catch though.

const bool isDart2Wasm = bool.fromEnvironment('dart.tool.dart2wasm');

I think this would be the preferable way to check for the compiler.

[osa1]

Thanks. So we accept this difference in semantics as expected.

Do we want to test bool vs. JSBoolean comparisons (and for other types) in this repo?

Given that this is not a specified behavior of js_interop (backends are allowed to do different things), I wonder if this should be tested in the SDK, some directory specific to dart2js, rather than in co19 as a language spec test.

[sgrekhov]

const bool isDart2Wasm = bool.fromEnvironment('dart.tool.dart2wasm');

It works, thank you!

https://api.dart.dev/dart-js_interop/ specifies the runtime type differences and that identical checks won't work, but perhaps it should also mention that one shouldn't do == checks (or anything that calls that like Expect.equals) between a JS value and a Dart value.

Now there is a note that one should do ==.

The types defined in this library only provide static guarantees. The runtime types differ based on the backend, so it is important to rely on static functionality like the conversion functions, for example toJS and not runtime mechanisms like type checks (is) and casts (as). Similarly, identical may return different results for the same JS value depending on the compiler. Use == to check for equality of two JS types instead.

I still don't have a clear understanding of what kind of tests we should write.

1. Equals and dartify.
   If the fourth option is preferrable (make a lint about ==) then how should we test that, say, globalContext returns a right value? Currently, the test below (without dartify()) works in dart2js but fails in dart2wasm.

main() {
  eval(r'''
    var jsVar = "jsVar";
  ''');
  Expect.equals("jsVar", globalContext["jsVar"]);
}

We could instead write something like:

main() {
  eval(r'''
    var jsVar = "jsVar";
  ''');
  Expect.equals("jsVar", globalContext["jsVar"].dartify());
  if (isJS) {
    Expect.equals("jsVar", globalContext["jsVar"]);
  }
  if (isWasm) {
    Expect.notEquals("jsVar", globalContext["jsVar"]); // If one day this fail it'd mean that the lint also should be removed
  }
}

@srujzs @osa1 @eernstg do we need the tests like the above? Or this, probably, makes sense for == operator only? And for all other cases just let's compare dartified values?

Honestly, I'd vote for option 3: to make dart2wasm and dart2js behave the same way. The code below looks very strange to me and requires a strong understanding of the internal workings of dart2wasm:

  // On dart2wasm
  print(globalContext["jsVar"]); // prints 'jsVar'
  print(globalContext["jsVar"] == "jsVar"); // false

Just saw @osa1's note that we’ve accepted the fact that the semantics of == differ between dart2js and dart2wasm. Ok, thank you!

2. Tests for JSAnyOperatorExtension.
   If all operations defined in JSAnyOperatorExtension are straightforward and simple, then I’ll simplify the tests. Just one check for each primitive type, array, function, and object. Don't check for specific values like 42, 0, -1, 3.13, -0.0; just verify that 42 + "" in JS produces the same result as 42.toJS.add("".toJS) in Dart. Use dartify() to compare the results (otherwise, we'll get different results on different compilers). For == check the results with dartify() and without it.

Does that work for you?

[osa1]

Thinking about this more, the lint we mention above already exists: https://dart.dev/tools/linter-rules/unrelated_type_equality_checks

The issue with equality is not specific to js_interop types, so the lint above should work.

Regarding option (3): I realized that it's actually not easily possible without potentically sacrificing a lot of performance in dart2wasm. The problem is operator == should be symmetric, e.g. a == b and b == a should return the same for any a and b. That means if we type check the right-hand side in JSValue's operator == and make JSBoolean equal to bool when the values are right, we have to add the type test to bool as well to handle JSBoolean on the right-hand side. Same for number types, strings, etc.

The code below looks very strange to me and requires a strong understanding of the internal workings of dart2wasm

I'm not sure if I agree.. You're comparing JSAny? with String. By default you should assume that it will return false as the types don't match. So arguably dart2wasm's behavior makes more sense. The lint above should warn you about this code.

We accept this difference between dart2js and dart2wasm not because it's the ideal language design, but for pragmatic reasons. Wasm and JS are different targets with different limitations and performance. If we want both of these backends to be useful we have to accept this kind of thing sometimes.

---

For testing JSAnyOperatorExtension methods, if we test that Dart values are converted to JS as expected and JS values are converted to Dart as expected, basic checks that e.g. add really adds and doesn't subtract should be enough. I think we don't need or want to test each operator for each combination of js_interop value types.

We don't want to test JS + function semantics. We just want to test that JSAnyOperatorExtension.add calls JS +.

So I think for each operation we want a few cases that to check that the right JS function is called. For add I think the case you show adding a number to string is a good one as no other JS operation (I assume) will give us the same result. Comparing the expected values with dartify is also a good idea as that will give us the expected results on both dart2js and dart2wasm.

[eernstg]

Hi @osa1, @srujzs, and @sgrekhov, there's a lot of discussion here. I think the semantics issue has been settled:

So we accept this difference in semantics as expected.

which means that tests should expect WASM or JS compilation in those cases, and specify different expectations. Next, the desired level of coverage is addressed here:

We don't want to test JS + function semantics. We just want to test that JSAnyOperatorExtension.add calls JS +.

So the tests would then be simplified considerably. Presumably that would be done by adding more commits to this PR, as well as a few others?

[sgrekhov]

So the tests would then be simplified considerably. Presumably that would be done by adding more commits to this PR, as well as a few others?

No. I created #3223 as a replacement for this and the others. Please review it. If that PR is landed, I'll close this and the other PRs.

[srujzs]

Now there is a note that one should do ==.

Well, with a clause that that's for comparing two JS values, not a JS value and a Dart value. :) But yes, we should tell users not to use == between a Dart and a JS value.

Expect.equals("jsVar", globalContext["jsVar"]);

The final PR converts both values to JS values but we could also go the other way: (globalContext["jsVar"] as JSString).toDart) (which is similar to dartify but is more explicit). Neither way is better, and they're both "correct" in terms of comparing.

Does that work for you?

Yes, thanks for simplifying here and all the work here, @sgrekhov!

Thinking about this more, the lint we mention above already exists: https://dart.dev/tools/linter-rules/unrelated_type_equality_checks

Ah yeah, and it's in core too. The one downside is Expect.equals isn't covered here or any of the test expectations, but that's test-only code, so that's less of an issue.

That means if we type check the right-hand side in JSValue's operator == and make JSBoolean equal to bool when the values are right, we have to add the type test to bool as well to handle JSBoolean on the right-hand side. Same for number types, strings, etc.

Agreed. I think poking a hole into == to account for boxing differences is going to be costly. I'd rather make it more obvious to users that they shouldn't be using == in that manner.

[sgrekhov]

Replaced by #3223