[dart devc] Pattern matching use same variable name cause unexpected behavior

[huanghui1998hhh]

Steps to reproduce

Create a flutter project and run flutter web on debug mode with this code:

void main() {
  String? fooValue = 'hello world';
  if (fooValue case final String fooValue) {
    print(fooValue);
  }
}

And this is it's js result:

  main.main = function main$() {
    let fooValue = "hello world";
    {
      let fooValue = null;
      let t$36$350$350 = fooValue;
      if (typeof t$36$350$350 == 'string') {
        fooValue = t$36$350$350;
        {
          core.print(fooValue);
        }
      }
    }
  };

So it will not print 'hello world'.

[eernstg]

Drive-by comment (I'm working on the language, not on the implementation of compilation to JavaScript in any tool).

@huanghui1998hhh, what's your configuration? With dart compile js on your example program (no optimization options) I get the following:

    main() {
      A.printString("hello world");
    }

Perhaps your issue should be classified as a DDC issue? Also, the kernel code looks as follows (but note that there may be differences between the kernel code generated for different backends):

  static method main() → void {
    core::String? fooValue = "hello world";
    {
      final hoisted core::String fooValue;
      final synthesized core::String #0#0 = fooValue{core::String};
      if(#0#0 is core::String) {
        fooValue = #0#0;
        {
          core::print(fooValue);
        }
      }
    }
  }

It looks similar to your JavaScript code, but there may be some rules about the meaning of hoisted that matter.

[mraleph]

@eernstg the issue title indicates it's a Flutter Web debug mode - which means DDC.

[mraleph]

cc @nshahan

[huanghui1998hhh]

Yes, this is a DDC issue. Maybe I should use tag dart devc. sry

[nshahan]

@johnniwinther

Inspecting the kernel for this example shows that the shadowing fooValue variable gets declared before the temporary used in the pattern which is too early. Is this something specific to the lowering used in DDC?

[rakudrama]

Look deeper into the 'shadowing'.
I think it is possible that Kernel can represent a program where every variable is renamed to x, but they are disambiguated since the references point to the declaration. There is no lookup of the name to find the variable, parsing to Kernel has done that already, so the name has no effect on the semantics.

DDC could fix this by generating different JavaScript names for shadowed names, either avoiding shadowed names entirely, or doing some analysis to tell if the separate JavaScript names are needed to express the correct lookup in the generated code's scope structure.

The Kernel printer could also disambiguate 'shadowing' by adding a suffix or other annotation to distinguish variables in nested scopes with the same name , e.g. using fooValue₁ instead of fooValue for one of the variables.

[johnniwinther]

It is unfortunate that the lowering introduces was looks like a shadowing. I'll see if we can avoid this.

[rakudrama]

There is a broader issue - the user program has two variables with the same name.
How does one debug a program with shadowed variables?

I believe currently that the DDC generated code has a JavaScript scope model that is mostly in alignment with the Dart model, so the debug experience works.
Ideally it should be possible during single-stepping to assign to variables and continue.

Johnni - what changes were you thinking of?

The CFE should not change user variable names. It might be possible to rearrange the lowered code so that the scopes don't 'interleave' like in the Kernel snippet. Can that the scopes be arranged to be non-interleaved in the general case?
If the interleaving is necessary, how does Kernel indicate which variables are visible on the Dart scope chain?

/cc @annagrin

[nshahan]

I agree, there is shadowing present in the original source code and it would provide the most consistent user experience in the debugger if the shadowing still exists. Along with a change to where the hoisted variable is declared so that doesn't break the initialization of the synthetic variable.

[johnniwinther]

I've created https://dart-review.googlesource.com/c/sdk/+/345942 which fixes the problem created by the CFE: That a legal shadowing (old variable read before the new was declared) was turned into an illegal "shadowing" (the old variable read after the new was declared).

The CL doesn't change the names of the variables, so the shadowing vs debugging must still be addressed.

[nshahan]

@johnniwinther Thanks for the quick fix!

[parlough]

Thanks for the fix! :D

Since you were looking for feedback on if this is worth being cherry-picked, I'd personally say yes, but probably only for 3.3.

I'd like to encourage developers to use if-case more when needing type promotion of a field rather than shadowing a field with a local variable (dart-lang/site-www#5461), but I'm hesitant if this will be causing confusion for a few months. In most cases it makes sense to provide a more specific name, but when trying to type promote a field, many developers likely choose to use the same name.

If it resulted in a compile-time error, I think it would be less important, but since it doesn't, I think this behavior could result in misconceptions, especially if they mostly develop or test their apps with debug web builds.

[huanghui1998hhh]

In my opinion(as a Front-end developer), local variable still have some advantage. Sometimes, we may face the following situation, there have a lot of conditional control, so we have to use early return. So the if-case is not applicable. Maybe dart need something like guard in Swift Language.

[parlough]

In my opinion...

@huanghui1998hhh Yep, I agree! If you want early return, a local variable is still necessary. See (and maybe react to) dart-lang/language#2537 for a potential solution to this :)

Thanks for reporting this issue by the way!

[nshahan]

@parlough

I'd like to encourage developers to use if-case more when needing type promotion of a field rather than shadowing a field with a local variable (dart-lang/site-www#5461), but I'm hesitant if this will be causing confusion for a few months. In most cases it makes sense to provide a more specific name, but when trying to type promote a field, many developers likely choose to use the same name.

Using an if-case for promotion was not the issue causing a bug here. The bug occurs when using an if-case to introduce a new local variable that shadows another.

This triggered the issue:

String? fooValue = 'hello world';
if (fooValue case final String fooValue) {
  print(fooValue);
}

The issue is not triggered when the new variable doesn't shadow the earlier local:

String? fooValue = 'hello world';
if (fooValue case final String newFooValue) {
  print(newFooValue);
}

Also, using a type match pattern without introducing any local variable does not trigger the issue either, while giving promotion where it is possible:

class C {
  final String? _fooValue = 'hello world';
  void fn() {
    if (_fooValue case String()) {
      print(_fooValue);
    }
  }
}

Does that change your opinion of the need to get this fix in 3.3?

[parlough]

Using an if-case for promotion was not the issue causing a bug here. The bug occurs when using an if-case to introduce a new local variable that shadows another.

@nshahan Ahh I see! I didn't realize this issue only occurred for local variables, not instance variables (fields).

I also misspecified, sorry about that. I meant as a workaround when field promotion doesn't work. Because (for better or worse) many cases I've seen of using if-case as a workaround do shadow the field name.

If it doesn't occur for fields, I'm definitely not as worried about it! It can always be reevaluated if more developers end up hitting this.

Thanks for explaining :)