Create a variant of omit_local_variable_types that permits non-trivial declared types

[eernstg]

[Edit Aug 2024: Changed the title: New lints are introduced, omit_local_variable_types is not modified.]

This issue is a proposal that omit_local_variable_types is mildened such that it only flags local variable declarations with a declared type in the case where the variable has an initializing expression with a type which is not 'obvious' (as defined below).

This is a non-breaking change, because it makes omit_local_variable_types flag a smaller set of situations.

It should still flag cases like int i = 1; (which should be var i = 1;), List<int> xs = [1, 2, 3]; (which should be var xs = [1, 2, 3];), C<num> c = C(1.5); where C is a class (it should be var c = C<num>(1.5);), C<double> c = C(1.5) (which should be var c = C(1.5), assuming that it infers as C<double>(1.5)),etc, because the initializing expression has an 'obvious' type.

But it should allow Foo<int> foo = someFunction(a, b()); even though that might also be expressible as var foo = someFunction<Whatever, It, Takes, To, Make, It, A, Foo_int>(a, b<Perhaps, More, Stuff>());. It should actually also allow it even in the case where the same thing is expressible as var foo = someFunction(a, b());, because the type of the initializing expression isn't obvious.

The motivation for doing this is the following:

The declared type of a local variable may be considered to be a simple, self-documenting, declarative approach to determine which actual type arguments to provide at various locations in the initializing expression. If, alternatively, we pass some actual type arguments in the initializing expression, we might infer the same declared type, but (1) it is not documenting the type of the variable (which means that it may be hard to understand how to use it correctly), (2) it is not simple (because we may need to look up various declarations in order to determine the type of the initializing expression), and (3) it is not declarative (because we provide feed-forward information in the initializing expression, rather than specifying the desired end result and relying on type inference to sort out the details). For example:

// Some other libraries contain various declarations that we're importing and using.
// The following declarations play that role.

Map<X, List<X>> f<X>(A<X> a) {
  var ax = a.x;
  return ax != null ? {ax: []} : {};
}

X g<X>(Map<X, List<X>> map) => map.keys.first;

class A<X> {
  X? x;
  A(this.x);
}

// We are using the above declarations.

void main() {
  // We can omit the variable type, because it is inferred.
  var v1 = g<int>(f(A(null)));

  // We can also declare the variable type.
  int v2 = g<int>(f(A(null)));
  ...
}

The lint omit_local_variable_types will flag v2, because the declared type is seen as unnecessary. The perspective that motivates this issue is that it is indeed possible to infer exactly the same declared type, but it may still be helpful for all of us to have that declared type when we're reading the code. In particular, we'll at least need to look up the declaration of g before we can even start guesstimating the type of v1. So the proposal here is to make omit_local_variable_types stop linting these non-obvious cases, thus allowing developers to document the type where it's actually needed.

(Interestingly, omit_local_variable_types does not flag int v2 = g(f(A(null)));, which seems to contradict the documentation of the lint, so maybe the lint is already doing some of the things which are proposed here. It clearly does not flag every local variable declaration that has a declared type which is also the inferred type of the initializing expression).

Note that it is assumed that omit_local_variable_types will never flag a declaration T v = e; where the declared type T differs from the type of e which is inferred with an empty context type, or where the type inference on e with an empty context type produces a different result (different actual type arguments, anywhere in e) than type inference on e with the context type T. This proposal is not intended to change anything about that, and it should not affect any of the issues where it is reported that omit_local_variable_types has a false positive, because it lints a case where it does make a difference. This proposal is only about not shouting at people when they have a declared type that may arguably be helpful.

Obvious Expression Types

In order to have a mechanically decidable notion of what it takes for a type to be 'obvious' we need a precise definition. The point is that it should recognize a number of types that are indeed obvious, and then all other types are considered non-obvious.

This means that omit_local_variable_types will allow (not force) developers to declare the type of a local variable with an initializing expression e when e has a non-obvious type, but it will continue to flag T v = e; when the type of e is obvious.

An expression e has an obvious type in the following cases:

• e is a non-collection literal. For instance, 1, true, 'Hello, $name!'.
• e is a collection literal with actual type arguments. For instance, <int, bool>{}.
• e is a list literal or a set literal where each element has an obvious type, and all elements have the same type. For instance, [1, 2], { [true, false], <bool>[] }, but not [1, 1.5].
• e is a map literal where all key-value pair have a key with an obvious type and a value with an obvious type, and all keys have the same type, and all values have the same type. For instance, { #a: <int>[] }, but not {1: 1, 2: true}.
• e is a record literal where every component has an obvious type. *For instance, (1, enabled: true), but not (1, enabled: foo()). *
• e is a local variable that has not been promoted. *For instance, int i = 1; then var x = i;.
• e is an instance creation expression whose class part is not raw. For instance C(14) if C is a non-generic class, or C<int>(14) if C accepts one type argument, but not C(14) if C accepts one or more type arguments.
• e is a cascade whose target has an obvious type. For instance, 1..isEven..isEven has an obvious type because 1 has an obvious type.
• e is a type cast. For instance, myComplexpression as int.
• e is a conditional expression where both branches have an obvious type, and they have the same type. For instance, condition ? 'yes' : 'no'.
• e is a type test. *For instance, myVariable is String.
• e is a throw expression. For instance, throw myError.
• e is a parenthesized expression where the contained expression has an obvious type.
• e is this.
• e is a type literal. For instance, String.

Discussion

We may need to add some more cases to the definition of an obvious type, because some other types may actually be obvious to a human observer. However, it is not a big problem that some types are obvious but are not recognized as such, because this means that developers have the freedom to declare a type when they want to do that. If the set of obvious types is enlarged later on, it will be a breaking change, but if the given type is actually obvious then there will be few locations where the new, more strict, version of omit_local_variable_types will flag a declaration, because few developers have actually declared the type in that case. So the changes that are actually breaking are also the ones that may be questionable, in which case the type might not be so obvious after all.

One outcome that we could hope for if omit_local_variable_types is mildened as proposed here is that this lint could become a broader standard (for example, it could be a member of the recommended set of lints), because a larger percentage of the community would be happy about the way it works.

Versions

• 1.0, Jun 29, 2022.
• (Small adjustments over time, no detailed description.)
• 2.0, Aug 6, 2024: Adjust the rules about obviously typed expressions. Collections and maps must satisfy that all elements have an obvious type, not just some elements. A type cast (e as T) is now considered obviously typed.
• 2.1, Aug 7, 2024: Add a "converse" lint specify_nonobvious_local_variable_types, and broaden the notion of what it takes to be an expression that has an obvious type.

[bwilkerson]

I agree that the results of type inference can sometimes be difficult for a human reader to predict, and that this can significantly impact the user experience. And I agree that adding explicit type annotations is one way to solve that issue by removing the need for the reader to predict the results. My concern with this proposal is that this lint currently enforces the style guide, and I wouldn't want to change the lint without changing the style guide to match.

[eernstg]

@bwilkerson wrote:

this lint currently enforces the style guide, and I wouldn't want to change the
lint without changing the style guide to match.

That would indeed be nice! The whole point with this issue is that it can be counterproductive to ban all declared types on local variables, because some of them are actually helpful (good for code readability and maintainability). I'm just asking that developers are allowed to use good judgment in this matter.

By the way, the current version of omit_local_variable_types does not lint every declaration of the form T v = e; where var v = e; yields T as the inferred type and doesn't change the semantics of e, it only lints some of them:

// Define `f`, `g`, and `A` as before.

int v2 = g(f(A(null))); // No lint.

I don't know exactly where the threshold is, but my proposal is simply to omit the lint in a larger set of cases, such that we only push developers towards var v = e; when the type of e is obvious. The proposed change will never push them in the opposite direction, so anyone who wants to use var v = e; where the type is completely beyond comprehension is free to do so.

I'm afraid we can't say that this is a bug, because the lint is implemented in a way that corresponds pretty well to the documentation and the style guide. So I'll remove the 'bug' label for now. But I'll be very happy if we decide to change the recommended approach slightly as proposed here, and in that case it will become an implementation request.

[bwilkerson]

Sounds good.

@munificent Just FYI.

[munificent]

I actually quite strongly prefer the current guidance in Effective Dart. I wouldn't support the lint deviating from it. My arguments are:

1. Billions of lines of correct code are written and maintained in dynamically typed languages where no variables are annotated. It can't be that necessary to allow redundant type annotations on local variables.

2. Most statically typed languages have or are moving to inferring locals. It has always been idiomatic in Scala, Kotlin, Go, Swift, and many other languages I'm not thinking of to omit the types on all locals. C#, Java, and even C++ (where type annotations actually change behavior) have moved decidedly towards it.

3. This has been the well established convention for many years in Dart with very few complaints aside from a small number of people (who seem to disproportionately be on the Dart team). All Google internal Dart code has been expected to omit type annotations for all correctly inferred local variables for years. It's a requirement to earn readability. This has never showed up as a major style complaint.

4. I have worked on several teams across multiple languages as they have gone from fully type annotated locals to type inferred locals. Every time, I have seen a number of users feel extremely uncomfortable with omitted types. They would eventually grudgingly accept omitting types on "obvious" initializers. Over time their definition of "obvious" would grow and grow until eventually, they get tired of remembering which initializers are obvious and just use it on all of them. I have never seen a team go backwards away from inferred locals towards more types.

5. My experience is that type annotations on locals tend to encourage worse names for local variables.

6. Scope of local variables almost always is and certainly should be small. If you can't understand a short function without redundantly type annotating a local variable, you likely have bigger readability problems and should focus on those.

7. Users will argue about the heuristics for which kind of names are "obvious" until the end of time. (This is why the annotation guideline for non-local variables is deliberately vague and leaves it up to individual taste.)

8. Trivial refactorings will often change an initializer from "obvious" to "non-obvious". For example, turning a named constructor into a static method with the same name should according to these rules require every local variable that calls that method in its initializer to be fixed to have a type annotation. Conversely, when refactoring a static method to a named constructor, this would require users to go back and remove all of the type annotations.

9. Allowing redundant type annotations on locals makes it harder to notice non-redundant type annotations. I think it's more valuable to have upcasts stand out visually.

I think we should just leave the lint and rule as they are.

[eernstg]

@munificent, this is of course a topic where we disagree substantially, so I'll have to comment on it. ;-)

Billions of lines of correct code are written and maintained in dynamically typed languages

But I wouldn't expect you to argue that we should drop all static typing? I thought an argument against this proposal should be something which is specific to local variables.

Most statically typed languages have or are moving to inferring locals

Sure, and I would certainly expect that to be the typical approach in Dart. I even think we should nudge people in that direction in as many cases as possible when the type doesn't provide useful information, that is, when the type is obvious in the initializing expression.

If you can't understand a short function without redundantly type annotating a local variable ..

So what's the type of var y = foo(x);?

No, I'm not going to tell you which type arguments foo accepts, and even the tools aren't going to tell you how type inference proceeded to infer types for that expression. The point is that you need to look it up, and foo is of course in some other package. (OK, you can hover in an IDE, but we do encounter code in other contexts as well.)

So the type is not redundant for a reader of the code, and I don't understand why we need to shout at people who prefer to inform readers about such non-obvious types, in some carefully selected particularly complex situations.

Users will argue about the heuristics for which kind of names are "obvious" until the end of time

Any user who wishes to omit the type of a local variable can freely do so because the lint will never ask for the type to be added, it will only ask to get the type removed (when it's obvious).

So the only case where anyone would want to change the heuristic is when they want to have a type annotation in a case where the lint says that the type is obvious.

I don't think that's very likely to happen, presumably the obvious types are obvious, and will remain so.

turning a named constructor into a static method with the same name should according to these rules require every local variable that calls that method in its initializer to be fixed to have a type annotation

No, we never require a type annotation, we just stop complaining about a type annotation which isn't obvious.

But it is true that the opposite change (static method turns into constructor) would cause the lint to ask for the type annotations to be removed, if any.

harder to notice non-redundant type annotations

That's an interesting point!

It could be useful to be able to detect the cases where the specification of a declared type changes the run-time semantics of the initializing expression (because type inference, coercion, generic function instantiation, etc. have a different outcome).

However, I don't see how a mere upcast would be more valuable to document than a non-obvious type. After all, if we seriously don't care about the type then why is there an upcast in the first place? And if we do care about the type then why must it be impossible to document that type just because a potentially very complex type inference, coercion etc. step produced exactly the same type as the one that we wish to document?

[munificent]

If you can't understand a short function without redundantly type annotating a local variable ..

So what's the type of var y = foo(x);?

In:

bar(foo(x))`;

What's the type of argument passed to bar()?

No, I'm not going to tell you which type arguments foo accepts, and even the tools aren't going to tell you how type inference proceeded to infer types for that expression. The point is that you need to look it up, and foo is of course in some other package. (OK, you can hover in an IDE, but we do encounter code in other contexts as well.)

All of this applies to my example too. We seem to be perfectly happy with not seeing type annotations on subexpressions, so I don't see a very compelling argument for them on local variables.

I don't understand why we need to shout at people who prefer to inform readers about such non-obvious types, in some carefully selected particularly complex situations.

The linter shouts at people for lots of reasons that absolutely don't matter at all when it comes to understanding their code. The answer for that is almost always that consistency has its own value. If we let people redundantly annotate local variables, then readers will be presented with some code that chooses to do that and some code that chooses to not. That difference will get their attention and distract them from other aspects of the code. They may wonder, "Is this an upcast?" (Or worse, it will be an upcast and they won't realize it because they think it's just a redundant annotation.) They may wonder why the author chose to annotate this one and not others nearby. Was there a reason for that, or was it just that one author had one preference and other authors of nearby code had others?

Overall, I haven't seen any compelling evidence that allowing variation here carries its weight.

Any user who wishes to omit the type of a local variable can freely do so because the lint will never ask for the type to be added, it will only ask to get the type removed (when it's obvious).

They can opt out of annotating in code they write, but they can't opt out of being presented with code written by others who made a different choice.

I don't think that's very likely to happen, presumably the obvious types are obvious, and will remain so.

I have literally watched this happen on large teams multiple times over my career, including in Dart.

I don't see how a mere upcast would be more valuable to document than a non-obvious type.

Because it tells the reader that the variable's type is not what they would intuit from the initializer expression.

After all, if we seriously don't care about the type then why is there an upcast in the first place?

So that the variable can be assigned a value later whose type isn't a subtype of the initializer's type. So seeing the type annotation on the local sends a useful signal of "hey this is going to be mutated later to a different type".

And if we do care about the type then why must it be impossible to document that type just because a potentially very complex type inference, coercion etc. step produced exactly the same type as the one that we wish to document?

It's not impossible. This is just a lint. But the lint does reflect what we think leads to the best, most consistently readable code across the ecosystem.

[eernstg]

In:

bar(foo(x));

What's the type of argument passed to bar()?

[it is not trivial to compute such types]

All of this applies to my example too.

Certainly, but the point is that software engineering quality includes readability and comprehensibility as core elements, and one of the foundational tools used to improve on the readability of a snippet of code is to split up large expressions by evaluating subexpressions and storing their value in local variables.

So let's say that bar(foo(x)) is sufficiently complex to justify some refactorings. The first one would be this:

var name = foo(x); // Where `name` is chosen such that it helps the reader.
... bar(name) ...

One difficulty with this kind of refactoring is that the type inference process works differently when foo(x) has no context type, so we may have to add a declared type to name in order to preserve the semantics:

SomeType name = foo(x);
... bar(name) ...

However, the declared type can be helpful for a reader of the code, because it helps them understand the meaning and purpose of name, and that could make it easier to read and understand expressions like bar(name).

I'm simply saying "let's stop shouting at the developer" in the case where SomeType has been declared for readability reasons.

If we let people redundantly annotate local variables, then readers will be presented with some code that chooses to do that and some code that chooses to not.

This would be a likely outcome if half of the community enables omit_local_variable_type and the other half enables always_specify_types.

What I'm proposing is that the people who agree with me that documenting local types can be good for readability in a few cases can enable omit_local_variable_type, because it won't shout at them when they do want a type annotation.

But in their daily work they'll have lots of little nudges in the direction of omitting that type, because there are so many cases where the type is obvious, and omit_local_variable_type will (rightfully) complain in those cases.

So the few local variables with a type will stand out, and it's a rather safe bet that the developer who wrote it wanted to have the type in that particular case because they needed to think hard about that particular type when they wrote the code, and then it's probably helpful to have it there when we read the code.

So the proposal will very specifically not nudge developers in the direction of writing the types just because that's a habit, each local variable with a type is there because someone actively made that declaration different from most others.

They can opt out of annotating in code they write, but they can't opt out of being presented with code written by others who made a different choice.

Today, those others would simply not enable omit_local_variable_type, and then there's no limit on the number of local variable types in their code.

With this proposal, they might accept enabling omit_local_variable_type because it allows them to use type declarations when they are helpful, and the net result would be that the community as a whole would move in the direction of omitting the declared type most of the time, rather than being more or less divided.

But the lint does reflect what we think leads to the best, most consistently readable code across the ecosystem.

We don't quite agree. ;-)

It just rubs me the wrong way to force developers into lowering the software engineering quality of their code, just because we refuse to give them a little bit of extra freedom to use human judgment when needed.

[goderbauer]

So far, omit_local_variable_type hasn't made the cut for the recommended set of lints in package:lints because some feel it is too strict and removes type annotations that are actually useful in understanding the code (see also dart-lang/lints#24). I have to read a lot of code in environments (e.g. on github) where I don't have an IDE handy that can infer the types for me - and I certainly appreciate the inclusions of types there. I find that reviews of google-internal code are actually harder to do due to the missing types because I often have to context switch to look up the non-obvious return type of a function.

The proposal in this issue sounds like a nice compromise to me between never specifying types and always specifying types since the latter can get tedious in the obvious cases as well. If a middle ground is implemented, it would probably be easier to get that included in package:lints and push people towards removing type annotations from more lines of code, where they are not hindering readability.

[pq]

I think this is worth getting some real world feedback on. Given @goderbauer's endorsement, we might even see some traction in flutter which would be fantastic. I'd support an experimental lint implementation that can be experimented with and iterated on. Ideally with links to the relevant conversations and a caveat in the docs.

[eernstg]

https://dart-review.googlesource.com/c/sdk/+/374601

[bwilkerson]

@johnniwinther

[natebosch]

IIRC at least some of the folks who did not want to use omit_local_variable_types were also opposed to leaving the decision up to authors and reviewers since that can cause churn durning reviews. We should make sure that folks are OK with many type annotations being up to author/reviewer discretion in a lint that only considers a subset of local variables.
cc @devoncarew @matanlurey

[goderbauer]

were also opposed to leaving the decision up to authors and reviewers since that can cause churn durning reviews.

Yes, I for one would strongly prefer that we can fully automate where types need to go and where not to avoid back and forth in code reviews about this.

[matanlurey]

@natebosch:

We should make sure that folks are OK with many type annotations being up to author/reviewer discretion

I think if we can get to a point where types are almost always omitted, but not flagged in some cases, that's good enough (I believe that's @eernstg's prototype above). I do wonder if it's just easier to push us on to omit_local_variable_types though.

@goderbauer:

I ... prefer that we can fully automate

There isn't a world where we are going to write the perfect the lint that will be universally adopted with the exact definition of "ambiguous" type, so we either need to be pragmatic (allow code-reviewer/author-driven type annotations) or we need to be dogmatic (just use omit_local_variable_types).

In other words, I understand your request @goderbauer - I don't think it's feasible.

Consider the following cases:

// Should this be OK?
var x = 5;

// Should this be OK?
int x = 5;

T identity<T>(T value) => value;

// Should this be OK?
var x = identity(5);

// Should this be OK?
int x = identity(5);

// Should this be OK?
var x = identity<int>(5);

// Should this be OK?
int x = identity<int>(5);

This is a tiny subset of the cases that will need to be considered. For example in @eernstg's prototype:

  test_10_newGeneric_ok() async {
    await assertNoDiagnostics(r'''
f() {
  A<num> a = A<int>();
}

class A<X> {}
''');
  }

This doesn't generate a diagnostic, but AFAICT neither does this:

  test_10B_newGeneric_withVarTypes() async {
    await assertNoDiagnostics(r'''
f() {
  var a = A<num>();
}

class A<X> {}
''');
  }

In other words, authors/reviewers now have a choice between multiple styles.

[eernstg]

Those points are well taken!

@natebosch wrote:

... leaving the decision up to authors and reviewers .. can cause churn durning reviews

Creation of software has many degrees of freedom, and we already entrust authors and reviewers with a lot of freedom to make good decisions about a huge number of things. That's the way it should be! For example, they make choices all the time about which local variables to create in the first place:

void main() {
  // Approach 1.
  var x = someExpression;
  foo(x);

  // Approach 2.
  foo(someExpression);
}

The difference does matter (it affects the readability of the code, it may change the type of someExpression, etc), and it introduces (going from approach 2 to 1) or eliminates (1 to 2) the question about whether or not to specify a declared type for that local variable.

However, I don't see us introducing rigid rules about preferring approach 1 over 2 or vice versa, based on the complexity of the expression (or indeed based on anything).

Similarly, I think we can and should rely on good human judgment about local variable type annotations. Surely we can handle that!

a lint that only considers a subset of local variables.

We already have a situation where omit_local_variable_types, if enabled, will allow a mixture of local variable declarations with and without a type annotation.

num i = 1; // No complaints from `omit_local_variable_types`.

omit_obvious_local_variable_types will just broaden the set of cases where the choice is left to the developer.

@goderbauer wrote:

I for one would strongly prefer that we can fully automate where types need to go

I agree with @matanlurey here, I honestly don't think that's realistic. We could approach it from above (saying "take away this type annotation" in cases where it is considered obviously unnecessary) and from below (saying "add a type annotation here" for whatever reason, e.g., "the initializing expression has a non-obvious type").

omit_obvious_local_variable_types is intended to perform the approximation from above, in a way that should not be controversial, based on the assumption that we can all (or nearly all ;-) agree that the expressions whose type is considered trivial do actually have a type which is easy to see locally.

I suspect that the approximation from below would be much more difficult to support mechanically. To give a silly example, we could single out local variable declarations whose initializing expressions have at least 3 of the following dependencies:

• It is an invocation of a generic function/method from another package.
• Type inference has provided an actual type argument somewhere in this expression which is not mentioned explicitly (so foo() is inferred as foo<C>() and a textual search for C in the surrounding code brings up nothing).
• The type of this expression depends on the type of one or more promoted variables.
• The type of this expression relies on the least upper or lower bound of several expressions.
• This expression contains an invocation of an extension member r.m, and (1) the type R of the receiver r was inferred, and (2) there were multiple accessible and available extension members named m (so if R had a slightly different value we might call a completely different piece of code).

There are lots of ways to establish delicate typing related dependencies, and I'm just saying that we shouldn't yell at people who want to narrow the ambiguity a some situations by declaring the type of a local variable.

@matanlurey wrote:

In other words, authors/reviewers now have a choice between multiple styles.

Indeed! But I'd say that this is inherently a property of software construction and maintenance, and we should be able to handle it also in the particular case of type annotations on local variables. Moreover, omit_local_variable_types also permits both A<num> a = A<int>(); and var a = A<num>();.

[matanlurey]

To be clear my comments about choice seeming like a bad thing are in the context of wanting a single enforceable lint that does everything.

(In my own projects I happily use omit_local_variable_types with no issue, but having a omit_local_variable_types_sometimes if it doesn't have opinionated heuristics about when types are required seems like a reasonable workaround for the Dash org).

[davidmorgan]

The fact that the lint permits num i = 1 is a carefully chosen feature :) ... it means that the only use of type annotations on the LHS is for upcasts; because upcasts are statically safe it's nice to have a way to write them that's distinct from the only-checked-at-runtime as.

This lint is pretty much a cornerstone of google3 Dart style after being enforced 5 years ago, if there is a need for a variant with heuristics then I do think a new lint would be better than a change to this one.

Thanks.

[eernstg]

The fact that the lint permits num i = 1 is a carefully chosen feature :)

Certainly, and this property is preserved. Note, however, that omit_local_variable_types does not recognize many situations where a declared type matters, which means that the same feature could be broadened substantially.

List<X> listOfWhatever<X>() => <X>[];

void main() {
  double d = 1; // LINT.
  List<int> xs = listOfWhatever(); // LINT.
}

if there is a need for a variant with heuristics then I do think a new lint would be better than a change to this one.

Right, and https://dart-review.googlesource.com/c/sdk/+/374601 is indeed a separate lint whose name is different (omit_obvious_local_variable_types).

[eernstg]

omit_obvious_local_variable_types is now available in the bleeding edge SDK, with the following analysis_options.yaml:

linter:
  rules:
    - omit_obvious_local_variable_types

Feedback is welcome, of course!

[devoncarew]

FWIW, this is firing in fewer situations that I would expect (i.e., final String name = resultObject['name'] as String; doesn't show a lint).

[eernstg]

The idea that for (int i = 0, ... is needed because (for var i = 0, ... is ambiguous does not make sense.

That was my reasoning as well, initially. We can change it back and forth, until we're mostly happy, it's a one-liner. ;-)

One thing that might matter is that any integer literal will now (where the type of an integer literal is considered non-obvious) potentially cause a bigger type to be needed:

void main() {
  // Needs a type.
  Map<String, Map<bool, Map<List<double>, int>>> map1 = {
    'a': {
      true: {[2.5]: 1}
    }
  };

  // Doesn't.
  var map2 = {'a': {true: {[2.5]: 1.5}}};
}

[goderbauer]

I just got back from vacation and have been catching up on this issue. I am super excited about the latest developments here! Thank you @eernstg and everyone else for all the work that went into this so far.

I saw that the bleeding edge dart version that contains these new lints just rolled into the Flutter framework. I plan to experiment a bit with them on our code base and will report back my experiences.

[Hixie]

@matanlurey this motivating example from earlier (which IIRC came from real world code that I was testing against):

for (var x = 0; x < size.width; x += 8) {
  // what type is x?
}

...is pretty compelling to me. If we replace "int" with "var" then it's very easy to get in situations where the code looks very much like it is doing one thing, even though it's unambiguously doing the other thing if you happy to know all the rules, and editing the code later leads to unexpected behaviour (e.g. if you add a call to a function that expects a double, and pass x from the example above, suddenly you get a type error, even though you thought x was a double the whole time).

@eernstg That example is really interesting. It's a perfect example of why I think there's value in treating int literals as unobvious. Without that, you'd get no type annotations there, but the type would be very much not what you intended, and would change if you changed 1 to 1.1 (or more likely, vice versa) which is very likely when tweaking settings, e.g. when building an animation.

[matanlurey]

if you add a call to a function that expects a double, and pass x from the example above, suddenly you get a type error

Right, that's also a static error, your code will not compile and the analyzer will complain.

We will not optimize for folks using a plain-text editor to edit Dart source code in 2024.

[Hixie]

I'm not sure what you mean about plain-text editors.

My point is that when you're reading the code, despite it being technically unambiguous (all statically valid code is technically unambiguous), it's not at all intuitive what the type is in this kind of scenario.

[eernstg]

@Hixie wrote:

That example is really interesting. It's a perfect example of why I think there's value in treating int literals as unobvious.

I do see the value of treating integer literals as unobvious because an integer literal can have the type double (if and only if it has a suitable context type).

However, if it is evident that there is no context type then the integer literal will definitely have the type int (in fact, double is essentially the only context type that will cause the integer literal to have type double). So the most important question would basically be whether or not we can expect that this question about context types has an obvious answer.

In the particular case where a local variable declaration has a shape like var name = 1; or final name = 1; it is probably OK to say that the answer is obvious. Similarly, it is easy to see from double d = 1; that it does have a context type (and that it matters), but that should be fine because omit_obvious_local_variable_types will not ask for removal of that type annotation (because an integer literal with type double is considered non-obvious in all approaches that we have discussed here).

For the example where map1 needs a type annotation and map2 doesn't, the point is that none of them would need a type annotation if we were to consider an integer literal with type int as obvious. They have different types and different semantics, but that was also what I intended when I wrote the example. The point is that the double literal 1.5 has a sufficiently obvious type to allow us to omit the type annotation from the entire nested map, but if we use 1 then the entire nested map is considered to have a non-obvious type just because that tiny 1 at the end has a type that we refuse to trust. ;-)

We might then use silly tactics like this:

void main() {
  // Doesn't need a type any more, because we fixed that integer literal.
  var map1 = {'a': {true: {[2.5]: (1 as dynamic) as int}}};

  // Doesn't need a type, either. Same as before.
  var map2 = {'a': {true: {[2.5]: 1.5}}};
}

A type cast is considered to have an obvious type, so we can use ... as int to ensure that the value 1 gets an obvious typing. We need ... as dynamic because 1 as int is linted as an unnecessary cast.

The point is that even in a situation where an integer really, seriously is intended to have the type int (and it might be considered obvious that it is so), we don't have an easy and concise way to say "this is an int and I mean it!".

It is a separate discussion that we might not want complex terms to have an obvious type (because we have to read a lot of code before we know what that type is, even in the case where each "atom" in the composite term has an obvious type). I don't see an easy way out of that, but we could of course say that any expression which has more than two levels of nesting is non-obvious, or anything more than 50 characters is non-obvious, etc.

[goderbauer]

Trying out these two new lints in the Flutter framework I am running into two technical issues (I haven't looked at what the code with reduced types looks like yet):

dynamic

The specify_nonobvious_local_variable_types lint doesn't like explicit dynamics:

void _foo(Size size) {
  final dynamic bar = size.width; // 🔥 LINT: specify_nonobvious_local_variable_types
  print(bar);
}

We have about ~140 of those cases in the Flutter code base (mostly in tests). I believe an explicit dynamic should not trigger the lint.

LinkedHashMap

In a handful of instances we have code like this, which the new lint now complains about:

  LinkedHashMap<String, int> _bar() {
    final LinkedHashMap<String, int> foo = LinkedHashMap<String, int>(); // 🔥 LINT: omit_obvious_local_variable_types
    return foo;
  }

Removing the "obvious" type triggers another lint:

  LinkedHashMap<String, int> _bar() {
    final foo = LinkedHashMap<String, int>(); // 🔥 Unnecessary constructor invocation. (prefer_collection_literals)
    return foo;
  }

Fixing this lint warning results in an error:

  LinkedHashMap<String, int> _bar() {
    final foo = <String, int>{};
    return foo; // 🔥 A value of type 'Map<String, int>' can't be returned from the method '_bar' because it has a return type of 'LinkedHashMap<String, int>'. (return_of_invalid_type)
  }

How should this code be fixed?

---

I am now waiting for https://dart-review.googlesource.com/c/sdk/+/380500 to roll into the flutter framework before taking a closer look at the code.

[eernstg]

I believe an explicit dynamic should not trigger the lint.

Good point, thanks!

How should this code be fixed?

Here is one approach:

LinkedHashMap<String, int> _bar() {
  final LinkedHashMap<String, int> foo = LinkedHashMap();
  return foo;
}

The point is that LinkedHashMap() does not have an obvious type because it is generic but does not receive actual type arguments.

With abbreviated references to static properties, this could be abbreviated as follows, which could be a justification for using this ClassName<Type, Arguments> v = <instanceCreation>; style, because the instance creation expression wouldn't have to duplicate the class name:

LinkedHashMap<String, int> _bar() {
  final LinkedHashMap<String, int> foo = .new();
  return foo;
}

By the way, I don't understand why prefer_collection_literals would be happy about the original version, but it might be a good idea to change that lint such that it doesn't lint constructor invocations whose type is a proper subtype of the type of the corresponding collection literal. Alternatively, the caller of _bar should not depend on the precise implementation of that map. ;-)

[Hixie]

Regarding the int thing, my fear is more about people reading the code primed to think of something else. For example, consider a situation where you copy the collection literal with the double from a tutorial on the web into your code, then tweak the numbers until you're happy (in the process changing a 0.0 to 0 or some such, or maybe reducing a list like [0, 0.2, 1] to just [0, 1]]). You won't necessarily notice that actually the type is int rather than double, even though yes, it's unambiguous and "obvious" when you look at it in isolation.

[natebosch]

By the way, I don't understand why prefer_collection_literals would be happy about the original version, but it might be a good idea to change that lint such that it doesn't lint constructor invocations whose type is a proper subtype of the type of the corresponding collection literal. Alternatively, the caller of _bar should not depend on the precise implementation of that map. ;-)

The rule we settled on is "you can only use LinkedList constructor if the context requires that type". Adding an otherwise unnecessary type annotation is the workaround when you need to force a context type that allows the constructor invocation. omit_local_variable_types permits this.

There was still some desire to allow var m = LinkedHashMap(). It looks like @pq had some concerns, and we never had a team member pushing for that approach. #57993

[natebosch]

You won't necessarily notice that actually the type is int rather than double, even though yes, it's unambiguous and "obvious" when you look at it in isolation.

In the cases where you don't notice, it won't matter. In the cases where it matters a static error will make you notice. I don't think "this snippet of code might be ambiguous" is at all convincing when the details that are ambiguous don't cause bugs.

What are the specific wrong decisions an author would make, that would not be caught by the compiler before they cause problems, due to not understanding whether a loop variable is an int or a double?

[Hixie]

I mean, it's your call, obviously, but personally I find the programming experience to be much more pleasant (and faster) when I find I understand the code well enough to not make mistakes that the analyzer or compiler then has to tell me about.

[eernstg]

As I mentioned here, integer literals are considered non-obvious at this time. I'd suggest that we gather some experience with that.

[lrhn]

You can always write a type if you want to, and not use this lint (or an ignore), so if someone uses this lint, they want to not write some types.

We'll assign an "obvious type" to some expressions, independent of context type and always the type they would have with no context type, and if your local variable is declared to have the obvious type of its initializer expression, that declared type is unnecessary.

Then we can define two lints from this:

• Always specify non-obvious types.
• Never specify obvious types.

You can use the latter without the former to allow omitting any non-obvious types, if you think they are obvious (n.toRadixString(16) is obviously a string).
Or the former without the latter, allowing you to write some obvious types because you like to.
Or both, for a single, unambiguous rule for where to have types and where not to.

The rest is just deciding on which expressions have obvious types, and which obvious types they have.

I'm with @matanlurey and would consider var x = 0; obvious. The expression 0 looks like it has type int, and it has type int, so it's a slam-dunk wrt. interpretation in a vaccuum.

There are places where the type of 0 is not obvious, but those are not initializers of local variables with no declared type, or with type int.

Which means:

var x = 1; // Fine.
int y = 2; // LINT
double z = 3; // Fine.

The double z = 3; is fine because the obvious type of 3 is int and the declared type double is different from that, so the variable type is not obvious — so the type is necessary.
The int y = 2; lints because the obvious type of 2 is int, so typing as int is redundant.

That does require the "compute obvious type" to have access to the original source code. If a prior step throws away the 3 and says "this was a double literal with value 3.0", then we don't have the necessary information. I don't expect that to be a problem, the analyzer should have all source available, otherwise it can't show the correct warnings and errors.

One worry is that any later change to which types are obvious will cause a lot of warnings. That's why it's important to get it right, and make it simple enough that new language features are unlikely to change the obvious type of expressions that don't use the new feature. (Or where the new feature isn't visible in the syntax, but that's problematic design to begin with.)

(Also, never type your variable as LinkedHashMap. LinkedHashMap is a final implementation class, not an interface. It has the same API as Map. Just use Map, and then just use a map literal when possible. Only use LinkedHashMap for its non-trivial constructors, and you should probably not use those much.)

[gnprice]

On LinkedHashMap: it does have the same signature as Map (the same set of member names, and the same signature for each member). But I wouldn't say it has the same API, because the API is about the semantics and not only the types. LinkedHashMap implements the API of Map but adds a further guarantee to the semantics, which is that the iterable getters iterate in insertion order.

When a given piece of code relies on that insertion-order semantics, it seems appropriate to express that in the type. There are a handful of places in Flutter that do that, like HashedObserverList._map.

If we wanted a cleaner way to express that required semantics, it could look like a marker interface InsertionOrderMap. Then use sites like that one could type their variables or fields as InsertionOrderMap, leaving open whether it was LinkedHashMap or some hypothetical other implementation of the needed semantics. I'm not sure that would be used often enough to be worth the trouble of introducing it, though.

[eernstg]

@lrhn wrote:

I'm with @matanlurey and would consider var x = 0; obvious.
...

var x = 1; // Fine.
int y = 2; // LINT
double z = 3; // Fine.

This is exactly the behavior we had with earlier versions of the lints, and I'm still in the camp that prefers this approach. However, in the current version of the lints we do consider an integer literal to have a non-obvious type, unconditionally. If this behavior doesn't give rise to any complaints then that's a result which is worth having in mind as well.

It should be noted that double z = 3; is accepted with no lint message in all models under consideration (we clearly don't want the lint to recommend changes that cause the resulting type of the variable to change).

[lrhn]

@gnprice Types and semantics and APIs are correlated, but they are not the same. I'd still write the code as

// Map preserves insertion order.
final Map<T, int> _map = <T, int>{};

It seems preserving insertion order is the goal.

It's an implementation choice to use LinkedHashMap to achieve insertion ordering. There could be other insertion-ordered map implementations, and switching to another one shouldn't change anything. Written as the above, with a comment, the contract is documented, but the implementation choice for that contract is not hardwired into the API, even the internal private API, with no documentation of why that choice was made.
And no internal code depends on the _map's type being LinkedHashMap, because they all use it as a Map. There is no way to use that it is typed as a LinkedHashMap, and deliberately so.

(For the code in question, I'm not even sure preserving insertion order is that relevant. If you add the same element more than once, and remove it again, you effectively always remove the last one, because the first ones interation position is kept. That seems arbitrary, you could have removed the first one, like what ObserverList does. The order doesn't seem important at all. I think what the class really wants is a consistent iteration order between executions. But it's not documented, so I have to guess, and later coders may choose to not change the LinkedHashMap to a better type because they don't know why it's there, and giving the variable the type too makes it look even more deliberate that it has to be that type of map.)

Anyway, sorry for the diversion.

[lrhn]

If this behavior doesn't give rise to any complaints then that's a result which is worth having in mind as well.

Count me as complaining if var x = 0; is not considered OK.

But then, I'm probably not going to use the lint anyway. I might want a "do type non-obvious values" lint, but the "don't type obvious values" lint isn't worth the hassle. If I've written a type that is correct, and I simplify the expression to the point where its type becomes obvious, I don't want to be urged to remove the type. That's simply not worth my effort.

[eernstg]

As of 79e8869, omit_obvious_local_variable_types and specify_nonobvious_local_variable_types are treating dynamic just like other type annotations. In particular, this would address this comment.

[srawlins]

Can we call this closed, @eernstg? With the rules out in the wild, we can address any issues that arise as individual issues?

[eernstg]

Yes, thanks!