"Add missing switch cases" fix doesn't account for shadowed names

[DanTup]

This was originally raised at Dart-Code/Dart-Code#5366 by @klukshu.

Given:

a.dart

sealed class Attribute {}
class Attribute1 extends Attribute {}
class Attribute2 extends Attribute {}

switch.dart

import 'a.dart';

Object? f(Attribute attribute) {
  return switch (attribute) {
    Attribute1() => throw UnimplementedError(),
    Attribute2() => throw UnimplementedError(),
  };
}

class Attribute1 {}

The Attribute1 in the switch case is not the correct one (this produces no warning - which is #56338), but using the quick fix to "Add missing switch cases" adds code that does the same:

  return switch (attribute) {
    Attribute1() => throw UnimplementedError(),
    Attribute2() => throw UnimplementedError(),
    // TODO: Handle this case.
    Attribute1() => throw UnimplementedError(),
  };

This results in the original diagnostic (not exhaustively matched) remaining, but an additional one for "this case is covered by previous cases").

I'm not sure what the right fix is though, probably it would require adding an import with a prefix, but I'm not sure if that's the fix the user would want?

[FMorschel]

I'm not sure what the right fix is though, probably it would require adding an import with a prefix, but I'm not sure if that's the fix the user would want?

I'd say it is the only possible fix if the class is declared on the same file (like the above) besides creating a typedef-like code somewhere. The code below shows the two ways I could think of to make this work (without - obviously - renaming classes). I simply added that import with prefix here for the example but doing that in any imported file would do, so the prefix may not be necessary.

import 'b.dart';
import 'b.dart' as b show Attribute1;

typedef AttributeOne = b.Attribute1;

extension type _AttributeOne(b.Attribute1 _) implements b.Attribute1 {}

Object? foo(Attribute attribute) {
  var _ = switch (attribute) {
    _AttributeOne() => 0,
    Attribute2() => 0,
  };
  return switch (attribute) {
    AttributeOne() => throw UnimplementedError(),
    Attribute2() => throw UnimplementedError(),
  };
}

class Attribute1 {}

[scheglov]

Usually we report a warning in case of such unrelated patterns, probably something to improve.
Maybe TypeSystemImpl.canBeSubtypeOf

And we should use an import with a unique prefix to reference such shadowed names.
This is probably an issue in DartEditBuilderImpl.writeReference2

[FMorschel]

@scheglov please take a look at #56338 as Dan mentioned above.

[FMorschel]

A similar error occurs when you have:

a.dart

class A {}
A foo() => A();

main.dart

class A {}

void main() {
  var a1 = A();
  var a2 = foo();
}

And now you try using Add type annotation to a2.

Also if you have in this case something like:

  var list = [foo()];
  list.forEach((e) => print(e));

With both avoid_function_literals_in_foreach_calls and always_specify_types the same error as above happens. I'm unsure if I should file each one in a new issue now.

[bwilkerson]

These probably all have the same root cause, so one issue is probably enough.

[FMorschel]

I'm looking at this. I've found all the information I need to fix this. I only came here to ask:

1. Where should we define a basic String Function(Uri) importPrefixGenerator implementation?
2. How should that implementation be?

---

My ideas:

1. Either on DartFileEditBuilderImpl itself (if the received field is null, we use our own impl), or inside ResolvedCorrectionProducer or something like that (which I'm more hesitant about since in this case whenever we do use the builder to write a type we must remember to pass that in as a parameter).
2. I've been thinking something in the lines of p* with * being a sequential number we try. So if there is already a p1 in the library (at the point of the edit - not sure yet if we know this, but I believe it should be doable), we skip it, and so on until we find a suitable number. The implementation is supposed to be simple enough that the user can easily rename the import and not try (at least right now) to come up with a smart name.

---

The current local impl would do:

a.dart:

class A {}

other.dart:

import 'a.dart';
A getA() => A();

main.dart:

import 'other.dart';

class A {}
void f() {
  var ^v = getA();
}

Main would ask for Add type annotation and it would:

import 'package:test/a.dart' as p0;

import 'other.dart';

class A {}
void f() {
  p0.A v = getA();
}

---

The only other case I couldn't yet imagine how to handle was if, for the above, getA was defined in the same library as the A it uses (say a.dart). This would mean that the import is already there, and we would actually need to refactor-rename all things from that import to use the new prefix (such as p0.getA).

If you have any ideas about this problem, I'd love to hear them, but while we don't accept refactors as fixes, we don't have a way to handle this.

[bwilkerson]

Where should we define a basic String Function(Uri) importPrefixGenerator implementation?

I would prefer to have it somewhere like DartFileEditBuilderImpl so that (a) it can be used by any code that's building an edit and (b) all of the logic around building the final edits that apply to imports can be in one place.

How should that implementation be?

Using a name like pN is a reasonable first step. Ideally I'd like to get to the point where we can use the library name when its unique, such as using math for an import of dart:math, or path for an import of package:path/path.dart, but it's fine if that doesn't get implemented now (and probably better if it's a separate CL).