Skip to content
This repository has been archived by the owner on Nov 5, 2022. It is now read-only.

Omit type annotations for local variables make code harder to understand. #45

Closed
bsutton opened this issue Dec 21, 2019 · 49 comments
Closed

Omit type annotations for local variables make code harder to understand. #45

bsutton opened this issue Dec 21, 2019 · 49 comments

Comments

@bsutton
Copy link

bsutton commented Dec 21, 2019

The latest version of the 'default' google analyser settings emit the warning:

Omit type annotations for local variables (omit_local_variables_types).

In many cases this improves the readability of code by removing redundant information.

e.g.

var  i = 1;

The removal of the type (int) here doesn't affect the codes readability as the type of i is obvious.

There are however many cases where removing the type actually makes the code harder to read and for a human to do static analysis.

e.g.

    var code = _extractCode(monetaryAmount, codeLength);

There is no information on this line that tells a reader what type 'code' is.
The lack of type information forces the reader to stop and check what type _extractCode returns.

If you are analysing a class of some complexity, having to stop and check the type of numerous variables significantly slows down the process.

In a world of auto completion the reduction of a type to 'var' provides little reduction in typing.

The only other argument for reducing a type to 'var' is visual clutter.
Reduction in visual clutter should focus on removing redundant information, in the examples given the information being removed is not redundant.

I would suggest that the Omit type annotations warning should only be emitted when it is obvious what the type is.
@bwilkerson
Copy link

@davidmorgan

@eernstg
Copy link

eernstg commented Dec 27, 2019

Agreed, even on local variables a type annotation can amount to useful documentation, especially if it is initialized by a non-trivial expression involving entities declared in some other library.

For instance var x = myComplex.expression(with, lots)..of.moving = part + s may give x a type that is determined by looking up the return type of expression (which could be a generic method), filling in the results of type inference involving the static type of with and lots, and so on. The resulting type may not be immediately obvious, and the code gets less maintainable if the type of x is subject to surprising and/or subtle changes because some declarations are modified in a library that some other organization maintains.

We might be able to use a syntactic criterion for determining which expressions have an "obvious" type, and only let the lint flag those local variables that are initialized by such expressions.

@bsutton
Copy link
Author

bsutton commented Dec 28, 2019

When var was first introduced to Java I thought it was a fantastic idea. In practice the results were not very useful and just make code harder to read.

I really think var should be used sparingly.
It should only be used when the right hand side of an expression is one of:
A constructor
An intrinsic type

There is also an argument that changing int to var is pointless.

@davidmorgan
Copy link
Contributor

Thanks everyone!

I personally handled the cleanup of google-internal code, so I have seen the effect of applying this lint to a very large body of code. It was overwhelmingly positive.

Although it's possible to come up with toy examples where type annotations add information this does not seem to happen in real code.

Notice that the same lints release requires return type annotations always, so types are fixed on API boundaries. Small methods were already a good way to improve readability; now they are also the main way to fix types if you feel that is necessary.

Nothing is set in stone and we will continue to tweak the recommended lints--including the implementations of the lints--if we see ways to improve them. Feedback is important in that process, so, thanks :)

@bsutton
Copy link
Author

bsutton commented Dec 30, 2019 via email

@davidmorgan
Copy link
Contributor

Thanks Brett. Yes, I meant it's easier to read without type annotations.

The most important use case we consider for interaction with code is when you are using the Dart analyzer. So, for example, browsing code in IntelliJ. Then you can easily discover types and navigate to them.

More important than the type, though, is the exact method you're calling. That gives you not just the type of the value, but the semantics of the value--what this specific instance is supposed to do/be.

Or, with an example: why bother figuring out that you have a List<String>, when what you really want to know is part of the API of the method you're calling--that it's a list of names of special offers applicable to the logged in user.

Types are extremely powerful when it comes to catching errors and making sure code is doing something sensible/supported. But you mostly don't need to know about the details. The type system makes sure that the code fits together in a plausible way--but it can do that perfectly well without your help.

What you need to worry about as a developer is the semantics of the information flowing about, which is a level the compiler can't help you with. The idea of this lint is that you should lean on the tools for the part that can be automated, leaving you to do what we still need a human for :)

@davidmorgan
Copy link
Contributor

One more note, an appeal to the data :)

It looks like teams mostly either choose to never use type annotations or to always use them.

If it was the case that local type annotations sometimes add value, as argued, then there would be teams that sometimes use local type annotations--i.e., when they add value.

This does not seem to actually happen with any detectable frequency :)

@bsutton
Copy link
Author

bsutton commented Dec 30, 2019 via email

@bsutton
Copy link
Author

bsutton commented Dec 30, 2019 via email

@davidmorgan
Copy link
Contributor

Thanks Brett--and in particular, thanks for giving more detail. It's always useful to hear about real life examples.

I'm a little puzzled about your example. Typically I expect all methods to have type annotations, and that this is easily sufficient to constrain the types and avoid the problems you describe.

In a case where methods don't all have type annotations, but some local variables do, I definitely agree that omit_local_variable_types is harmful. Adding type annotations to all methods signatures should be done before removing them from locals.

In a case where methods are overly long, and use explicitly typed local variables to break up the flow, I also think omit_local_variable_types is harmful. In this case, the methods should be split up before removing type annotations from locals.

If neither of these applies, then I don't really see where the problems you describe could be coming from?

Thanks again for the input :)

@natebosch natebosch transferred this issue from dart-lang/sdk Jan 2, 2020
@natebosch
Copy link
Contributor

natebosch commented Jan 2, 2020 •
edited
Loading

The code base had zero type annotations. The code passed some thirty message types around. When trying to analize the code we where getting no where. So we decided to take the fairly invasive approach of adding types to core paths of the library. As you might expect the compiler spat out numerous errors that the original JavaScript Devs weren't even aware of, but more importantly we were able to reason about code in a way that simply wasn't possible pre adding types.

This situation can happen when looking at argument or return types, which are encouraged and not impacted by this lint. This situation will not happen for variable types. Changing a var to the type that was inferred, or vice versa, will never add static errors or change runtime behavior. Static errors and runtime behavior can change if the replacement is to a type that wasn't inferred, but these will not trigger the lint. For example changing num x = 1; to var x = 1; or the reverse can have an impact, and will not have a lint. Changing between int x = 1; and var x = 1; will have neither a static nor runtime impact, and the int version will have a lint.

@bsutton
Copy link
Author

bsutton commented Jan 2, 2020

Let my try and find some examples where I think this lint is harmful. This will likely take me a week or so (I'm on holidays) and I will get back to this issue.

@eernstg
Copy link

eernstg commented Jan 4, 2020 •
edited
Loading

I think the main point is breakage modes. @davidmorgan wrote:

Yes, I meant it's easier to read without type annotations

It may be easier to read a snippet of code when local variables have no type annotations, but the declaration of a type on a local variable whose initializing expression is non-trivial introduces a dependency barrier as well as documentation: The developer who writes T x = e; commits to the type T for x, and documents this choice.

The point is that the type of e may be complex to compute (and involve inference and promoted variables), and it may depend on declarations is other libraries, maintained by other organizations.

So when some tiny little part of that complex setup changes and the code suddenly has compile-time errors (or, worse: it breaks at runtime because some newly created List now has a different type argument, or somesuch), a small, judiciously chosen set of local variable type annotations will make it easier to see exactly where the breakage originates, and why.

This is about 'breakage modes' in the sense that we may either have breakage at run time where it could have been at compile-time, or it may happen "late" (that is, further down the dependency graph where a variable x is used) rather than "early" (say, at T x = e;) because e no longer has type T.

So casual reading may be easier without type annotations, but when a deeper and more precise understanding of the code is needed, e.g., because it has compile-time errors or breaks at run time, the documentation and dependency barrier provided by a few type annotations may be helpful.

@davidmorgan
Copy link
Contributor

I think the main point is breakage modes. @davidmorgan wrote:

Yes, I meant it's easier to read without type annotations

It may be easier to read a snippet of code when local variables have no type annotations, but the declaration of a type on a local variable whose initializing expression is non-trivial introduces a dependency barrier as well as documentation: The developer who writes T x = e; commits to the type T for x, and documents this choice.

My argument is that that's incredibly rare. You still have the option to pull out a private method when it looks like an issue.

The point is that the type of e may be complex to compute (and involve inference and promoted variables), and it may depend on declarations is other libraries, maintained by other organizations.

Overall we make computation of types much simpler by forcing them all to the RHS.

Prior to this lint being enforced you had to worry about LHS->RHS interacting with RHS->LHS inference in figuring out the actual type. Now, the inference that can happen is much simpler.

So when some tiny little part of that complex setup changes and the code suddenly has compile-time errors (or, worse: it breaks at runtime because some newly created List now has a different type argument, or somesuch), a small, judiciously chosen set of local variable type annotations will make it easier to see exactly where the breakage originates, and why.

List types is a good example of something that just got simpler. There's now only one place to put T, the RHS. Same goes for generic type parameters, e.g. constructors; they are now largely written with the constructor invocation, instead of sometimes coming from the LHS via inference.

This is about 'breakage modes' in the sense that we may either have breakage at run time where it could have been at compile-time, or it may happen "late" (that is, further down the dependency graph where a variable x is used) rather than "early" (say, at T x = e;) because e no longer has type T.

So casual reading may be easier without type annotations, but when a deeper and more precise understanding of the code is needed, e.g., because it has compile-time errors or breaks at run time, the documentation and dependency barrier provided by a few type annotations may be helpful.

There's a particularly nasty case where a method does

T doSomething<T>() => foo as T;

which acts as a cast to whatever you write on the LHS:

int foo = doSomething();

With this change, it's clear that int is not just a type annotation, it's a parameter to the method:

var foo = doSomething<int>();

The same applies to the complex expressions you're talking about: if we allow types on the LHS, they can become parameters to arbitrary parts of the RHS. A type on the LHS can be a combination of four things: an upcast, a downcast, a parameter to the RHS or a comment.

So, as well as being more readable, removing types from the LHS actually makes it easier to reason about what the code is doing when you start to dig into the details.

@eernstg eernstg mentioned this issue Jan 7, 2020
Closed
@ShivamArora
Copy link

What you need to worry about as a developer is the semantics of the information flowing about, which is a level the compiler can't help you with. The idea of this lint is that you should lean on the tools for the part that can be automated, leaving you to do what we still need a human for :)

Just an argument over this.

How can someone infer the type when reading the code for GitHub or while reviewing a pull request?

We don't have our IDEs there which would tell what the method is returning on the right side of the assignment.

This rule seriously affects the readability of the code.

@davidmorgan
Copy link
Contributor

Thanks Shivam.

That is a good point, thank you.

Internally at Google we browse and review code via tools that understand Dart. They're not as powerful as an IDE view but they do allow following xrefs to dig into types. I agree the experience on Github is not as good.

It looks like this is the SDK issue for adding github navigation support: dart-lang/sdk#37418

Unfortunately, I don't think that will happen any time soon.

I don't work a lot with open source code so I don't have a personal recommendation. One possibility would be to load the code in your IDE to review it. I imagine this would be annoying to do at first--but might pay off in the long run. I'd be interested to hear if someone tries it.

I'll comment over at the SDK issue that this is now a more important difference between google internal and Github coding environments.

Thanks.

@eernstg
Copy link

eernstg commented Jan 7, 2020 •
edited
Loading

@davidmorgan wrote:

My argument is that that's incredibly rare. You still have the option to pull out a private method

Given that the goal is to document and commit to a specific type for a local variable whose initializing expression is non-trivial, I'm not quite sure how helpful that would be. But we could certainly ignore the lint:

// Somewhere over the rainbow, in code maintained by other folks.
class SomeWidget<X> ... {...}
SomeWidget<Y> foo<Y extends num>(List<Y>) {...}

// My code.
void main() {
  var xs = someExpression;

  if (xs is List<int>) {
    // We fully trust those other folks, and inference on `xs`, to determine the type of `w1`.
    var w1 = foo(xs);

    // Using a local function to explicitly document and commit to a type for `w2`.
    // Wouldn't a private method be similarly verbose?
    SomeWidget<int> localFoo(List<int> xs) => foo(xs);
    var w2 = localFoo(xs);

    // Disabling the lint will allow us to directly document and commit to a type for `w3`.
    // ignore: omit_local_variable_types
    SomeWidget<int> w3 = foo(xs);
  }
}

But is it actually an incredibly rare situation that you wish to document and commit to a specific type for a local variable? I'd think that it should be relatively rare, but allowed. We shouldn't chastise developers for making a careful and reasonable judgment.

That's the reason why I think the lint should flag cases where the type of the initializing expression is "obvious", not simply the cases where inference produces the same result. I believe that we can define a reasonable rule for which types are "obvious".

Overall we make computation of types much simpler by forcing them all to the RHS.

I agree on that in general. For instance var xs = <int>[]; is a more direct way to control the type argument of the list than Iterable<int> xs = [];, and I acknowledge that it is a source of complexity that the type annotation can be

an upcast, a downcast, a parameter to the RHS or a comment.

However, the variable type annotation serves to document and commit to a specific outcome of the computation of the type of the initializing expression. In that sense the type annotation is at least partially redundant, and that's not so surprising: this is a common property of abstraction barriers, and a type annotation on a local variable is a tiny abstraction barrier.

You could also say that the type annotation is declarative: Specify the desired outcome and let some computational step (namely type inference) make it so. In contrast, the explicit type elements in the initializing expression serve as inputs to the computation, basically eliminating type inference.

But passing type arguments etc. in the initializing expression may be an indirect and convoluted way to document and commit to a specific type for the local variable: var w = foo<int>(xs); will give w the type SomeWidget<int>, and we can't see that locally.

Also, we have a rule against passing actual type arguments in an invocation of a generic function whenever inference would fill in the same type.

The main point is that "inference would fill in the same type" is not the same thing as "at this point I want to document and commit to a specific type for this variable". The latter includes a software engineering judgment and the former is just a mechanical computation.

@ShivamArora
Copy link

One possibility would be to load the code in your IDE to review it. I imagine this would be annoying to do at first--but might pay off in the long run. I'd be interested to hear if someone tries it.

Thanks @davidmorgan .

But consider having to checkout the PR every time within an IDE just to review the code adds a whole lot of extra effort.

And people wouldn't like to go that way, they would have to do this for just one programming language out of a whole lot and even within that language for just one linter rule that's being strictly enforced.

It's better to mark this linter rule as optional rather than strict.

With this rule being optional, those who want to use this can effectively use it.

And it wouldn't affect others who are raising concerns over why this rule has been made strict.

Surely, people can exclude the rule in analysis_options.yaml, but the pub.dev website uses the standard pedantic rules and thus affects the package score.

@davidmorgan
Copy link
Contributor

Thanks Shivam. The decision about pub.dev will happen over at package:pana: dart-lang/pana#593 and dart-lang/pana#601

@davidmorgan
Copy link
Contributor

Thanks Erik.

I think the problem with "obvious" is that it's not the right measure; what we want is "important". Something can be non-obvious but also not important. If you're taking a type from an API, then passing it right back into the same API, it's arguably better if you don't assert on the type. The API is clearly designed to accept its own output.

What I saw in the code is that people usually either always write type annotations or always omit them. That tells me that people are not thinking about which type annotations are important, and if we let people add type annotations under any broad set of circumstances, they will mostly just add none or all.

I agree it feels like there should be a way to assert on a type. But, I don't think type annotations do this particularly well. Because of type inference and casts, they are not unambiguously a declaration about the type. They might be part of determining the type and they might not precisely match the actual runtime type.

I wonder whether we might want to use something like this to more clearly signal intent:

void assertType<T>(T t) {}

--but right now that's also confusing due to implicit casts.

@eernstg
Copy link

eernstg commented Jan 7, 2020

@davidmorgan wrote:

Thanks Erik.

And thanks, David, for keeping this discussion so thoughtful and interesting!

If you're taking a type from an API, then passing it right back
into the same API, it's arguably better if you don't assert on the type.

That particular case shouldn't be a problem: The updated omit_local_variable_types lint that I'd prefer would always accept var x = e;, it is just more forgiving than the current lint because it allows T x = e; when e does not have an obvious type.

people usually either always write type annotations or always omit them

That could be a lint thing. If we introduce a lint that actively encourages developers to omit a lot of obvious type annotations (just like today), but allow them to have some non-trivial type annotations where they really want them, we might be able to help each other developing a good style in this respect.

@bwilkerson
Copy link

Just to state the obvious: If we want to relax the rule we'll need to be able to clearly define what we mean by "important" or "obvious" in terms of the static semantics of the code. That isn't always easy to do.

@natebosch
Copy link
Contributor

This is a difficult conversation to have, because the toy examples we come up with suffer from other readability issues. var w1 = foo(xs); is not unreadable because of the var, it is unreadable because of the meaningless foo, w1, and xs. That's not to say that a local type annotation is never a readability benefit, but I'm fairly confident I've never come across such a scenario that wasn't better fixed by other refactorings or renames. I have used this lint in projects for years and I can't recall a time I thought "this would be easier to read if I didn't need to write var here."

So when some tiny little part of that complex setup changes and the code suddenly has compile-time errors (or, worse: it breaks at runtime because some newly created List now has a different type argument, or somesuch), a small, judiciously chosen set of local variable type annotations will make it easier to see exactly where the breakage originates, and why.

This is the argument brought up in the fixnum package, that there would be performance penalties if an int ever became inferred as an Int64 because of a bug elsewhere, and so we must explicitly type our locals as int or double. I'm also not convinced that this situation would happen in the real world. Maybe with overuse of dynamic in other places it could crop up? We encourage typing function arguments and return values, so the type constraints end up feeling very local even if they aren't on variables themselves since it's not often to have a large chunk of code that doesn't deal with other functions.

It would be incredibly valuable if any of the proponents for type annotations on local variables could try living with the lint for a while and see where they hit either of these two cases - where the code is more readable or less prone to breaking, and reply with real world code instead of toy examples.

@zathras
Copy link

zathras commented Jan 14, 2020 •
edited
Loading

I'd like to offer a perspective in support of relaxing or eliminating the application of omit_local_types.

Viewing a local type declaration as a kind of assertion is, I think, spot-on. It asserts exactly the right thing for most cases, too: It asserts that the object is assignable to the given type. That's generally preferable to an assertion on the actual runtime type of a given object. Statically typed OO systems have featured first-class support for making this assertion at compile time for decades, to good effect.

Further, I find the argument that it's not much effort to figure out the static type constraints of a variable in an IDE to be, at best, unconvincing. A goal of software authorship should be to make software readable with a minimum of effort. "Not much effort" is still more than "no effort," so in effect forbidding this form of documentation/assertion does a disservice to the reader. This is one of the main principles behind Donald Knuth's literate programming; good reusable software is read more often than it's written.

Another problematic case is when software intentionally uses a type up the hierarchy, even though a more specific type is knowable via static analysis. I do this all the time; it's a local application of the venerable dependency inversion principle. By doing this, the following code can't accidentally depend on something that's not part of the supertype's contract. As an example, see https://github.com/zathras/misc/blob/master/dart/jovial_misc/test/io_utils_tests.dart (search for omit_local_variable_types).

The broad net that omit_local_variable_types casts does a fair bit of damage, and following it eliminates well-established practices for good software engineering.

The argument in favor of it seems to be mostly that it avoids clutter.

This goal could be met sufficiently well with a narrower rule that avoids the very real damage done by omit_local_variable_types. For example, a lint rule could be written that flags local variable types in the case where the given type is explicitly mentioned on the top level of the RHS expression tree. So, for example:

    Frobber frobber = Frobber();

would be flagged, but

    Frobber frobber = someFunction();

would not be. Nor should it! An automated test can't determine whether or not someFunction()'s return type is "obvious enough" to the reader. That's a human judgement, and it depends on the audience. Further,

    FrobberSupertype abstractFrobber = Frobber();

would, correctly, escape the lint chopping block.

@natebosch
Copy link
Contributor

natebosch commented Jan 14, 2020 •
edited
Loading

Another problematic case is when software intentionally uses a type up the hierarchy, even though a more specific type is knowable via static analysis.

This is allowed and won't be flagged by the lint. Try your FrobberSupertype example to validate that it is allowed.

Viewing a local type declaration as a kind of assertion is, I think, spot-on. It asserts exactly the right thing for most cases, too: It asserts that the object is assignable to the given type.

The variable types that get removed because of this lint will have no runtime impact. An assertion is generally something that is checked at runtime. The lint allows you to keep local type annotations that do impact runtime, for instance those that are a downcast.

An automated test can't determine whether or not someFunction()'s return type is "obvious enough" to the reader. That's a human judgement, and it depends on the audience.

As above, this isn't unreadable because of a local type on the variable, it's unreadable because the name someFunction carries no meaning.

@zathras
Copy link

zathras commented Jan 14, 2020

Another problematic case is when software intentionally uses a type up the hierarchy, even though a more specific type is knowable via static analysis.

This is allowed and won't be flagged by the lint. Try your FrobberSupertype example to validate that it is allowed.

I stand corrected.

... An assertion is generally something that is checked at runtime.

Actually, no. In the software engineering community, saying that something is a "compile-time assertion" is common. Compile-time assertions are viewed as superior to runtime assertions, because they're caught earlier and more reliably. This goes way back.

Of course, I agree that the Dart "assert" facility (logical descendant of C's <assert.h>) is for runtime assertions, but the word "assertion" is not so narrow. A type declaration is a compile-time assertion the author is making about their code. And it's often a valuable one.

An automated test can't determine whether or not someFunction()'s return type is "obvious enough" to the reader. That's a human judgement, and it depends on the audience.

As above, this isn't unreadable because of a local type on the variable, it's unreadable because the name someFunction carries no meaning.

It doesn't carry no meaning, it just doesn't carry meaning that easily identifies the static type. Your argument amounts to saying that function names should make the static type of the return value obvious in all cases. The rejection of Hungarian notation in environments with better static typing support demonstrates the fallacy of that argument.

@bsutton
Copy link
Author

bsutton commented Jan 14, 2020 via email

@bsutton
Copy link
Author

bsutton commented Jan 15, 2020

In the end I think we are forgetting about the letting the developer make important decisions.

Just the amount of conversation that this lint has generated strongly suggest that a segment of the community thinks this lint (as its written) is a poor idea.

Up until this point I've had no problem with all of the 'standard' google lints.
They provide consistency and make the code more readable, even if I don't necessarily agree with every decision.

It is clear that a segment of the community believe this lint (as it stands) makes the code less readable.
I strongly disagree with the idea of forcing programmers to include the 'type' in a method name.
I worked as a windows dev for many years using hungarian notation and the problem is that half the time the notation is wrong. Lets not go back down that path.

I would agree with @zathras proposal to relax the rules.

@natebosch
Copy link
Contributor

I strongly disagree with the idea of forcing programmers to include the 'type' in a method name.

We have no intention for forcing, or advocating for this. One of the principles behind this practice is that the type is very often not the thing that conveys meaning to the reader. When I talk about how readable the code is I mean how easy it is to read and understand intent and behavior. I specifically don't mean how easy it is to know the types. I don't think we have seen any data pointing to authors including types in method names in response to enabling this lint.

Just the amount of conversation that this lint has generated strongly suggest that a segment of the community thinks this lint (as its written) is a poor idea.

Yes, we are still trying to figure out the right balance to strike here. This conversation is an important piece of that.

We have multiple, sometimes competing goals. Developer comfort is one goal and for a subset of developers it is clear that this goes against developer comfort. Helping developers use Dart effectively is another goal, and one of the benefits that we have seen with enabling this lint is that developers who are transitioning from a language which requires variable types are more likely to write idiomatic Dart.

@davidmorgan can correct me if I've overlooked some feedback. As far as I'm aware all of the internal projects who were opposed to this lint have been migrated anyway, and I don't think we've had significant pushback since, or been shown any non toy examples of code that becomes less readable after removing local variable types.

@bsutton
Copy link
Author

bsutton commented Jan 15, 2020 via email

@zathras
Copy link

zathras commented Jan 15, 2020

I strongly disagree with the idea of forcing programmers to include the 'type' in a method name.

We have no intention for forcing, or advocating for this. One of the principles behind this practice is that the type is very often not the thing that conveys meaning to the reader. When I talk about how readable the code is I mean how easy it is to read and understand intent and behavior. I specifically don't mean how easy it is to know the types. <...>

Well, limiting the discussion of names to just that contradicts one of the centerpieces of the stated argument in favor of this lint: rule: That the type is obvious to the reader. That's why I said "Your argument amounts to saying that function names should make the static type of the return value obvious in all cases."

I of course agree that this rule doesn't directly require Hungarian notation. But when you think through the implications of adopting the rule, the end result goes in a similar direction. If nothing else, it replaces a proven, reliable mechanism for documentation and avoiding bugs with an unenforceable and vague design goal.

<...> one of the benefits that we have seen with enabling this lint is that developers who are transitioning from a language which requires variable types are more likely to write idiomatic Dart.

You can achieve that goal with a much less violent lint rule, e.g. as outlined above.

In sacrificing the documentation and static correctness checking of well-placed type declarations, you're throwing the baby out with the bathwater.

... internal projects ...

Lack of pushback in internal projects is a terribly unreliable indicator, particularly over this time scale.

Indeed, as with much in software engineering, I'd expect the consequences of a bad decision here to be most acute months and years later, when the code is no longer fresh in the minds of those maintaining it.

This is an area where there is wisdom in treading lightly, with an appropriate level of humility.

@bwilkerson
Copy link

I'm assuming on a per project or per file basis we can disable specific lints.

Yes. For the whole project, include something like the following in the analysis options file:

analyzer:
  errors:
    omit_local_variables_types: ignore

For a single line, include a comment of the form

// ignore: omit_local_variables_types

either on the line before or on the same line as the location of the lint.

For a whole file, include a comment of the form

// ignore_for_file: omit_local_variables_types

before the first directive or declaration in the file.

@davidmorgan
Copy link
Contributor

Thanks everyone. As Nate says, discussion around these issues is valuable, so thanks for the input/feedback.

Nate is correct: when we enforced this on all google internal Dart recently there was almost no negative feedback. Actually, less than on this thread. So, it looks like google is happy with it.

In the end that's what pedantic is: the set of lints that google is happy with. It's what we enforce--so it's supposed to also be a good starting set for external projects.

You are free to customize on top of it :)

And encouraged to give feedback, since that can influence how it develops in future.

Thanks :)

@bsutton
Copy link
Author

bsutton commented Jan 15, 2020

Given @bwilkerson above mentioned customisation I can probably live with it.

@eernstg eernstg mentioned this issue Jan 23, 2020
Closed
@camsteffen
Copy link

Let me see if I can summarize the two views here...

In favor of the current lint:
I should be able to easily infer the semantic meaning of variables when reading the code. This can always be accomplished through meaningful variable and function names. I only need to know the specific type of variables when debugging the code, and my IDE can help in those cases. Code is read more than it is written. The compiler is capable of automatically catching type-related errors in a large majority of cases.

Opposed to the current lint:
I should be able to easily infer the type of variables when reading the code. Knowing the type is crucial for understanding how the code works and easily identifying bugs. Even if a function is appropriately named to convey semantic meaning, the type may not be obvious. Also, type annotating a variable makes it resilient to accidentally changing the type in the future.

Middle ground

@bsutton suggested splitting the lint in two. I think that's a good idea and here's how it can work.
omit_constructed_local_var_types (okay, that's probably too long)
This lint will flag type annotated variables that are initialized with a constructor or a literal.

@zathras
Copy link

zathras commented Feb 8, 2020

Middle ground

@bsutton suggested splitting the lint in two. I think that's a good idea and here's how it can work.
omit_constructed_local_var_types (okay, that's probably too long)
This lint will flag type annotated variables that are initialized with a constructor or a literal.

SGTM - that's essentially what I outlined above: "[...] a narrower rule that avoids the very real damage done by omit_local_variable_types. For example, a lint rule could be written that flags local variable types in the case where the given type is explicitly mentioned on the top level of the RHS expression tree.". The "constructed" idea is good - more precise.

BTW, in some subsequent coding, I came across a really good example where declaring a local variable type makes the code more readable, and reliable:

final phoneURL = 'tel: $f';
final smsURL = 'sms: $f';
final mailURL = 'mailto: $f';
final webURL = f;
final Future<bool> phoneOK = canLaunch(phoneURL);
final Future<bool> smsOK = canLaunch(smsURL);
final Future<bool> mailOK = canLaunch(mailURL);
final Future<bool> webOK = canLaunch(webURL);

canLaunch() is from the url_launcher package. It returns a future for a good reason, but that reason isn't at all obvious if you haven't looked at the internals. Making the return type obvious from the name (canLaunchFuture?) would have been horrible; that's like Hungarian notation. In this case, there's a good reason to defer the await, which happens in a place that's not right next to the canLaunch() calls.

Simply providing the local variable type is a nice, compact way of saying to the code reader "Be aware this is a future. I'm doing this on purpose." And it has the advantage that the static assertion that the return type is a future is proven by the compiler.

BTW, personally, I find it nice that specifying the variable type is optional rather than mandatory -- in the case of the four strings, specifying the type just adds clutter. But I think this example shows why it hurts readability to forbid providing the type, as the current policy attempts to do.

@natebosch
Copy link
Contributor

Simply providing the local variable type is a nice, compact way of saying to the code reader "Be aware this is a future. I'm doing this on purpose." And it has the advantage that the static assertion that the return type is a future is proven by the compiler.

If you are using await_only_futures you shouldn't need to worry much about type annotating variables of type Future. Other than the few methods on Object there isn't between bool and Future. Any use of the variable will give you that static assertion (not at the variable declaration, but not too far away) and it generally won't be very easy to confuse them. The exception to this is that if I'm using print debugging I will occasionally end up with a not useful String representation of the Future instead of what I wanted since .toString() exists on both. These cases are rare and don't impact the production ready code.

This lint will flag type annotated variables that are initialized with a constructor or a literal.

We had this idea come up in previous discussions. It could be worthwhile to raise an issue at https://github.com/dart-lang/linter/issues to ask for this. I don't think it will change the decision for package:pedantic, but it could be a factor in decisions around package:pana and the health score on the pub package site. cc @mit-mit

@zathras
Copy link

zathras commented Feb 8, 2020 via email

@nogicoder
Copy link

nogicoder commented Feb 27, 2020 •
edited
Loading

For case like this:

Future<List<Client>> fetchClientList({int branchId, String token}) async {
    try {
      final response = await dio.get('/branches/$branchId/contacts',
          options: Options(headers: {
            HttpHeaders.authorizationHeader: 'Bearer $token',
          }));
      // Response is returned successfully
      if (response.statusCode == 200) {
        final clientList = [];
        response.data['data'].forEach((clientData) {
          clientList.add(Client.fromJson(clientData));
        });
        return clientList;
      }
      return null;
    } catch (e) {
      print(e.toString());
      return null;
    }
  }

The exception will be caught since the clientList is not explicitly defined as List. But if I define it, the analyzer will warn on the type annotations.
The current way I'm doing is to cast List.from() for the returned clientList.

@bwilkerson
Copy link

I believe that the preferred solution is to initialize clientList to <Client>[]. The compiler will then infer the type of clientList to be List<Client>, and you won't need to create a second list.

@nogicoder
Copy link

I believe that the preferred solution is to initialize clientList to <Client>[]. The compiler will then infer the type of clientList to be List<Client>, and you won't need to create a second list.

Thanks Brian! I'll try that in my code

@natebosch
Copy link
Contributor

Another option on recent SDKs:

        final clientList = <Client>[];
        response.data['data'].forEach((clientData) {
          clientList.add(Client.fromJson(clientData));
        });
        return clientList;

could be

        return [
          for (var clientData in response.data['data'])
            Client.fromJson(clientData),
        ];

@camsteffen
Copy link

Hahaha that's funny @natebosch! There are no programming languages that cool.

@insinfo
Copy link

insinfo commented Mar 24, 2020

I'm having a problem with this, I really think that this analysis fails for some cases like this.

image

@natebosch
Copy link
Contributor

@insinfo - I don't understand how your code relates to that error. I don't see any List<dynamic> getting instantiated here.

If, in response to this lint, you changed the code to this:

final results = [];

That will cause a List<dynamic> to be instantiated and could lead to a stack trace like you show here. The idiomatic way to write that is:

final results = <String>[];

Even better might be:

final results = [
  for (var block in mBlocks)
    block.buildStr(this),
];

@insinfo
Copy link

insinfo commented Mar 24, 2020

and for this case

image

@ibcoleman
Copy link

Nate is correct: when we enforced this on all google internal Dart recently there was almost no negative feedback. Actually, less than on this thread. So, it looks like google is happy with it.

I'm firmly in the "types sometimes make the code more legible" camp, but boy do I find the above argument incredibly uncompelling.

@matanlurey
Copy link

Because these lints are enforced in google, I don't see us removing this lint. It's trivial to ignore it:

# analysis_options.yaml

include: package:pedantic/analysis_options.1.9.0.yaml

analyzer:
  errors:
    omit_local_variable_types: ignore

In terms of what affects pub scoring, that is better served by filing issues here:
https://github.com/dart-lang/pana/issues

@ShivamArora ShivamArora mentioned this issue Aug 9, 2020
Closed
@Aaron009
Copy link

我假设基于每个项目或每个文件，我们可以禁用特定的棉绒。

是。对于整个项目，在分析选项文件中包括以下内容：

analyzer:
  errors:
    omit_local_variables_types: ignore

对于单行，请包含以下形式的注释

// ignore: omit_local_variables_types

在皮棉位置之前或同一行上。

对于整个文件，请包含以下形式的注释：

// ignore_for_file: omit_local_variables_types

文件中第一个指令或声明之前。

Misspelling, it should be： omit_local_variable_types

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests