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.11k stars 1.56k forks source link

Behaviour of `Move to file` when it finds references for private elements #56791

Open FMorschel opened 1 day ago

FMorschel commented 1 day ago

When trying to extract a method that references something it will miss if extracted (variable from inside that context), it gives a warning and stops the process. But if we do the same with the new "Move class to file", currently we don't receive any warning (when it references something private from that library) and when extracted we can get lots of missing references even if it adds the import for the source file on the destination.


Today if you have the following and you try to extract the method, you get:

for (int i = 0; i < 1; i++)
Switch(
  value: true,
  onChanged: (v) {
    debugPrint('Switch value: $v - $i');
  },
),
Cannot extract the closure as a method, it references the external variable 'i'.

But if you have a class that depends on any form of a private class/function/variable/constant we get no errors, and the new file created gets the import for the original file even though it will trigger unused_import if the only elements from it that it has a dependency are private.

Source:

class C {
  void foo(_C c) {}
}

class _C {}

Destination after the move:

import 'a.dart';      // unused_import

class C {
  void foo(_C c) {}   // `_C` triggers undefined_class
}

Source after the move:

class _C {}

Some similar behaviour would happen if I tried moving _C, it would import the destination on the source file but the same warnings would appear.


Should we consider adding the same kind of message in this case?

Related to consider https://github.com/dart-lang/sdk/issues/56392, and the fact that it moves sealed classes and its child classes together.

dart-github-bot commented 1 day ago

Summary: The "Move to file" refactoring fails to warn users when moving a class that references private elements from another file. This results in missing references and unused imports in the generated code, leading to potential errors and code maintenance issues.

FMorschel commented 1 day ago

From @DanTup's comment https://github.com/dart-lang/sdk/issues/56392#issuecomment-2376971563:

Perhaps there should be a prompt though to tell you that the operation will result in analysis warnings and let you choose to proceed (like we do with renaming something that would shadow, and give a "Rename anyway?" button).