Proposal: Generalize avoid_types_as_parameter_names to include type parameters

[eernstg]

Thanks to @martin-east for bringing up this topic.

Description

The lint avoid_types_as_parameter_names will report situations where a formal parameter declaration in a function or type alias declaration shadows a type declaration with the same name in an enclosing scope.

This is a proposal to generalize that lint such that it also reports the same situation when the formal parameter is declared by a function type, and also when the formal parameter is a type parameter.

To Reproduce

// We are only interested in `avoid_types_as_parameter_names`:
// ignore_for_file: prefer_generic_function_type_aliases

void f(int) {} // Lints `int`.
void g<X>(X) {} // Lints the second `X`.
typedef void F(int); // Lints `int`.

void Function(String int) v = print; // No lint. The proposal is to lint `int`.
void h<int>(int int) {} // No lint. The proposal is to lint 1st and 3rd `int`.

Expected behavior

avoid_types_as_parameter_names lints parameter names in function types and names of type parameters as well, when they shadow a type in an enclosing scope.

Additional context

Positional parameter names in a function type are never the result of a lookup, so we might ignore them. Nevertheless, it is probably a mistake, and a source of confusion, if such parameters are specified to have a name which is also the name of a type in the enclosing scope, so it is probably a better idea to treat them just like all other parameters.

[bwilkerson]

Unfortunately, this is a breaking change. We'll need to assess the scope before we can decide whether the cost is worth the benefit (though I suspect that the cost will be low).

[pq]

Brian is right. As with all "false negatives", we have to be careful about impact. (This is a core lint too so all the more reason.) That said, this does seem like a good change to explore to me. To start, it'd be interesting to fix it as proposed and see if we detect any violations in the SDK, Flutter or Google3.

[srawlins]

I'd just fork the rule with a new name, types_as_parameter_names, and deprecate+delete the old one.

[pq]

I'd just fork the rule with a new name. types_as_parameter_names and deprecate the old one.

Sam makes a good point. This is an option. I'm not sure it really saves us any work in the long run though (since deprecation and removal adds a bit of it's own work). Worth considering though!

[martin-east]

Would leaving avoid_types_as_parameter_names untouched and creating new rules for the two situations not covered by it be easier? It would also give the user finer control.

[srawlins]

Leaving it untouched would be easier, but worse for the user, by adding confusion about why there are two rules, and it would be worse for the team, who would have to maintain both. I don't think there is a reason we'd want to support the looser existing rule and a stricter new rule, and I don't know of a reason that a user would want the looser version.

[pq]

I agree with Sam. Finer control is really appealing (but comes at a cost).

@eernstg: I'm curious if you've done any experimenting to see how much impact this would actually have? I'm guessing it doesn't happen that much in practice but it'd be interesting to get a better idea.

[eernstg]

No, I haven't implemented anything. I suspect that it wouldn't be hard to create a CL that changes avoid_types_as-parameter_names to flag those new locations in addition to the current targets, and the flagged locations would then be all-new because this lint is core already. With that knowledge, we could introduce a new lint if needed.

[lrhn]

Have you considered using language versioning with lints, so a change to a lint is only enabled for code when it changes to the new language version.

(Adding warnings is not breaking. You can still compile and run a program with warnings, unless you turn warnings into errors, but that is a choice you make to make your build process stop on non-breaking changes. The change wasn't breaking.)

We'll still have to fix all internal code, though.

[srawlins]

By "breaking", Brian means that the change may be excruciating to land. Since flutter CI turns red when a new static warning is reported, you have to clean up new violations in flutter before landing. Since flutter customer tests turn red when a new static warning is reported, you have to clean up new violations in flutter customer tests before landing. Same with the flutter engine, flutter plugins, Dart SDK, and all of google3.

[lrhn]

This is a case of people painting themselves into a corner. Warnings are not errors, otherwise they would have been errors.
If you treat warnings like errors, you're choosing to break your own build over something that is no more a problem than it was yesterday. It's a very populare choice, maybe because it really is the only way to ensure that warnings ever get addressed, but it's also one that makes it, as you say, excruciating if more warnings are added.

The thing is, warnings are not added to annoy people, but to help them. If a lint is enabled, it's because someone considers it important to satisfy the lint in their own code. If a tool gives a new warning, it's because it thinks it's important to look at that code.
If we find missing cases in a lint implementation, code that clearly violate the intent of the lint and wasn't recognized as such before, then we should tell peopel. The existing code is still bad even if you don't get the new warning, you just don't know it. And since you enabled the lint, you probably want to know.
We should be happy to provide more warnings for existing code. If we are afraid to cause more warnings, we're not giving people the help they asked for.

(Is the issue tooling? Could we have a marker on each lint warning that says when it was introduced, and then a build-system can choose to not turn warnings into errors if they're newer than 30 days, or whatever limit they want?
Or, as suggested, can we make it langauge versioned, so you won't get new warnings until you upgrade your language version? Can we just do something special internally, for Flutter and google3, and give external users all the warnings they deserve? Anthing that makes us not afraid to add perfectly reasonable warnings!)

[eernstg]

I don't think there's anything surprising or wrong in introducing avoid_types_as_type_parameter_names. When it comes to parameter names in function types, I'd expect that it occurs so rarely that it could be a change which is not breaking in practice (the problem only arises when there is a formal parameter name in the function type, and function types will treat a lone name as a type, as in void Function(int), as opposed to the old-style type alias where typedef void F(int); considers int to be a parameter name, not a type). So perhaps that's a way to introduce these checks essentially without breaking anything.

[eernstg]

Here's a small experiment: https://dart-review.googlesource.com/c/sdk/+/381841. It extends avoid_types_as_parameter_names to report the case where a type parameter has the same name as a type in scope. It causes failures in 15 cases (flutter-analyze-try), 9 cases (pkg-release-linux-try), and 2 cases (g3-build-try). As far as I can see they are all legitimate (we do have a type parameter whose name is the same as a type in scope).

One type of situation occurs several times:

class A<X> {
  static X myStaticMethod<X>(X x) => x; // LINT
}

This is a violation of the generalized avoid_types_as_parameter_names because X is introduced as a type parameter (of myStaticMethod) at a location where X is already in scope, denoting a type (because it is also a type parameter of the enclosing class).

This might be OK because a static member has no access to the type parameters of the enclosing class, and hence it could make sense to copy the type parameter list of the class into a static method declaration "because it needs the same type parameters as the enclosing class". If it is considered to be OK then we'd need a special exception for this kind of situation.

However, I'd prefer to report the situation even in this case. For instance, it could also be a source of confusion for a reader who thinks that an occurrence of X in the body of myStaticMethod is the type parameter of the class. Also, the claim that myStaticMethod needs the same type parameters as the enclosing class might not be a very good argument if you think about it.

In summary: The breakage isn't daunting. 😁

[eernstg]

Here's a reason why we should probably recommend that developers do not shadow a class type parameter with a static member type parameter:

class A<X> {
  final X x;
  A(this.x);
  List<X> get asList => [this.x];
  static List<X> asListStatic<X>(A<X> this_) => [this_.x];
}

void main() {
  A<num> a = A<int>(1);
  print(a.asList.runtimeType); // 'List<int>'.
  print(A.asListStatic(a).runtimeType); // 'List<num>'.
}

In other words, it is not safe to assume that it's "the same X everywhere".

Updated summary: It's all good breakage! 😁

[srawlins]

g3-build-try is only a smoke test. To find all violations, you'll need to follow the instructions at http://go/dart-lint-cleanup#get-all-violations-of-a-lint-or-hint

[srawlins]

Note about a similar existing rule: avoid_shadowing_type_parameters explicitly allows "shadowing" a type parameter in a static element. See test: https://github.com/dart-lang/sdk/blob/main/pkg/linter/test/rules/avoid_shadowing_type_parameters_test.dart#L187

[eernstg]

OK, 330 violations, 211 of them static.

[eernstg]

At this time, the lint implementation in https://dart-review.googlesource.com/c/sdk/+/381841 no longer reports the situation where a type variable of a static method shadows a type variable of the enclosing declaration (which is a class, mixin, mixin class, extension type, or extension). 106 violations.