Failures on [tests] Remove triple shift test skips from status files

[nshahan]

There are new test failures on [tests] Remove triple shift test skips from status files.

The test

co19/LanguageFeatures/Triple-Shift/Constants_A01_t05/01 Pass (expected CompileTimeError)

is failing on configurations

dart2js-hostasserts-strong-linux-x64-chrome
dart2js-hostasserts-weak-linux-x64-chrome

cc @rakudrama @johnniwinther

[rakudrama]

This looks like an analyzer bug:

 --- Command "dart2analyzer" (took 33ms):
 DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dartanalyzer --enable-experiment=triple-shift --ignore-unrecognized-flags --packages=/b/s/w/ir/cache/builder/sdk/.packages --format=machine --no-hints /b/s/w/ir/cache/builder/sdk/tests/co19/src/LanguageFeatures/Triple-Shift/Constants_A01_t02.dart
 unexpected analysis errors:
 In tests/co19/src/LanguageFeatures/Triple-Shift/Constants_A01_t02.dart:
 - Line 26, column 19: COMPILE_TIME_ERROR.CONST_EVAL_THROWS_EXCEPTION
   Evaluation of this constant expression throws an exception.
 --- Re-run this test:
 python tools/test.py -n analyzer-asserts-strong-linux co19/LanguageFeatures/Triple-Shift/Constants_A01_t02

What might be happening is that (-2 >>> 1) is incorrectly evaluated.
The results of int.>>> should always be non-negative, but if it was negative, that would make line 26 an error.

...
main() {
  const int c1 = (-2 >>> 1);
  Expect.equals(-2 >>> 1, c1);

  const int c2 = (125 >>> c1);           // Line 26
  Expect.equals(125 >>> c1, c2);
...

[nshahan]

cc @srawlins

[srawlins]

I just landed a change for >> in const evaluation. e267fb2

[nshahan]

Actually I think this might be a CFE error because there is a missing compile time error.

The code in question is:

...
class MyClass {
  final int a;
  const MyClass(i1, i2) : a = (i1 >>> i2);
}

main() {
  const MyClass c1 = MyClass(1.0, 1);       //# 01: compile-time error
...

The VM gets a compile time error because the constant double 1.0 does not have the triple shift operator. Both of the web backends are not getting that compile time error.

@johnniwinther Is there some check for constants that should be applied to the Targets for the web compilers as well? Or is it made more complicated by our number representation?

[johnniwinther]

The numbers in constants are treated wrt. runtime semantics so I would assume that 1.0 is treated as 1 and therefore does have a >>> operator.

@fishythefish Do you agree?

[fishythefish]

No, I think the CFE should be producing consistent compile-time behavior across the backends here, as we do for other operations.

Consider:

void main() {
  // Error:
  /*
  const x = 1.0 << 1;
  print(x);
  */

  print(foo(1.0)); // 2
}

@pragma('dart2js:noInline')
int foo(x) => x << 1;

[fishythefish]

FWIW, perhaps this should be part of a larger discussion on web number semantics. My understanding is that, historically, we attempted to conform to the spec/VM wherever possible, and any deviations at runtime were seen as unavoidable compromises in the name of performance or because we can't distinguish between int and double. Thus we have situations like the one above, where the compile-time error we see reflects what the spec says, but disagrees with the same computation done dynamically.

Now that we have separate constant evaluation for web backends and we've revised how the spec/documentation specify number semantics across platforms, maybe we should revisit this?

[nshahan]

For me, it would be confusing to see a compile time error (red squiggles) in your IDE but no error when you actually compile. I think that is what would happen because the analyzer would treat this as an error but if it happens to be in a web project then it would be allowed at compile time.

Can the analyzer know what type of project is being viewed and give different errors?

[srawlins]

Can the analyzer know what type of project is being viewed and give different errors?

I don't think we have any mechanism for doing that. And most (perhaps?) code is agnostic; could be compiled with VM or dart2js or DDC.

[johnniwinther]

@fishythefish Do note that the choice of using runtime number semantics in constant evaluation is not for producing (or not producing) compile-time error but to ensure that constant expressions evaluate to the same value as their runtime equivalent. In this case the omitted compile-time error is just a side effect of that.

One could rewrite the program so it doesn't have a compile-time error (in neither CFE nor analyzer) but needs the JavaScript semantics in the constant evaluator to match that of the runtime:

const bool isWeb = identical(1, 1.0);
const dynamic a = 1;
const dynamic b = 1.0;
class MyClass {
  final int a;
  const MyClass(i1, i2) : a = (i1 >>> i2);
}

main() {
  print(identical(1, 1.0)); // This will be true at runtime, `isWeb` needs to be true at compile-time.
  const MyClass c1 = MyClass(isWeb ? b : a, 1); 
  MyClass c2 = MyClass(a, 1); 
  print(identical(c1.a, c2.a));
}

[fishythefish]

@johnniwinther I agree that we want to get the same result from compile-time constant evaluation as we do from runtime evaluation, provided that the code compiles. (Side note: we still have some holes in this. For example, double.infinity is int currently evaluates to false with constant evaluation, but to true at runtime.)

Per #45438, which summarizes a discussion from the web team meeting yesterday, I think the consensus is that const x = 1.0 >>> 1; should not compile. The fact that we can provide runtime semantics for it is unintentional, and it would be nice to add some strictness on our end so that we can provide analogous runtime errors. Moreover, this means that triple shift will behave just like the other shift operators do, and if we want to change any of that behavior, we should do it all at once later on, rather than have >>> be the odd one out.

[franklinyow]

Have we decided who own this?

[johnniwinther]

I've added https://dart-review.googlesource.com/c/sdk/+/193406 to document the current behavior.