Skip to content

Rename refactoring doesn't take super parameters into account #52305

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

Closed
bwilkerson opened this issue May 8, 2023 · 5 comments
Closed

Rename refactoring doesn't take super parameters into account #52305

bwilkerson opened this issue May 8, 2023 · 5 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-refactoring Issues with analysis server refactorings P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@bwilkerson
Copy link
Member

With the lint `` enabled, it would be nice if super parameters that have the same name as the corresponding parameter in the super-constructor were renamed when the super-constructor's parameter is renamed (whether directly or by renaming a field in the case of a field formal parameter).

For example, given

class Base {
  Base(this.value);

  final int value;
}

class Sub extends Base {
  Sub(super.value);
}

It would be nice for super.value to be renamed if the field value is renamed.

Depending on the outcome of #59104, we might also want to rename the super parameter even when the name is the same modulo a leading underscore.

@bwilkerson bwilkerson added legacy-area-analyzer Use area-devexp instead. devexp-refactoring Issues with analysis server refactorings labels May 8, 2023
@pq pq added the P3 A lower priority bug or feature request label May 8, 2023
@jakemac53
Copy link
Contributor

I just ran into this as well

@clik86
Copy link

clik86 commented Jan 2, 2024

Renaming the class field change both Base and Sub class parameters

@srawlins srawlins added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Mar 18, 2024
@FMorschel
Copy link
Contributor

FMorschel commented Nov 15, 2024

As stated above, I do think this is done today (just tested and it renamed everything so I might have missed something in your request). Can you clarify if there is anything in your request that is not handled yet @bwilkerson?

@FMorschel
Copy link
Contributor

FMorschel commented Nov 15, 2024

I found problems when making the value private.

class Base {
  Base({required this.value});
  Base(this.value);

  final int value;
}

class Sub extends Base {
  Sub(super.value);
}

Should become something like this:

class Base {
  Base({required int value}) : _value = value;
  Base(this._value);

  final int _value;
}

class Sub extends Base {
  Sub(int value) : super(value);
}

But for some weird reason, I tested with:

class A {
  final int foo;

  A({required this.foo});
  A.other(this.foo);
}

class B extends A {
  B({required super.foo});
  B.other(super.foo) : super.other();
}

void foo() {
  A(foo: 1);
  B(foo: 2);
  var b = B.other(3);
  print(b._foo);
}

And renaming final int foo; to _foo, then this is the output:

class A {
  final int _foo;

  A({required int foo}) : _foo = foo;
  A.other(this._foo);
}

class B extends A {
  B({required super.foo});
  B.other(super.foo) : super.other();
}

void foo() {
  A(foo: 1);
  B(foo: 2);
  var b = B.other(3);
  print(b._foo);
}

Although I can't seem to understand where is foo declared in B.other(super.foo) : super.other(); it does not throw any warnings and does run with no issues.

But when renaming at A.other(this.foo); it completely ignores the previous logic in some points and this is the output:

class A {
  final int _foo;

  A({required this._foo});  // <-- private_optional_parameter
  A.other(this._foo);
}

class B extends A {
  B({required super.foo}); // <-- super_formal_parameter_without_associated_named
  B.other(super.foo) : super.other();
}

void foo() {
  A(foo: 1);  // <-- undefined_named_parameter
  B(foo: 2);
  var b = B.other(3);
  print(b._foo);
}

But when I rename at A({required this.foo}); I get:

class A {
  final int _foo;

  A({required this._foo});  // < -- private_optional_parameter
  A.other(this._foo);
}

class B extends A {
  B({required super._foo});
  B.other(super.foo) : super.other();
}

void foo() {
  A(_foo: 1);   // <-- different from the above where this didn't get renamed
  B(foo: 2);    // <-- undefined_named_parameter (issue https://github.com/dart-lang/sdk/issues/54645)
  var b = B.other(3);
  print(b._foo);
}

So it seems some logic is missing here for everywhere to work the same way.

One last thought here. When renaming at A({required this.foo}); to a private _foo I do get a "Rename anyway" popup.

image

Why doesn't it simply follow the working behaviour as above since private_optional_parameter is an error (https://dart.dev/tools/diagnostic-messages#private_optional_parameter)?

@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
@FMorschel
Copy link
Contributor

Here is the CL. Added you, @bwilkerson, as a reviewer https://dart-review.googlesource.com/c/sdk/+/413340.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-refactoring Issues with analysis server refactorings P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants