Inconsistent behavior from avoid_renaming_method_parameters

[bwilkerson]

The lint claims that it's there to protect against issues when dartdoc comments are copied from the overridden method. I presume that's the reason that the lint doesn't report when the overriding method has a doc comment. But by the same token, it seems like it shouldn't report when the overridden method doesn't have a doc comment, but it does. It's not critical, but it does feel inconsistent.

[srawlins]

I think to be precise: it need not apply to a method declaration when none of the overridden methods, up the hierarchy, has a doc comment.

[srawlins]

This is more complicated with augmentations :D which @pq has already put some work into. But I think there are more questions. I've got two examples, first, what I am referring to above:

class A {
  /// [p] is important.
  void m(int p) {}
}

class B extends A {
  @override
  void m(int q) {} // Bad; the docs (e.g. on hover) refer to `p`.
}

class C extends B {
  @override
  void m(int q) {} // Also bad, independently from how `B.m` is bad.
}

And here's an augmentation example: What if you override a method, and the overridden method has an augmentation?

class A {
  void m(int p) {}
}

augment class A {
  /// [p] is important.
  void m(int p) {}
}

class B extends A {
  @override
  void m(int q) {}
}

I think that the algorithm needs to recognize: B.m renamed the p parameter to q. While this may not look like a problem, because A.m (the unaugmented version) has no doc comment, the augmented version does have a doc comment.

[bwilkerson]

The point about augmentations is well taken, and I'm now second-guessing myself. Maybe it's a good thing that the lint reports whether there's a doc comment on the overridden method and the real problem is that the documentation makes it sound like we only care about this from the perspective of dartdoc.

What if you override a method, and the overridden method has an augmentation?

I agree, I think we need to flag in this case. And it seems like a strong argument that the lint should just prevent renaming parameters no matter what: it seems like it would be a poor UX if a macro, or worse yet a code generator, added an augmentation with a comment that then caused non-generated code to have diagnostics.

[srawlins]

It all feels pretty hypothetical, as I don't know any good use cases for renaming a parameter. (Excluding _ which I think is a separate issue.)

Meaning, in your case about a macro or code generator, what is the story where a user would be really interrupted or surprised by augmented doc comments? Macros should be pretty stable, such that lint warnings aren't frequently flipping between reported/not-reported. Maybe less so for a generator writing an augmentation. But what is the story where the developer really wants or really needs to rename a parameter? I don't know that we've seen those examples.

All that being said, I'd be fine with closing this as WAI.

[bwilkerson]

It all feels pretty hypothetical ...

That's true, it is. We really don't know how users will use augmentations, so any attempt to consider the possible impact has to depend on hypothetical cases.

... what is the story where a user would be really interrupted or surprised by augmented doc comments?

I should have been more complete. I was thinking of the case you added above:

class A {
  void m(int p) {}
}

augment class A {
  /// [p] is important.
  void m(int p) {}
}

class B extends A {
  @override
  void m(int q) {}
}

If the augmentation is added by a code generator then before the generator is run there wouldn't be any diagnostics, but afterward there would be.

... I don't know any good use cases for renaming a parameter.

I don't either. Hence the second-guessing. I was just surprised because of what the docs implied (that there wouldn't be a diagnostic in a case where there was one).