Skip to content
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

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 this issue Nov 26, 2024 · 5 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-dysfunctionalities Issues for the CFE not behaving as intended type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@denniskaselow
Copy link

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
Copy link
Collaborator

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.

@dart-github-bot dart-github-bot added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Nov 26, 2024
@eernstg
Copy link
Member

eernstg commented Nov 26, 2024

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?

@eernstg eernstg added area-front-end Use area-front-end for front end / CFE / kernel format related issues. and removed area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. labels Nov 26, 2024
@lrhn
Copy link
Member

lrhn commented Nov 26, 2024

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
Copy link
Author

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 johnniwinther added the cfe-dysfunctionalities Issues for the CFE not behaving as intended label Nov 27, 2024
@johnniwinther johnniwinther self-assigned this Nov 27, 2024
@johnniwinther
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-dysfunctionalities Issues for the CFE not behaving as intended type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

5 participants