dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.2k stars 1.57k forks source link

Add support for an allow list of deprecated API usage #51984

Open natebosch opened 1 year ago

natebosch commented 1 year ago

Flutter CI fails for any diagnostics, including usage of deprecated APIs. In practice this means that APIs from packages used by any flutter code, and APIs in the SDK cannot be marked deprecated until after flutter usage has been cleaned up. This holds us back from sending a valuable signal about the deprecation to the ecosystem, and extends the critical path for making changes. Adding // ignore comments is noisy in the diff, and high in toil. It's also not easy to find the set of usage that needs to be cleaned up before the deprecated API is deleted.

If we can find some way to refer to the usage of an API in a way that is safe against changes to the surrounding code (we can't refer by line and column because unrelated changes could shift the code around) then we could generate an allow list during a package or SDK roll. The allow list would serve as a list of tech debt issues to be cleaned up.

Plausibly the flutter auto roller could use the analyzer to generate this list, and potentially file an issue when it needs to add new entries.

natebosch commented 1 year ago

For instance, the flutter packages repo pins Dart owned dependencies to single versions specifically because deprecated APIs break CI.

https://github.com/flutter/packages/pull/3544#issuecomment-1483106845

leafpetersen commented 1 year ago

Does it need to be per usage? It seems easier to just to ignore an entire deprecation across the repo. It doesn't stop backsliding, but I think the expectation around this would need to be for fairly prompt cleanup anyway. Also, how much of the pain here is around the difficulty of testing changes vs difficulty of fixing changes?

natebosch commented 1 year ago

That's true, if the cleanup happens quick enough we don't need to make the allow list be per usage.

how much of the pain here is around the difficulty of testing changes vs difficulty of fixing changes?

The difficulty comes from:

leafpetersen commented 1 year ago

The difficulty comes from:

  • often needing extra commits - currently we can never deprecate any API until some commit after the replacement is available.

Roughly speaking, this part I would describe as "atomicity". Currently, the roll into flutter is atomic, so you can't make the deprecation atomic (you have to split the deprecations into two pieces each of which can be arranged to roll into flutter atomically). This proposal would flip this by splitting on the flutter end: the deprecation could happen atomically, and the fixing flutter part gets split into two stages.

  • getting commits rolled back because we didn't to find some usage in flutter (@lrhn has experienced this)

This is difficulty of testing, and feels like something we should 100% fix regardless. Why does this not get caught by our try bots? It seems to me that at least for things with static diagnostics, we should be able to get 100% coverage on this.

keertip commented 1 year ago

This issue involves more than just the analyzer, we should look at putting in place a workflow to allow for the deprecation.

@itsjustkevin , is this something you can drive?

leafpetersen commented 1 year ago

Note that I'm not sure there's consensus on using such a mechanism in flutter, so unless we have other use cases in mind, I'd suggest holding off on doing actual work on this. cc @Hixie

Hixie commented 1 year ago

I don't really understand why migrating people before deprecating is a burden. In general I find that it's a good thing, because it lets you test things in a production environment before you've committed to deprecating the old feature. Sure, sometimes PRs in flight land new uses of the feature after you do the roll, but the alternative is putting the burden on the people writing those PRs (since now they suddenly find that their code is triggering a new deprecation lint, and they had no warning of it), and that doesn't seem any better.

I've deprecated some pretty core APIs that get used a LOT and even with migrating google3, and having to spend a few weeks playing the wack-a-mole across multiple repos, I didn't think the burden was that bad. If anything, it was good for me to feel the pain because it is proportional to the pain I'm putting on our developers.