[lint] [omit_obvious_local_variable_types] false positive

[stephane-archer]

(DropDoneDetails dropDoneDetails) async {
          LutFoldersNotifier lutFoldersNotifier =
              await ref.read(lutFoldersProvider.notifier);
          var paths = dropDoneDetails.files.map((e) {
            return e.path;
          });
          for (String path in paths) {
            if (FileSystemEntity.isDirectorySync(path)) {
              await lutFoldersNotifier.addLutDir(path);
            }
          }
        }

Omit the type annotation on a local variable when the type is obvious. Try removing the type annotation.

for me, there is nothing obvious here that makes path a String except the name.
So I think this is a false positive for omit_obvious_local_variable_types

[bwilkerson]

@eernstg

[eernstg]

The reason why the type of path is classified as obvious by the lint is that it is an iteration variable in a for-each statement whose iterable is also classified as having an obvious type.

The iterable is a local variable, paths, which has not been promoted. A non-promoted local variable is classified as having an obvious type. This happens just because it's a local variable which isn't promoted, and there's no attempt to ensure that said local variable has itself been declared in a way that actually makes its type obvious.

(If you want that then specify_nonobvious_local_variable_types will do the job.)

The underlying rationale is that the developer who wrote that local variable declaration (of paths, in this case) would have made sure that the type of the local variable is as evident as it should be, and the variable itself is henceforth considered to be obviously typed (unless it is promoted, which is always considered to be non-obvious).

In short, when it comes to local variables that depend on each other, the rule of thumb is as follows: "If some of these variables need a type annotation then go to the root cause rather than fixing it downstream". This would make the code more evidently typed overall with a small number of type annotations (most dependency trees have more nodes near the leaves than near the root).

We could adopt other approaches. E.g., the type of a local variable could be considered to be non-obvious if its declaration has an initializing expression whose type isn't obvious (which could then rely on recursive queries in cases like var v4 = [v1, v2, v3];). However, I do think it's a better general strategy to maintain a style where no local variable has an obscure type (which is a matter of human judgment), and every local variable that hasn't been promoted is considered to have an obvious type.

[stephane-archer]

every local variable that hasn't been promoted is considered to have an obvious type.

We can see with paths that it is not always the case.

If paths type was obvious I would agree with your lint but because here var is used it is not obvious. Your assumption about local variable obviousness is not always right.

I think you should revisit this assumption and see if var is used on local variables before deciding it's obvious

[eernstg]

@stephane-archer, it sounds like you'd prefer the version in late rather than the version in early:

void early() {
  Iterable<String> paths = dropDoneDetails.files.map((e) { // <---
    return e.path;
  });
  for (var path in paths) { // <---
    if (FileSystemEntity.isDirectorySync(path)) {
      await lutFoldersNotifier.addLutDir(path);
    }
  }
}

void late() {
  var paths = dropDoneDetails.files.map((e) { // <---
    return e.path;
  });
  for (String path in paths) { // <---
    if (FileSystemEntity.isDirectorySync(path)) {
      await lutFoldersNotifier.addLutDir(path);
    }
  }
}

The lint will flag the late version, but not the early version.

I wasn't suggesting that paths has an obvious type in the original example (and the late one), I was recommending that the code is written in a slightly different style (namely the early one) such that it is reasonable to consider every local variable as having an obvious type. This would imply that paths should have a type annotation, which would make the type of paths obvious, which would then eliminate the need to have a type annotation on path.

It would of course also be possible to consider local variables to have a non-obvious type, but my experience with this approach (which was the approach I used at first, and for a while) is that it gets quite noisy (that is, there will be type annotations all over the place).

We could also say that a local variable has an obvious type if its declaration has an initializing expression whose type is obvious, or if it has a type annotation (and in both cases: if it isn't promoted).

Thinking about it. ;-)

[stephane-archer]

@eernstg
if my code was:

void early() {
  Iterable<String> paths = dropDoneDetails.files.map((e) {
    return e.path;
  });
  for (String path in paths) { // <- omit_obvious_local_variable_types warning
    if (FileSystemEntity.isDirectorySync(path)) {
      await lutFoldersNotifier.addLutDir(path);
    }
  }
}

I would agree with the lint because paths type is obvious.
but in my original example, it is not obvious so I consider this warning a false positive.

From my understanding, you explain that this warning forces you to write your code like the early example and make your local variable have a clear type. However, I prefer the late version because I don't care here if paths is a Set, a List, or any type. What I care about is whether I can iterate on it and have a String. This is clearly expressed in the late version.

every local variable that hasn't been promoted is considered to have an obvious type.

in the original code, we can see this assumption is not true.

var str = "hello word" // ok

var paths = dropDoneDetails.files.map((e) { // not ok, we need the presence of type annotation to assume that
    return e.path;
  });

I hope you can consider my point and I'm happy to discuss about it.

[lrhn]

This lint tells you to not type variables whose types are obvious.
It does not tell you to type variables whose type is not obvious, so it doesn't know whether an untyped variable's type is obvious or not. The fact that you didn't type paths is taken as evidence that you considered it obvious.

I think that's a reasonable base assumption. If you want to catch cases where a variable isn't obvious, you need the other lint.

[stephane-archer]

I used var on paths not because his type was obvious but because I didn't care about the type as long as I had a string in the for loop.

Here the lint mentions that string is an obvious type and it's not if you look at the original function.

I see this has a false positive for the lint because it asks you to remove a type that is not obvious to see without the type annotation

[eernstg]

@stephane-archer wrote:

I didn't care about the type as long as I had a string in the for loop.

I can certainly follow your line of reasoning: The type annotation on paths would be Iterable<String> (or a subtype thereof), which is longer and more complex than the type annotation String on path. You don't actually care whether paths is known to be an iterable, a list, or a set, as long as it allows for the iteration variable to be a String.

However, this line of reasoning doesn't quite match the notion of "having an obvious type" that governs the behavior of the lints omit_obvious_local_variable_types, specify_nonobvious_local_variable_types, omit_obvious_property_types, and specify_nonobvious_property_types. They rely on characterizing the initializing expression of a local variable declaration or a property declaration (that is, a non-local variable declaration) as having an obvious type. This means that you can basically look at the syntax of said initializing expression and read its type off of it.

This works fine for "A string literal", a #SymbolLiteral, true, and a list literal like <int>[] or [1, 2, 3].

However, we don't get to use declarations without a type annotation often enough just based on these fully unambiguous cases. In practice, there are many other expressions that deserve to be treated as "obviously typed". This is a policy rather than an objective fact, so we can't expect to settle a debate about where to draw the line based on hard facts alone, this will to some extent be an opinionated choice.

So we include constructor invocations like C.name(), which is deemed to "obviously" have the type C. This could actually be a static method invocation (or a getter invocation followed by a function call), but in that case it will be classified as non-obvious by the lints. Hence, the very fact that these lints classify C.name() as an obviously typed expression serves as a signal that it is indeed a constructor invocation and not a static member invocation. So var x = C().name(); tells us that x has type C (in particular, if you have enabled both omit_obvious_... and specify_nonobvious_...). Similarly, C.name() is not classified as obviously typed if it means C<Some, Type, Arguments>.name().

This means that "having an obvious type" relies on very foundational properties, like "this is a constructor" vs. "this is a static method", and a reader of the code doesn't need to look up the declaration of that constructor in order to know more about the type of the result. For example, type arguments must be provided explicitly in order to make the type obvious (and the reader doesn't need to look up the constructor declaration in order to reason about how those actual type arguments could be inferred from the types of the actual arguments).

The treatment of local variables is similar: It should be enough to know that a particular name denotes a local variable, and then it's classified in a certain way by the .._obvious_../.._nonobvious_.. lints.

During my early experiments with the lints I hadn't done anything special about local variables. This gives rise to a very heavily type annotated style (because specify_nonobvious_local_variable_types will ask for type annotations). The policy whereby every local variable is assumed to have an obvious type was introduced in response to this experience.

So we have a conflict: In isolation, your argument makes sense (that is, String path should be allowed because it's well-placed and concise, and paths can have any type whatsoever as long as it doesn't make String path an error). However, the bigger picture is that "having an obvious type" is otherwise based on the relevant expression itself, plus some very foundational facts about the entities denoted by that expression. This is exactly what we're trying to maintain by recommending and assuming that every local variable has a (sufficiently) obvious type.

[stephane-archer]

@eernstg I think we both agree that this lint assumes a local variable has an obvious type and my example shows that is not always the case.
This is certainly the case when using specify_nonobvious_local_variable_types but I do not use this lint (most people will just use what Flutter makes default).

If we consider that local variable types have obvious types when they are annotated, then the lint will work as usual for people who use specify_nonobvious_local_variable_types and not complain in the example. But I see this needs a lot of special cases to make variables without type annotation obvious (constructor and string initialization, etc.)

I think the main issue I have with this lint only concerns loops.
if we assume for loop variable: "local variable types have obvious types when they are annotated" then the lint seems to have the expected behavior each time. Do you see a case where this might not be elegant? What is your thoughts on introducing a special case for loops.

[eernstg]

I think we both agree that this lint assumes a local variable has an obvious type and my example shows that is not always the case.

Yes, I do agree.

I also understand that the case you mentioned is a strong counter example because the type of path is simple (String), and the type of paths is more complex and more verbose (something like Iterable<String> or List<String>). This makes the for-each statement a particularly likely generator of examples where developers might want to do exactly what you have done.

However, I prefer the remedy which is to use a style where every local variable is sufficiently clearly typed to justify the rule that the type of a local variable is always "obvious", rather than the remedy which is to classify the type of some local variables as non-obvious, and then propagating that status to any number of other local variables (e.g., from paths to path).

You mention that you don't care about the type of paths, and this conflicts with the style that I'm recommending. So I'm actually suggesting that, all things considered, you might be more happy about this style in spite of the fact that it requires you to care about the type of paths.

The point is that the following is a simple policy which will help keeping the total number of type annotations low while preserving a clear typing of all expressions: "Local variables have an obvious type, and if you don't think it is sufficiently obvious then please add a type annotation to the earliest possible point in the dependency graph".

Otherwise (if this policy is not adopted), I'm worried about the amount of type annotations which will be held up as appropriate if you enable both omit_obvious_local_variable_types and specify_nonobvious_local_variable_types. Enabling both of these lints is very likely to be a recommended usage, for the following reason:

Some local variables have a type which could never be determined from the type of an initializing expression. For example, Pattern p = 'Hello'; has this property, possibly because the developer wanted to reserve more freedom to assign other Patterns to p, not just Strings. For these declarations, the lints will never ask for the type annotation to be removed.

For all other local variables with an initializing expression, there will be a unique recommendation if both omit_obvious_local_variable_types and specify_nonobvious_local_variable_types are enabled: It should have a type annotation if the type is non-obvious, and it should not have a type annotation if the type is obvious.

This is an automatic judgment, and the point is that (1) we don't have to spend a lot of time discussing whether or not a given local variable should have a type annotation, because the lints will say so, and (2) we can use dart fix and similar mechanisms to standardize the set of type annotations in any given library. Just like formatting, we can just stop talking about it, because there are many other things which are a lot more urgent and useful to talk about. ;-)

This means that if we change omit_obvious_local_variable_types to treat local variables differently then we'll move around the other lints as well (specify_nonobvious_local_variable_types, omit_obvious_property_types, and specify_nonobvious_property_types), and that could be a pretty substantial change to a large amount of code.

In the end, the overall "goal" of having these lints is that the code should be as readable as possible, with as few local variable type annotations as possible, and I'm afraid the code will have a lot more type annotations if we start considering some local variables to have a non-obvious type.

[stephane-archer]

I also understand that the case you mentioned is a strong counter example because the type of path is simple (String), and the type of paths is more complex and more verbose (something like Iterable or List). This makes the for-each statement a particularly likely generator of examples where developers might want to do exactly what you have done.

You explained my point perfectly.

I'm still in favor of omitting the lint warning only in "for loop" if the type is not annotated.

var a;
for (int i in a) { // no warning
}

List<int> a;
for (int i in a) { // warning
}

But I have no more evidence to show and you have well understood my point, I trust in your judgment. Should we close this issue? 🙂