tree 0f6bef88b22436fa048ce2519580c8c3786b19e7
parent 0a75645f03fcbdb14c6dfa54206d23e02e7edbd6
author gregce <gregce@google.com> 1585689090 -0700
committer Copybara-Service <copybara-worker@google.com> 1585689183 -0700

Make StarlarkDefinedConfigTransition immutable.

Generally speaking, it's not safe for config transitions to keep state because
a transition instance may be shared across multiple rule instances. Rules should
not be able to affect each other through shared transitions.

This particular case removes an event handler that stores errors (like trying
to load invalid build settings) which the configuration machinery replays later
to communicate to the user. The risk trajectory is that a transition that fails
on a badly formed rule might repeat the same error for a well-formed rule
becuase of the shared state.

The most likely user-visible output of this problem is non-deterministic build
errors on rules using Starlark build settings and transitions. i.e. repeat the
same build twice and get different results.

This is a huge change (apologies), but conceptually straightforward. We simply
add an EventHandler parameter to ConfigurationTransition.apply to provide an
alternative place to record errors. This lifts StarlarkDefinedConfigTransition
of the burden to define its own.

Most of the hugeness in this change is that the ConfigurationTransition
interface is used in many places, so we have to pass / define this parameter
everywhere. A less invavise alternative could be to use Java interface defaults,
but I think the current approach is a better API (and the pain is limited just
to this change: there's no real extra API burden as a result).

Testing note: I'm impressed that this change only broke one test:
StarlarkRuleTransitionProviderTest#testAliasedBuildSetting, and for
straightforward reasons. See new comments in ConfigurationResolver for details.

PiperOrigin-RevId: 304043250