Linter: identify place to host canonical lints

[pq]

Let the bike-shedding begin!

To start, here are a few proposals.

Proposal 1 : (new) package:canonical

Name

package:canonical (https://pub.dev/packages?q=canonical 👍 )

Structure

lib/
  analysis_options.yaml
  canonical_1.0.0.yaml
  recommend/
    canonical_1.0.0.yaml
    analysis_options.yaml

A few details:

• analysis_options.yaml just include:s latest the same way pedantic currently does
• lib/recommend/canonical_1.0.0.yaml has an include: of the root (scoring) lints and adds the recommended ones to it

Sample Uses

To get the latest scoring lints:

include: package:canonical/analysis_options.yaml

or to pin to a version:

include: package:canonical/analysis_options.1.0.0.yaml

Turning the dial up to the recommended lints looks like:

include: package:canonical/recommend/analysis_options.yaml
include: package:canonical/recommend/analysis_options.1.0.0.yaml

Proposal 2: package:pedantic

Alternatively, we could use the existing package:pedantic. (Cheers @munificent for this one.)

Structure

lib/
  analysis_options.yaml
  pedantic_1.0.0.yaml
  pub/
    pedantic_1.0.0.yaml
    analysis_options.yaml
  recommend/
    pedantic_1.0.0.yaml
    analysis_options.yaml

In this case the "root" one would probably have to remain the current google style.

Sample Uses

To get google style:

include: package:pedantic/analysis_options.yaml
include: package:pedantic/analysis_options.1.0.0.yaml

Pub scoring:

include: package:pedantic/pub/analysis_options.yaml
include: package:pedantic/pub/analysis_options.1.0.0.yaml

Recommended:

include: package:pedantic/recommend/analysis_options.yaml
include: package:pedantic/recommend/analysis_options.1.0.0.yaml

/fyi @bwilkerson @munificent @natebosch @jakemac53 @davidmorgan @mit-mit @lrhn for input

[natebosch]

I think this should not be a pub package, this should be something shipped in the SDK. I don't see any way that we can enable scoring lints by default (even if we don't do it to start with) if they are defined in a package.

[jakemac53]

I think this should not be a pub package, this should be something shipped in the SDK. I don't see any way that we can enable scoring lints by default (even if we don't do it to start with) if they are defined in a package.

We could make it so that pub lish warns if you don't add the scoring lints? Regular apps and non-published packages though would be harder to point at these lints to be sure.

Putting it in the SDK is not without downsides either - it is harder to evolve, adds more friction to updating your sdk, and makes it so users on old sdks don't see the updated scoring lits. Users on the pre-release sdks will also see a different set than pub.dev sees (assuming it uses stable).

[jakemac53]

As far as the original proposal - I would be somewhat inclined to move the top level analysis_options.yaml under pub_score directory? It isn't clear to me that those are the pub scoring lints with the current structure.

I would also probably go for recommended instead of recommend.

[munificent]

If we put it in the SDK, that means the set of lints can spontaneously change on users when they upgrade SDKs. If we put it in a package, it gets pinned in their lockfile and upgraded when they choose.

[natebosch]

it gets pinned in their lockfile and upgraded when they choose.

If this is a goal it excludes showing lints to folks without config. I think that work we do now which encourages _more_ config today it's a step in the wrong direction - early opt-in is one thing but _additional_ config like a package dep that is temporary feels like noise and debt to clean up later.

I think it's important that we can provide help and tips to users even if they didn't start their project with stagehand or know to add some magic config to their analysis_options.yaml and pubspec.yaml.

[natebosch]

set of lints can spontaneously change on users when they upgrade SDKs

We could choose which lints to show based on language version. That's a mechanism that would be very likely to be useful down the road anyway.

[jakemac53]

If we put it in the SDK, that means the set of lints can spontaneously change on users when they upgrade SDKs. If we put it in a package, it gets pinned in their lockfile and upgraded when they choose.

Fwiw, I think this is actually a negative for the package option. At least when it comes to scoring related lints I think we would always want all users to see exactly what pub will be scoring on _right now_, and not have to do anything special to update. It is possibly more likely that shipping it with the sdk would achieve that more consistently.

We also don't have a built in way of updating a single package, so upgrading lints becomes tied in with updating _all dependencies_. Granted the sdk option has a similar problem of it being tied to the sdk update.

[NatoBoram]

What are canonical lints? Which rules are included? Could they be added to effective_dart?

[mit-mit]

If we put it in the SDK, that means the set of lints can spontaneously change on users when they upgrade SDKs. If we put it in a package, it gets pinned in their lockfile and upgraded when they choose.

For scoring lints, I think we have to move towards a breaking change like notification process anyway, given how impactful they are to package authors.

[davidmorgan]

FWIW I suggest package:analysis_options if we end up with a new package.

Re: pinning, having versioned yaml files within the package allows people to choose between pinning and not pinning, even if it auto updates with the SDK.

[lrhn]

If we distribute the package with the SDK, this will be the first package that comes with the SDK. We'll need new resolution logic in the Pub tool (it's a package which is not fetched from pub), and we'll need to decide whether to move it into the pub cache, or whether to link directly to it in the SDK repo (I'd probably go for the former).

And if someone wants to use a newer the package with an older SDK, they're completely out of luck.

I think I'd prefer to just publish it as a normal package. I don't want to mix it with package:pedantic.

We can make Pub recommend a pub get when it sees that your analysis options package is not up-to-date (older than what the Pub tool itself uses).

I'm probably fine with having the analyzer use a default set of lints (which is the newest recommended set) if there is no lint configuration at all, or only lint-x :ignore configurations. If there is any positive lint configured, then drop the defaults and use the configuration.

[mit-mit]

Pub supports SDK constraints (Flutter uses them extensively), so it's not very much work if we wanted to.

dependencies:
  canonical:
    sdk: dart

Regarding versioning, we could still allow referencing older versions explicitly, e.g. something like this for selecting the 2.12 scoring lints?

analysis_options.yaml:

include: package:canonical/scoring/2.12/analysis_options.yaml

[lrhn]

With the issues we've had with Flutter's version-pinned packages, I'm very wary of doing anything remotely similar.

It means that if you want a newer lint set, you have to upgrade your SDK. It's probably just a dev-dependency, so it shouldn't affect users of your package - except that you'll have to update your SDK-min-constraint for it, and then so will your users in order to use your package. (Nothing particularly new here, that goes for all dev dependencies).

[natebosch]

If we distribute the package with the SDK, this will be the _first_ package that comes with the SDK.

I don't think it should be a package at all. I don't want the users to need anything in their pubspec.yaml in order to use this. In the long term I'd like them to not need an analysis_options.yaml file at all in order to use this. In the shorter term it is OK to need a line in analysis_options.yaml. Even in the short term I think it's a step in the wrong direction to need any configuration in pubspec.yaml.

[bwilkerson]

In the long term I'd like them to not need an analysis_options.yaml file at all in order to use this.

Are you proposing that these checks be opted in by default, or do you have another mechanism in mind for opting in to them?

[natebosch]

Are you proposing that these checks be opted in by default

In the long term, yes.

[munificent]

We also don't have a built in way of updating a single package

$ pub upgrade foo

[jakemac53]

$ pub upgrade foo ...... I had no idea that existed 😆

[bwilkerson]

I'll just point out that currently the difference between a hint and a lint is that hints are opt-in by default while lints are opt-out by default. If we want these checks to be opt-in then we should consider converting them to be hints.

[davidmorgan]

Thanks Brian :)

What would a path for lints to be promoted to hints look like? Something like:

• Hint created with the same logic / message / code
• Linter notices if it's running in analyzer with the hint implemented and turns the lint off? This seems easy enough to do and good for performance as well as usability--we don't want hint+lint to ever fire together.
• Hint to remove the lint from analysis_options.yaml? But this would be annoying if you're using an imported lint set... maybe deprecate+remove lints in batches as a separate process?

For lints where this makes sense, it does seem a clear win for users. Only downside I see is that it's more work on our side :)

[bwilkerson]

What would a path for lints to be promoted to hints look like?

Given that we control both the implementation of hints and the version of the linter being run, we can create a version of the linter that no longer supports the lint and pull that version into the SDK in the same commit that that adds the corresponding hint. That way it's an atomic change and we don't need to worry about whether they could ever fire at the same time.

We already have a diagnostic telling users when they're referencing an undefined lint, but it's non-fatal so it won't impact users if they choose to delay cleaning up their analysis options file. We do not, however, produce diagnostics for issues in included analysis options files under the assumption that most users won't own the included file, and that if they do own it they'll see the diagnostic when they analyze the package that contain it.

I think it's fairly clean for most users. The biggest impact is that updating Dart / Flutter might cause some new diagnostics to be produced. While that's not ideal, we've never worried much about it in the past because we generally only add hints when they signal real errors in the code. I assume that it would be the same here.

[bwilkerson]

Oh, and the amount of work required on our side is probably less than it would take to create a way to enable lints by default or to be able to reference an analysis options file that isn't in a package, so I think this is also a win in terms of implementation time.

[pq]

For lints where this makes sense, it does seem a clear win for users.

💯

2418

[goderbauer]

If this ends up being a package: There was some discussion over on https://github.com/flutter/flutter/issues/78432#issuecomment-802309066 whether we need double-versioning (i.e. the package itself has a version and then each version includes a set of versioned analyzer_options.yaml files as suggested in the very first message of this thread). The conclusion there was that one version - the version of the package - would be enough to simplify things.

[goderbauer]

Also: In flutter the idea is that flutter create would set this all up. It would generate whatever is needed to enable the canonical lints.

[mit-mit]

From recent discussion between @devoncarew and myself, we'd like to not store the lints in a published package. Doing so has a number of user experience issues:

1. No linting available for bare Dart files (e.g. mkdir myapp; cd myapp; nano hello.dart; dart analyze)
2. No way of upgrading all projects to a new set of lints when upgrading to a new major SDK (similar to what Flutter does today)
3. No way of ensuring consistency between the version being run with locally and the version pub will run scoring with

Thus, we'd like to suggest we store the lints in the Dart SDK, for example in dart-lang/sdk/pkg/lints and that we do not publish this package to pub.

With that, I'd like to still support manually selecting an older lint set, so I suggest we store the files inside /pkg/lints with a new file for every major SDK version, similar to pedantic, e.g. for the Dart 2.13 release:

/pkg/lints/lib/analysis_options.2.10.yaml
/pkg/lints/lib/analysis_options.2.12.yaml
/pkg/lints/lib/analysis_options.2.13.yaml
/pkg/lints/lib/analysis_options.yaml: just contains an import of /pkg/lints/lib/analysis_options.2.13.yaml

[bwilkerson]

... _and_ that we do not publish this package to pub.

How would a user import from one of these analysis options files if the package isn't published?

[jakemac53]

No way of upgrading all projects to a new set of lints when upgrading to a new major SDK (similar to what Flutter does today)

It wouldn't be force updated, but a pub upgrade would update it. That has some advantages in that you can update it more frequently than SDK rolls as well.

No way of ensuring consistency between the version being run with locally and the version pub will run scoring with

Imo the proposed solution doesn't solve this, it actually makes it worse. If this is in a package it allows users of older sdks to use the newer lints (what pub is scoring with). If shipped with the SDK then users on older sdks have no way of getting the new lints. The pub lish command could warn if it sees you have an outdated version as well.

[devoncarew]

For some clarity, what we'd discussed (off-line) was something like:

include: dart:core/lints/core.yaml

And the implementation file(s) in the sdk would live in <dart-sdk>/lib/core/lints/core.yaml.

In other words, we'd use dart: scheme URIs to refer to the sdk vended analysis options files.

[natebosch]

IMO the recommended lints should be on by default and the way to back out of them should be something like

analyzer:
  use_recommended_lints: false

and we can allow opting out of scoring lints which folks might want to do if they aren't publishing.

analyzer:
  use_recommended_lints: false
  use_pub_score_lints: false

[goderbauer]

No way of upgrading all projects to a new set of lints when upgrading to a new major SDK (similar to what Flutter does today)

I am actually seeing this as a downside (and I think it has been one of the reasons we haven't updated the set of enforced lints in Flutter for more than a year). If the set of enforced lints is determined by my (Flutter or Dart) SDK version I am going to experience more breaking changes in my project if I am upgrading to a new SDK that introduces additional lints (assuming that I have CI running to ensure that my project is analyzer clean). If the lints live in a separately versioned package, upgrading to a new SDK is less breaking and I can migrate to the latest set of lints at my leisure by bumping the lint package version in my pubspec.

Managing the lints in the Dart SDK will presumably also make rolling new Dart SDK versions into Flutter more painful because newly introduced lints may break some of our customer tests (contributed via https://github.com/flutter/tests). Decoupling means that new SDK versions can roll in independently from customers upgrading to the latest lint sets.

[pq]

If I understand the proposal right, you can still pin lints to a major version as there will be options files corresponding to each major version. Under such a scheme, when bumping to SDK 2.14 (say) you could pin yourself to the 2.13 lints by including a file like dart:core/lints/core_2_13.yaml or similar (rather than dart:core/lints/core.yaml which is always the latest) .

[mit-mit]

Correct: My goal is to have the optimal defaults, but still support easy customization should you not want these defaults. One such customization is to pin to a specific / older version.

[pq]

Resolved. See: https://github.com/dart-lang/lints