dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.31k stars 1.59k forks source link

`if case` with type check turns values to null if not covered in the first `if case` and the assigned name is the same as the local variable #59613

Open denniskaselow opened 1 week ago

denniskaselow commented 1 week ago

This code:

void main() {
  for (final thing in [42.42, 'foo', Object()]) {
    if (thing case final double thing) {
      print('$thing is double');
    } else if (thing case final String thing) {
      print('$thing is String');
    } else if (thing case final Object thing) {
      print('$thing is Object');
    } else  {
      // analyzer warns this is dead code, but it gets executed because thing is null
      print('$thing is unknown ${thing.runtimeType}');
    }
    // accessing thing here is still the original value
  }
}

Outputs:

42.42 is double
null is unknown Null
null is unknown Null

Expected output:

42.42 is double
foo is String
Instance of 'Object' is Object

The output is correct if I don't assign the loop variable to a variable with the same name in the if case.

A loop isn't required for this bug, something like final thing = ['foo', 1].first; has the same problem.

Simply using final thing = 'foo'; before the if case is even worse:

DartPad caught unhandled JSNoSuchMethodError:
TypeError: can't access property Symbol("dartx.runtimeType"), thing is null
unparsed                                                                  TypeError: can't access property Symbol("dartx.runtimeType"), thing is null
blob:null/893fe534-725f-4a0f-a513-3941f5589dd5 444:18                     load__dartpad_main.<fn>.<fn>
https://storage.googleapis.com/nnbd_artifacts/3.5.4/dart_sdk.js 48513:14  _rootRun
https://storage.googleapis.com/nnbd_artifacts/3.5.4/dart_sdk.js 47560:14  run
https://storage.googleapis.com/nnbd_artifacts/3.5.4/dart_sdk.js 48660:92  _runZoned
https://storage.googleapis.com/nnbd_artifacts/3.5.4/dart_sdk.js 48618:18  runZoned
blob:null/893fe534-725f-4a0f-a513-3941f5589dd5 442:20                     capture
Stack trace truncated and adjusted by DartPad...

Tested on DartPad, stable and main: Dart SDK 3.5.4 and Flutter SDK 3.24.5 Dart SDK 3.7.0-183.0.dev and Flutter SDK 3.27.0-1.0.pre.623

dart-github-bot commented 1 week ago

Summary: if case statements with identically named variables shadow the original, causing unexpected null values if the first if case condition isn't met. This leads to runtime errors.

eernstg commented 1 week ago

The following variant behaves as expected:

void main() {
  for (final thing in [42.42, 'foo', Object()]) {
    if (thing case final double t1) {
      print('$t1 is double');
    } else if (thing case final String t2) {
      print('$t2 is String');
    } else if (thing case final Object t3) {
      print('$t3 is Object');
    } else {
      // analyzer warns this is dead code
      print('$thing is unknown ${thing.runtimeType}');
    }
    // accessing thing here is still the original value
  }
}

Sounds like a code generation bug in the common front end. I think there was an issue with some of the same elements: The use of several variables with the same name around patterns gave rise to wrong code generation in some cases. @johnniwinther, WDYT?

lrhn commented 1 week ago

This definitely looks like a bug in the compiler.

The scope of the new thing in the if-case statement:

if (thing case final double thing) {
   print('$thing is double');
} else ...

should be the when clause (if there had been one) and the true branch. The new variable must not be in scope in the else branch. Or if it is, if we accidentally specified it badly, the variable must not be considered initialized in the else branch (because it isn't). Reading null from it is just plain wrong, and unsound, since every variable here is non-nullable.

void main() {
  test(42);
}

void test(Object o) {
  late ({Object Function() get, void Function(Object) set}) closures;
  if (o case 37 && final num o && final num z) {
    print("Wat?");
    o == 87; // Not an error?
  } else {
    print(o); // Prints null.
    // print(z); // Error: Undefined name 'z'.
    print([o].runtimeType); // Prints (effectively) `List<Object>`, so unsound to have value `null`!
    o = "a";
    print(o); // Prints a
    closures = (get: () => o, set: (v) => o = v);
  }
  print(o); // Prints 42
  closures.set(false);
  print(o); // Prints 42
  print(closures.get()); // Prints false
}

The final num o seems to introduce a new variable, is not final like it should be, and which leaks into the else branch of the if, which it shouldn't do. If we change the pattern to final num o when o == 37 instead, the value of the o variable in the else branch will start as 42 instead.

denniskaselow commented 6 days ago

BTW, I only tested on DartPad, but now that I did further testing, the error only occurs when compiling using ddc. It works as expected with dart run, dart compile exe, when creating a web release build and with wasm.

johnniwinther commented 6 days ago

The CFE encoding is technically correct since it doesn't rely on the names of variables but instead references to variable declarations in its encoding. Unfortunately DDC assumes that names can be used, so because the generated code introduces shadowing, using the names fails.

I'm working on a fix.

cc @nshahan