proposal: positional_super_parameter_names

[jakemac53]

positional_super_parameter_names

Description

Positional super parameter names should always match exactly the name of the corresponding parameter on the super constructor that they are matched to.

Details

When using positional super parameters in constructors, the name is ignored and they are matched based on position. This can be confusing, and to avoid confusion you should always just name them exactly the same name as the corresponding parameter from the super constructor.

Kind

Guard against errors

Bad Examples

class Rectangle {
  final int width;
  final int height;
  
  Rectangle(this.width, this.height);
}

class ColoredRectangle extends Rectangle {
  final Color color;
  
  ColoredRectangle(
    this.color,
    super.height, // Bad, actually corresponds to the `width` parameter
    super.width, // Bad, actually corresponds to the `height` parameter
  ); 
}

Good Examples

class Rectangle {
  final int width;
  final int height;
  
  Rectangle(this.width, this.height);
}

class ColoredRectangle extends Rectangle {
  final Color color;
  
  ColoredRectangle(
    this.color,
    super.width,
    super.height, 
  ); 
}

Discussion

Since these are only positional parameters, this lint should probably always be followed and exceptions should not exist or be exceedingly rare. It is not breaking to change the name (except that it might trigger this lint on another package).

I would strongly suggest including this in the core or recommended lint sets (ideally core).

Notably I actually did run into this when migrating the macro apis to use super parameters - I got lucky that the types were different on the parameters so it triggered a different diagnostic at usage sites, but it was highly confusing.

This does not currently correspond to an effective dart style rule, but we should likely create one, cc @natebosch @munificent .

Discussion checklist

• List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• List any relevant issues (reported here, the SDK Tracker, or elsewhere).
• If there's any prior art (e.g., in other linters), please add references here.
• If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
• If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[srawlins]

Wow, big +1

[natebosch]

The small downside is that it becomes "breaking" to rename a positional parameter in some cases where it was not breaking before. I don't think that is likely to cause much pain. I do think we should include this in the recommended set.

[jakemac53]

The small downside is that it becomes "breaking" to rename a positional parameter in some cases where it was not breaking before.

This is no more breaking than deprecating an API. Packages should not take into consideration that this is "breaking" when doing their versioning (although they may have to clean up some things in certain repos like flutter or flutter customer tests).

As such I think it should be in the core set, it is always a footgun to violate this lint imo. If you really want to change the name, do it explicitly so you can see what is happening in the super constructor invocation more clearly.

[munificent]

Yes, I absolutely want this lint. I got burned by this, like, the very first time I tried super. parameters.

[pq]

/cc @lrhn @eernstg @goderbauer for any feedback.

(FWIW, big +1 from me.)

[goderbauer]

Love it! This sounds like a great lint to have. Curious to see what it will uncover in the flutter framework.

[goderbauer]

As such I think it should be in the core set.

Can you file a request for that at https://github.com/dart-lang/lints/issues so we don't forget about this next time we overhaul the list of lints in core/recommended?

[lrhn]

It should work well as a lint. Sometimes you do want to rename, and then you can ignore it.

[pq]

Given the unanimous feedback, I'd say we're good to go. Marking "accepted".

[eernstg]

I think this is fine, it is difficult to come up with a situation where there is a genuine need to violate this lint, and there are a lot of cases where it will flag a situation which is a plain (and highly misleading) typo.

Let's consider a couple of false positives anyway:

A Vehicle might have an operator constructor parameter, a subclass like Car could have a driver, Bike could have a rider, Aeroplane could have a pilot, etc. In other words, an object "playing the same role" in the classes of a class hierarchy could have different names associated with them as the most natural choice, and they might still be handled using super (and storing them in some private instance variable, or whatever).

However, I think this just serves as an illustration why this situation would be quite rare.

PS: Do we need a similar lint for regular method overloading?

class House {
  void rent(int numberOfWeeks, int insurancePolicyNumber) {...}
}

class SummerHouse implements House {
  @override
  void rent(int insurancePolicyNumber, int numberOfWeeks) {...}
}

void main() {
  House h = SummerHouse();
  h.rent(1, 8787181179); // Just one week, right?
}

Of course, they could just use an inline class to express the fact that those two int values have a completely different purpose. However, in practice we will have some situations where the types won't reveal the mistake, and an accidental reordering of a couple of parameters in an override could create some very weird bugs. ;-)

[srawlins]

PS: Do we need a similar lint for regular method overloading?

avoid_renaming_method_parameters should cover this, but it currently only supports extension-subtyping, and not implements-subtyping.

[goderbauer]

I was playing around with this new lint and I ran into one strange case of which we have a couple of instances in the flutter code base. Wondering what people here think how this case should be handled:

class Base {
  Base(this._value);

  final int _value;
}

class Sub extends Base {
  Sub(super.value); // 🔥 info: The super parameter named 'value'' does not share the same name as the corresponding parameter in the super constructor, '_value'.
}

The lint wants me to change it to:

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

The leading underscore as an indicator for something being private looks very strange to me there and may confuse people? Do people think this is the right change here? Or should the lint maybe allow for super.value, if the parameter in the super class is named _value?

[jakemac53]

I could see either way, but I don't think that generally super._value is any different/worse than this._value. A bit weird, but already an issue, and having them be exactly aligned could potentially help fix bugs even in this case (although only in very weird situations to be sure).

[goderbauer]

A bit weird, but already an issue

Should we keep that weirdness locally, though, and avoid spreading it to other classes with this lint?

[jakemac53]

Should we keep that weirdness locally, though, and avoid spreading it to other classes with this lint?

It is really trading one weirdness (private parameter name) for another (mis-matched super parameter name). I am not sure if one is definitively less weird than the other?

[eernstg]

We have this issue in the language repo, discussing how to handle initializing formals with private instance variables, e.g., this._x along with final int _x;.

I expect this proposal to have broad support from the language team, even though the language team hasn't yet reached a final decision.

The main point is that it is possible to use an initializing formal like this._x (positional or named, with or without a type) with the same semantics as today, but in the case where it is a named parameter, call sites must use the corresponding public name (for _x the corresponding public name is x).

If you rely on that approach then the treatment of the example would be adjusted as follows:

class Base {
  Base(this._x1, {this._x2 = 0}) : y = _x1;
  final int _x1, _x2, y;
}

class Sub extends Base {
  Sub(super.x1, {super.x2}); // No lint/error, parameter names are `x1` and `x2`.
}

void main() {
  Base base = Sub(2, x2: 3); // Named parameter name is `x2`.
  print('${base._x1} ${base._x2}'); // Instance variable names are preserved.
}

[FMorschel]

How much value do you think this proposal would gain by also triggering at:

class A {
  final num bar;

  A({this.bar = 0});
}

class C extends A {
  C(int foo) : super(bar: foo); // Here
}

Here foo could have been an old name for bar and after a refactor rename it got lost. I'd say this feels a bit odd and is probably a case for this lint. Not triggering for foo if it is named is fine, I think.

[jakemac53]

How much value do you think this proposal would gain by also triggering at:

class A {
final num bar;

A({this.bar = 0});
}

class C extends A {
C(int foo) : super(bar: foo); // Here
}
Here foo could have been an old name for bar and after a refactor rename it got lost. I'd say this feels a bit odd and is probably a case for this lint. Not triggering for foo if it is named is fine, I think.

I think that should be a different lint as it is a bit more dubious and likely to have false positives.

I want this lint to be very focused so that it can be included in the core lint set without controversy.