Code health: clean up Expander.java interface #11221
Labels
P3
We're not considering working on this, but happy to review a PR. (No assignee)
stale
Issues or PRs that are stale (no activity for 30 days)
team-Configurability
platforms, toolchains, cquery, select(), config transitions
type: process
Expander is responsible for Bazel's make variable resolution. Its Java API is a bit delicate and could use some refactoring.
Copied out from pending change https://bazel-review.googlesource.com/c/bazel/+/134315/3/src/main/java/com/google/devtools/build/lib/analysis/Expander.java#68. :
// It's important that we inherit lookedUpVariables from old instances. RuleContext keeps a
// reference to the Expander it gives to rule implementations. But rule implementations may
// clone that with different settings (see the methods below that instantiate new instances).
// We have to guarantee whatever they look up in this instance is also recorded in the original
// instance RuleContext has.
//
// This is unfortunately fragile. A better approach would be to integrate a Builder or more
// customizable constructor into RuleContext so rule implementations can get exactly the
// instance they need from RuleContext's API. Then we can eliminate all other methods that
// construct new instances. Unfortunately there are too many calls in the code base to the
// current approach, so that's a non-trivial cleanup.
//
// Another approach could be to have rule implementations, not RuleContext, be responsible for
// processing lookedUpVariables to report which Make variables they consume. They can do this
// with whatever Expander instance they wish. This is even more fragile: every rule
// rule implementation would have to remember to do this (probably through a RuleContext call),
// to maintain accurate results. That's an unrealistic burden.
//
// TODO: Eliminate all methods that construct an Expander out of an existing Expander.
The text was updated successfully, but these errors were encountered: