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

Rename(refactor) on method's parameter not applying to method in super/sub class. #53187

Open hamsbrar opened 1 year ago

hamsbrar commented 1 year ago
class T1 {
  void f(int a) {}
}

class T2 extends T1 {
  @override
  void f(int a) {}
}

Renaming a in T1.f doesn't rename a in T2.f.

> dart --version
Dart SDK version: 3.0.7 (stable) (Mon Jul 24 13:17:56 2023 +0000) on "linux_x64"

> code --list-extensions --show-versions | grep dart-code
Dart-Code.dart-code@3.68.1

I think rename a in T1.f should also rename a in T2.f and vice-versa. And if there are more types in hierarchy that has f, then it should be applied to those as well.

scheglov commented 1 year ago

Strongly speaking semantics does not require this, in contrast to named formal parameters, which we do rename.

But I agree that it would be reasonable to have an option to rename even positional formal parameters in the whole hierarchy, probably even making this the default behavior.

bwilkerson commented 1 year ago

I think it would be reasonable to use the enablement of avoid_renaming_method_parameters as the option that controls whether matching parameters in overridden / overriding method are also renamed. I wouldn't make it the default behavior, though.

hamsbrar commented 1 year ago

@bwilkerson the lint avoid_renaming_methd_parameter is recommended and enabled by default so it'll be default anyway. But I understand, in case the lint is found to be disabled, it's highly likely that user is using different names, and they definitely won't like to see those change.

However, I think this could still be made the default behavior with a slight change. While renaming ith paramater(say a) in whole hierarchy, ignore the method if its ith parameter is not a(i.e user is using a different name). I'm pretty sure, every user, regardless of whether avoid_renaming_method_parameters is enabled, will like it.

If there are other reasons why you think it won't be good to make this the default behavior, then I'm interested to know what are those.

Edit: Oh there's one issue though, it'll be difficult for a user who wants to start using a different name for ith parameter in a single method.