Fixing deps for build_runner in flutter/packages broke Dart infra

[Piinks]

Yesterday we heard from the Dart team that use of the latest build_runner was being blocked on flutter/packages#10078

We landed that change to resolve the issue, but a few hours later heard the Dart team's infra was broken as a result:

flutter-analyze / flutter-analyze-trybot failures:

Analyzing go_router_builder...

warning - lib/src/type_helpers.dart:798:22 - 'formalParameters' is experimental and could be removed or changed at any time. - experimental_member_use
warning - lib/src/type_helpers.dart:801:22 - 'formalParameters' is experimental and could be removed or changed at any time. - experimental_member_use

2 issues found.

It looks like this bot is not hermetic and it can start failing without a particular reason in Dart SDK. It is very unfortunate that it is included into CQ and these failures would block Dart SDK changes from landing.

@stereotype441 diagnosed 🙏:

in versions of the analyzer prior to 8.2, we had some APIs that were marked as @experimental even though they weren't really experimental, and were intended to be supported for the long term. One of those APIs was the FunctionType.formalParameters getter. In dart-lang/sdk@a3bd2ad (which shipped in analyzer 8.2), I removed all those bogus @experimental annotations. Later, in dart-lang/sdk@533f833, I added a warning to the analyzer that fires if an @experimental API gets used unintentionally. This warning is important because it unblocks new language feature development work that is currently in progress.

The problem is that the flutter-analyze and flutter-analyze-try bots are linking the go_router_builder package to a version of the analyzer prior to 8.2, but they're analyzing that package using the bleeding edge analyzer. Which means that when go_router_builder tries to use the FunctionType.formalParameters getter, it gets flagged by the warning, even though it's actually safe (because in truth, that API isn't experimental at all; it's fully supported).

The failure just started happening because the uses of FunctionType.formalParameters just got added to go_router_builder in [the original linked PR above].

We resolved the issue by adding ignores in flutter/packages#10275
There was some confusion about which to apply where since we could not reproduce the upstream issue, which analyzer version the bot was using was not what others were using.

More info:

The Dart team has a luci bot (flutter-analyze-try) that tries to analyze each pending SDK CL against bleeding edge flutter, and fires if that produces failures. Its purpose is to prevent us from accidentally landing changes into the dart SDK that would break the dart/flutter integration. But that bot can equally easily be broken if a change gets landed into flutter that would break the dart/flutter integation (which is what happened this time). To detect that scenario prior to landing a flutter change, we would need a bot that tries to analyze each pending flutter PR using the bleeding edge dart SDK.

In retrospect, how could this have gone differently, starting with the blocking issue on build_runner? Could the subsequent issues have been detected earlier before the change landed?

[alexmarkov]

The situation when Dart changes or Flutter changes break bleeding edge combination of Dart x Flutter is not unique to analyzer. That's the reason why we have Dart-Flutter head-head bots - they give us an early heads-up and allow us to fix problems even before they are flagged by failed Dart->Flutter rolls. The unique problem with flutter-analyze bot is that for some reason it is included into Dart commit-queue, even for totally unrelated changes (it is a pre-submit check which blocks landing of CLs). This makes it highly urgent to fix breakages uncovered by this bot, so it looks like it is doing more harm (by blocking unrelated changes) than good. Maybe we can reduce pressure by making this bot post-submit or include it into pre-submit checks only if analyzer sources are modified.

/cc @iinozemtsev @athomas

[FMorschel]

I've already discussed this with @srawlins and @bwilkerson. Some lint/analyzer changes regarding diagnostics do need this bot, and we even have a Flutter customer one, but that one must be manually selected by reviewers and is therefore error-prone (see dart-lang/sdk#59869 (comment) where we forgot to run, and this unfortunately added a long timeout because the member working on that had to work on something else).

One idea that has already been discussed was about making these only trigger when making changes inside a specific folder. That way, we could say both the flutter-analyze and the Flutter customer tests could run only when they might be affected, and all other changes would be fine.

[stuartmorgan-g]

It looks like this bot is not hermetic

For some context on this, the test is currently (AFAIK) pretty much just a copy of the flutter/packages analysis test. The fact that it's not hermetic in flutter/packages is intentional-ish: we really want to be testing our packages the way our most of our users will actually experience them, which means we want the resolver to actually run. In theory, it would be ideal to pin every Dart dependency across the entire repo and then roll everything constantly; in practice I don't see a way to do that currently without adding a lot of gardener pain.

I think the tradeoff could easily be different for the Dart CI copy of these tests though; updating the pins should be much less CI-intensive and flake-intensive in a repo that is only running analyze, and also it's less critical that the pins always be current. As @alexmarkov mentioned above, the goal of the Dart version of the test is to (generally) avoid landing things in Dart that will eventually break the roll into flutter/packages, and the vast majority of the time I would expect that to be surfaced regardless of the versions of dependencies. There would be outlier cases like this one, but having a very rare case of something like this failing in the flutter/packages roller seems reasonable to me.