Issues with forced omit_local_variable_types

[DavBfr]

In my library, I use always_specify_types to be sure that the public functions and variables have a proper type, but now with omit_local_variable_types I have a lot of hints that I cannot correct as both rules contradict each other.

[isoos]

@DavBfr: I feel your pain, personally I'd prefer to do the same. However both pedantic and effective_dart has the omit_local_variable_types rule, and none of them has the other.
https://dart-lang.github.io/linter/lints/omit_local_variable_types.html

Traditionally at pana we have followed pedantic, and I think that will not change in the near future. There is an opt-out, you could add the following item at the top of your files:

// ignore_for_file: omit_local_variable_types

With the current setup this will omit the lint messages and won't affect the package's health score either. However, there is no long-term commitment to this opt-out, it just happens to work, and we haven't done any countermeasures.

[isoos]

Another thing: I'm not sure about the lint process, but it may be worth to dispute the lint in pedantic's repo: https://github.com/dart-lang/pedantic

[DavBfr]

Thanks, I will try that. I'm OK with omit_local_variable_types, but I would need another rule to check for public API types. Maybe an issue for the analyzer team?

[pq]

@DavBfr: a great conversation to kick off over on the pedantic tracker (and/or linter). Cheers!

[zathras]

I commented in the referenced issue, but over here, have you considered making a long-term commitment to accepting ignore_for_file: opt-outs in pub? After all, that's an explicit statement that says "I've considered your advice, but have made an explicit decision otherwise." Personally, I see nothing wrong with that; I think we need to be accepting of the fact that different people/teams/groups prefer different coding styles.

Consider, for example, pointycastle. Does it really deserve -417.33 points under "health suggestions"? Given the nature of this popular and important library, it's more a case of a difference of opinion over some of the pedantic lints. If the author made a statement that, in effect, said "I've considered these lint rules and rejected them," I'd happily take him at his word.

Alternately, maybe something project wide? Kind of like analysis_options.yaml, but accepted by pub?

My main point is this: There's a big difference between an experienced developer saying "I know about your advice, I've considered it, and I've come to a different conclusion," and an inexperienced developer going against advice because they've never considered it. The current setup doesn't distinguish these two cases.

As a side benefit, you could run statistics on which rules folks opt out of, and use that as an input to weed out bad rules!

[gothamgasworks]

It's funny that it matters whether I use single or double quotes for strings depending on the content, but I can't specify the type of a local variable if I want to get more points on pub and don't want to write an ignore comment in every single source file.

[isoos]

@zathras Thanks for the suggestions.

I think the framing of "Does it really deserve -417.33 points?" is a bit misleading, rather it should be "Does the analysis and the score communicate to other developers what to expect when using the package?" And the later should include a lot of things, among others:

• Does the package get fixes and updates?
• Does the package confirm the now-current standards and style?
• Does the package have good test coverage?
• Does the package have lots of users? Or a few, but they are using it instead of another alternative for a reason?
• Will it be easy to use?
• Will it be easy to fork (if/when needed)?

Code style (including code indent, naming conventions, code granularity, idioms and patterns used AND avoided) is inherently subjective and contains a lot of personal preference. Having a widely accepted standard is useful, and pointing out that a packages conforms or diverges from the now-current standards and style is a value for the developers using the package in question.

The package author can decide to ignore the recommended standards, they can publish the package, but I think it is fair to the others to receive good visual indications when this happens, what is the extent of it, and what they can get out of investing their time using the package.

I'm not saying that any penalty is deserved, the current system has many flaws, which is kind of expected if you want to distill complex answers to a single number. We have started to explore the idea of switching the current penalty system to positive scores. Maybe it could be better, but it is not a simple effort to design and implement such scoring that reflects the values of the package in a meaningful way. Every input is appreciated!

Also: as a stopgap measure, we've limited the penalty for lints to 25 health points, and temporary reverted back to pedantic 1.8.0. These changes may not be live on pub.dev as of now, but will be soon.

[zathras]

@zathras Thanks for the suggestions.

I think the framing of "Does it really deserve -417.33 points?" is a bit misleading, rather it should be "Does the analysis and the score communicate to other developers what to expect when using the package?"

Sure, using the big number was a bit of hyperbole on my part. As you say, this points to the problem of trying to boil down "quality" to a single number. It also suggests that some rules are more important to real quality than others. I picked pointycastle because it's a particularly useful library (and presumably a high-quality one, given its pedigree) that received a particularly low score by this metric, and it looks like this was based on some of the more marginal and questionable lint rules. Going through something the size of pointycastle to obey all of the new lint rules would likely be A Bad Idea (due the the danger of introducing bugs in the process); a more selective application of the truly important rules would be the wiser option. Disallowing opt-outs is a disincentive to selectively adopting new rules.

But pointycastle is just an example.

There's a more fundamental, and more important disconnect at work here. According to the maintainers of the lint rules pub uses, their mandate is "the set of lints that google is happy with."

pub is marketed as something more: As a resource for the larger community. That's a much larger and more diverse group.

From this more diverse audience, it's not surprising to see a greater willingness to speak out when a lint rule does actual harm, as I and others contend is the case with the overly-broad omit_local_variable_types.

If the party line on pedantic is that their audience is internal to google, then there needs to be a home for a less narrowly focused set of rules enforced in the greater community. I think there's real value in having a score that's highly visible, IF that score reflects rules around which there's a real community consensus. That goal is at odds with a fast-evolving set of internal rules, where experimentation can be more tolerated (and where, frankly, there are more levers to squash dissent).

Letting in rules that aren't ready for prime time (like omit_local_variables) devalues the notion of assigning a health score based on lint rules, and that's a real loss.

[zathras]

Writing a bit of code, I ran across an example where providing local types makes the code easier to understand, and more maintainable:

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 my assertion that the return type is a future is proven by the compiler.

(BTW, This example was not cross-posted to the related Google-internal issue, since that's not my direct concern. Feel free to share if you like, though.)

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.

[zathras]

Fun fact: The announcement for Dart 2 has this as its first code example, extolling the virtue of sound typing:

void main() {
  List<int> prices = ['99', '27', '10000', '20000000'];
  
  // Sort in place from smallest to largest
  prices.sort();
  
  print('Lowest price is ${prices[0]}!');
}

This example would be flagged as "bad" by omit_local_variable_types...

Note also that this May 2019 article by Google (I would say correctly) identifies omit_local_variable_types as overeager.

It's nice to know that we're not the only ones who think this lint rule in its current form is A Bad Idea!

[ShivamArora]

So, it's been more than 8 months since this issue is open and a similar issue which was opened at pedantic: googlearchive/pedantic#45 (comment)

The issue at pedantic has been closed with a statement that this lint is enforced at Google and they won't remove it from pedantic and pana is the appropriate place to report package scoring issues due to this controversial lint.

I just have a quick question to the pana team over here and I want to bring @isoos to this conversation.

Who's pana and pub supposed to serve? Is it google or a wider worldwide developer community?

If it's google, then I must say please stop saying that pub store is a store for the community if the voice of community is going to be rejected on a plea of Google rather than a more appropriate and sensible response.

If it's for a wider worldwide developer community, why not make the community happy with what they want rather than a controversial lint.

I'm ok with pana using pedantic as it's base.

But pana can still make use of a separate analysis options file to override the pedantic rules as agreed by the community and not just by Google.

[jonasfj]

Who's pana and pub supposed to serve? Is it google or a wider worldwide developer community?

I think it's safe to say that pub is meant to serve the wider developer community, in fact from published research it is well known that internally at Google all code lives in a large mono-repository.

I'm not sure how this argument is constructive.

---

We are where we are because picking a set of lints is hard.
There is value in having consistent lints applied, as it makes jumping from one package to another easier.
I think that striving for consistency is how we got where we are.

I think we picked pedantic a long time ago, because we didn't want to do a lot of bike shedding over which lints to enable/disable. Picking lints of often very subjective.

Currently, we don't even enforce the latest pedantic because it required too many packages to change.

We could choose to give packages points for having an analysis_options.yaml and honoring it, or pick a minimal set of lints we want to force on all packages. Or do something else entirely.

I think there was some work on trying to pick a set of default lints, and that's why we've been avoiding discussions on issues like this :)

[isoos]

@ShivamArora: I think this may be actually a bug, which could have happened with the new scoring model. As @jonasfj said we are pinning on pedantic 1.8.0, and that does not have the omit_local_variable_types rule enabled. Do you have a package that is affected by this rule?

[isoos]

Sidenote:

So, it's been more than 8 months since this issue is open

This issue was opened on Dec 20, 2019. On Jan 13, 2020 we've applied the following PR on pub.dev that restricted the pedantic ruleset to 1.8.0, effectively making the issue resolved (as the penalty of the lint no longer applied). However, we forgot to close the issue.
dart-lang/pub-dev#3237

I think, given that we had holidays between the two date, the ~3-weeks response time doesn't seem to be that bad to warrant such a bad tone. Yes, we may have had a bug that resurfaced it with the new scoring, but the lint-issue was not active for 8 months.

[isoos]

I've got a local reproduction, and found the core issue:
https://github.com/dart-lang/pana/pull/764/files#diff-72c013e5d663c4116f43a3b6fe7b3ca3L196-R200

[ShivamArora]

We are where we are because picking a set of lints is hard.
There is value in having consistent lints applied, as it makes jumping from one package to another easier.
I think that striving for consistency is how we got where we are.

I think we picked pedantic a long time ago, because we didn't want to do a lot of bike shedding over which lints to enable/disable. Picking lints of often very subjective.

Currently, we don't even enforce the latest pedantic because it required too many packages to change.

Agreed.

That's the reason why people broke out over this controversial rule because with a change in pedantic rules it affected every package out there which used pedantic as a base.

I also agree with the point that it's hard to pick up which lints to enable/disable since this is very subjective.

I think the only reason people doesn't like this rule is it doesn't make complete sense why this rule is mandatory and that too for a language which talks about it's advantage being statically typed over javascript.

I don't think this rule brings an advantage to the table rather it only a couple of disadvantages which limits people to use types which are of paramount importance in a typed language.

I just wanted to introduce a suggestion and it's completely upto you guys how to handle this:

1. Use pedantic as the base since everyone is familiar with that and it saves you time to pick up / remove lints.
2. Maintain a list of lints which you can enforce or restrict on top of pedantic.

With the current solution, i.e. using an older version of pedantic you might be missing over some other suggested lints which are good.

But if you mark pedantic as just the base and have certain inclusive/exclusive lints on top of that, you get best of both just like everyone can maintain an analysis_options.yml file in their package.

[ShivamArora]

@ShivamArora: I think this may be actually a bug, which could have happened with the new scoring model. As @jonasfj said we are pinning on pedantic 1.8.0, and that does not have the omit_local_variable_types rule enabled. Do you have a package that is affected by this rule?

Yep. My package is being affected by this rule. However, for now I've just ignored the lint within the specific files.

[ShivamArora]

I think, given that we had holidays between the two date, the ~3-weeks response time doesn't seem to be that bad to warrant such a bad tone. Yes, we may have had a bug that resurfaced it with the new scoring, but the lint-issue was not active for 8 months.

I'm sorry if I went to harsh on what I said. But what frustrated me was the discussion over the thread at pedantic where I didn't find a sensible reply for why this rule might be helpful instead of seeing comments that it's being used at Google and we're fine with that.

To clear things out, I never meant to target you individually. My apologies if you thought that.

But the only reason, I mentioned you within my comment was I've earlier had discussions in other issue threads with you and I knew you respond quickly to people and your name was familiar to me as a member of the maintainers.

Moving on to that, I know this was reverted a while ago since I was affected earlier as well and probably I created a separate ticket as well either at pana or pedantic which I don't remember.

But what triggered me was with what note the issue at pedantic was closed (for which I was notified) and not the package score.

What I wanted is just a discussion, that respects the views of others rather than just people at Google which I couldn't find of in that thread.

And then I saw the references shared by @zathras , thanks to him for sharing those, which just creates more confusion about what Dart is showing off as their features and what's being restricted with this rule.

[zathras]

This issue was opened on Dec 20, 2019. On Jan 13, 2020 we've applied the following PR on pub.dev that restricted the pedantic ruleset to 1.8.0, effectively making the issue resolved (as the penalty of the lint no longer applied). However, we forgot to close the issue.

Restricting the ruleset to 1.8.0 does not effectively make the issue resolved, UNLESS there is some kind of future commitment.

Good engineers who want to build something that lasts are forward-looking.

[jonasfj]

The issue as it pertains to the omit_local_variable_types rule is resolved. I believe the regression that happened with the new scoring model will be resolved with #764, as it rolls out (hopefully tomorrow), packages should see analysis results change as they are analyzed again (this might take a few days).

Finding a good set of lints for pana is outside the scope of this issue. We do hope to find a better solution than simply deferring to an arbitrary version of pedantic -- it's no secret that this is not ideal :)

But when/if that happens is not something we generally commit to. We can only disappoint if we make any such promises. Who knows it might be hard to get a lot of people to agree about lints :)