omit_local_variable_types has spurious diagnostics when the variable type impacts inference

[natebosch]

void main() async {
  final String s1 = await someGeneric(['']); // omit_local_variable_types
  print(typeOf(s1)); // String
  // Removing the annotation results in a static type change:
  final s2 = await someGeneric(['']);
  print(typeOf(s2)); // dynamic
}

Future<T> someGeneric<T, S extends List<T>>(S v) async => v.first;

Type typeOf<T>(T arg) => T;

The lint fires and asks for the local variable type to be removed, but doing so changes the static type of the variable. We don't correctly check when the variable impacts inference, because we only check whether the return type is a type variable.

sdk/pkg/linter/lib/src/rules/omit_local_variable_types.dart

Line 132 in 6885e82

if (element.returnType is TypeParameterType) {

Once the type is more complicated, like Future<T>, the lint misfires.

cc @pq @yanok

[srawlins]

Sigh, yeah I think we have to file this in the bucket of "in order to implement this perfectly correctly, the lint rule needs to run during inference" (or at least have knowledge of context types). ☹️

[yanok]

To be perfectly correct, right, lint needs to be able to re-run inference for the expression without a context type. But to just exclude false positives, we could simple modify the check from "is a type variable" to "contains a type variable". This is a conservative check, so there will be false negatives. But at least it will be more consistent than it is now, since now we can get both false negatives and false positives.

[bwilkerson]

... the lint rule needs to run during inference ...

For what it's worth, I'm not convinced that even that would be enough. I think it's the case that we would need to be able to ask "what if" questions. Things like "what would the (inferred) type of this variable be if we were to remove the type annotation. Yes, that requires running inference over the modified code, but I don't think we'd be able to answer that question while running inference over the unmodified code.

[srawlins]

But to just exclude false positives, we could simple modify the check from "is a type variable" to "contains a type variable".

I think this is very pragmatic! 👍

I think it's the case that we would need to be able to ask "what if" questions.

Oh I think you're right. Yeah this might need to include flow analysis of statements and expressions that follow the variable declaration, with a "hypothetical" type. This is the same conundrum as the unnecessary_cast false positives.

[srawlins]

@pq as a P1, this ticket should be actively worked on. Is this one you are planning to take, or should I assign to someone else?

[pq]

Agreed. I'm not actively on this one so if anyone is game, they're welcome to jump on it!

[eernstg]

There is a way to simplify the "false positives are complex" issue with omit_local_variable_types: If we milden this lint such that it only asks developers to remove a declared type when the initializing expression has an 'obvious' type (as proposed in #58773) then we would probably have very few false positives in the first place.

[pq]

@stereotype441: more fodder for the flow analysis conversation