linter repo move readiness // planning

[pq]

🚧 (This is a living document and a work in progress. Watch this space for emerging details. And do provide feedback!) 🚧

Background

Costs. There are costs to having the linter source living externally and separate from the analyzer. In practice, the two are quite tightly coupled and not being able to make changes atomically across them (when an analyzer API changes or a new lint is added for example) creates extra work (e.g., otherwise needless analyzer package publications) and bookkeeping (e.g., TODOs or issues to track the implementation of quick fixes for new lints that cannot be landed until the new lint is available in the SDK). Lints are also very difficult to evolve as any downstream impact from changed semantics is only detected when a linter roll into the Dart SDK's DEPS is evaluated. Having the linter workflow external and unfamiliar to Dart team members has also (arguably) limited Dart team contributions.

Benefits. There are benefits too. Particularly in the early days of this project we've greatly benefited from the relative nimbleness of a workflow that favors external contributions, allows easy CI customization and enjoys the relative focus of a dedicated issue tracker.

Today. When the linter was growing rapidly and was a tool that was effectively adjacent to the core Dart SDK offerings, the benefits (IMO clearly) outweighed the costs. The landscape today is different though. The linter is effectively now itself a core offering, with lints a central part of the developer experience and various subsets endorsed by the Dart and Flutter teams and promoted as part of standard best practice (see package:lints).

Making a move. In light of this, we'd like to move the linter sources (and issue tracking) into the Dart SDK.

Plan

(WIP planning notes. Details to be filled in.)

Readiness. There are a bunch of steps to make the linter ready for a move.

Sources

Build. To make the source buildable in sdk/pkg, we'll need to:

• [x] #4412

To not lose the build check automation we enjoy from GitHub actions, we'll want to:

• [x] #4416

Testing. To make the source testable in the SDK build, we'll need to:

• [x] https://github.com/dart-lang/linter/issues/4404

(Nice to have) To speed up test runes we might:

• [x] https://github.com/dart-lang/linter/issues/3535

Documentation. We currently auto-generate lint docs here and host them on GitHub pages. We need:

• [x] #4460

We'll also need to:

• [x] update contributing guidelines and docs to integrate with an SDK development workflow

Process. We should tidy up stale process bits.

Notably, as we're no longer publishing the package, we should:

• [x] 🧹 https://github.com/dart-lang/linter/issues/4389

Post move. After moving a number of bits will need tidying up:

• [x] #4418

Issues

To get the issue tracker ready for migration we'll need to:

• [ ] https://github.com/dart-lang/linter/issues/4729
• [ ] align our issue priorities with SDK analyzer issue priorities
• [x] #4461

We might also:

• [ ] consider adding workflows to the SDK repo that take over our custom auto-labeling

Feedback and Thanks

Please provide feedback! A lot of thinking has gone into this and I'm happy to elaborate; just ask. 🙏

The quality of external contributions has been a bright light for this product (and for me personally). My sincere hope is that this move and the benefits of integration with our core source will not discourage contributions. On the contrary, I hope this will empower us to make the linter even better for all of us.

---

/fyi @a14n @athomas @asashour @bwilkerson @devoncarew @jacob314 @jcollins-g @parlough @srawlins @stereotype441 @whesse

[a14n]

From an external contributor perspective my main concern is that it's way easier to contribute to independent package compare to packages in the dart-sdk (mainly because of the tool chain and project structure are really different). For contributors used to the dart-sdk repo I think it will be almost the same (the other people will suffer :smile: - NB I don't know in which category I put myself).

TBH I would have prefer to make linter an analyzer plugin by working on an official analyzer plugin feature.

[stereotype441]

Thanks for writing this up, @pq. I think it's a good plan.

I want to acknowledge @a14n's concerns about how it's more difficult to for external contributors to contribute to the dart-sdk repo. The Gerrit code review tool is unfamiliar to external developers, as is the LUCI testing infrastructure used to test the SDK, and that's definitely an obstacle for external contributors.

A lot of the reason I think this move is still worth it, in spite of the downsides for external development, is because of my experience working on the null safety migration tool. Although the null safety migration tool was never in a separate repo from the rest of the SDK, when it began our intention was to publish it as its own pub package, and that caused a lot of the same issues to arise that the linter faces today: because of the tight coupling between the migration tool and the analyzer, often a trivial change to one package would require a corresponding change to the other, and that forced us to stage out the change over multiple commits, with a release of the analyzer codebase in between. The whole process often stretched out over the course of a week and required multiple people to coordinate.

A purist might say that we should have designed a stable API between the two packages. And that probably would have been the right approach if that API could have been small, or useful to a large number of other projects. But the fact of the matter was that the API surface area between the two projects was quite large, and the needs of the migration tool were so unique that chances of some other project needing to use the same API were very small. We simply couldn't justify the engineering expense of designing, maintaining, and evolving a stable API for use by just a single client.

When we finally decided to stop publishing the migration tool on pub, and simply ship it as part of the SDK, the API between it and the analyzer effectively became a private API, and we were able to clean it up and evolve it much more efficiently. And that created a significant benefit for our users, because the extra efficiency meant that we could finish the migration tool in time for the null safety release.

That's just my personal anecdote, of course. But based on that experience, I suspect that once the linter is inside the SDK repo, we'll find that a lot of clean-ups and refactors that would previously have been unfeasiable will suddenly become easy, and as a result we'll be able to clean things up in a way that makes it significantly easier to develop lints, both for internal and external developers. It might be the case that for some external developers, the extra development efficiency will never feel worth the cost, but I think that in aggregate, averaged over all the developers that touch the linter, and particularly for Phil, it will be worth it.

[pq]

Thanks so much for the response @a14n. You echo my concerns exactly. I'd love to better understand the friction for contribution to the Dart SDK proper and do whatever we can to reduce it (at least for the linter). It would be sad for the project to lose the valuable contributions from folks like you.

/fyi @kevmoo @mit-mit @devoncarew @natebosch @athomas (among others) who may have insight into any plans to make Dart SDK contributions more approachable.

@asashour: as someone who has made a lot of contributions here and the SDK, it would be interesting to get your 2 cents.

[whesse]

We are ready to move the linter repository into the sdk repository. Because we also don't want to break Flutter, I am proposing doing this as a non-breaking change, by adding the linter repo into the SDK without removing the copy in third_party, downloaded by DEPS.

This is because changes to DEPS downloads are the hardest to sync into the Flutter engine build, and because people's local checkouts will continue to have the copy in third_party until they run "gclient sync -D" or manually delete it. The steps I have in mind, and the sequence, is:

• [x] Change the (sdk)/tools/generate_package_config.dart to recognize the linter repository in both the old and new locations, and to use the new location if it exists. Do not print a warning about the old location being present, at this time.
• [x] Take the selected commit in dart-lang/linter and run the issue link expansion on it locally, to create a new linter history with issue links expanded to dart-sdk/linter(hash symbol)1234.
• [x] Merge the linter history into (sdk)/pkg/linter using the git subtree merge command.
• [x] Push the merge commit to the sdk repo.
• [x] Change Flutter engine DEPS to drop the third_party copy of linter, verify that it goes through to the Framework without errors.
• [x] Change Dart SDK DEPS to drop the third_party copy of linter.
• [x] Add a warning to generate_package_config, telling users to delete the copy of linter in third_party.
• [x] Remove the special-case handling for linter and use the standard error message of generate_package_config when duplicate packages are found.

Each of these steps should be tested as a tryjob, also on Flutter engine (via monorepo).

[whesse]

The linter repo does not follow the dart formatting rules used in the SDK repo in the test data. We should also land a change to the PRESUBMIT.py to exclude this directory from the formatting tests.

[whesse]

Various unit tests in the linter repo have runtime failures or analysis failures when run as part of package testing in the SDK. Some runtime failures are caused by tests that assume the current working directory is the package root.

These can either be fixed in the linter repo before moving it, or marked as approved failures, issues filed, and then fixed after the merge happens. The failures can be seen on the CL https://dart-review.googlesource.com/c/sdk/+/321722

[bwilkerson]

The steps you outlined look good to me, but @pq is more likely than I am to spot missing steps.

I will note that the test data directory is in the process of being removed (currently on hold until after the move) in favor of the reflective test runner style of tests used by the other analyzer packages. Once it has been removed then we won't need to worry about formatting, analysis failures, etc. in those files. If it would simplify the process enough we could think about how much work remains and whether it makes sense to finish that arc of work before the move.

[srawlins]

If it would simplify the process enough we could think about how much work remains and whether it makes sense to finish that arc of work before the move.

At my count there are around 110 data files remaining; it would take a few weeks time.

[whesse]

@pq has already picked the commit to merge, we are ready to proceed with that work immediately. I think it makes sense to fix the problems after the merge, and to mark the test failures as approved. The flutter trybot failures seem unrelated - the builds went through correctly, and the failures seem to have nothing to do with linter.

[pq]

The steps look good to me!

I'm with @whesse, and am good with marking tests as failing. I'll tackle the cleanup promptly after the integration.

As for the test_data formatting directory accommodation, I think we can safely ignore formatting for now and remove the exception once we've migrated all the remaining test_data style tests.

[whesse]

The migration has happened. No further steps on the checklist can be taken until the SDK rolls into Flutter engine. Dart SDK builds are now using the new copy of the repo.

Steps that can be taken now are to fix the broken unit tests, and remove the unformatted files.

[pq]

With the linter now settling into it's new home in the SDK, I'm going to close this one out. Follow-up actions will get tracked in their own issues.

Thanks to all for the group effort! 🎉