tree 9e4eb82e2e662fc574abee929049edb285512f6d
parent 0ea6076bb72eb9d78309b121a893593fd52091d6
author brandjon <brandjon@google.com> 1591644769 -0700
committer Copybara-Service <copybara-worker@google.com> 1591644853 -0700

Make BuildViewTestCase's use of custom RuleClassProviders and bzl inlining less error prone

This refactoring aims to convert the hours I wasted into saved time for somebody else.

Problem:

BuildViewTestCase allows you to supply an alternative RuleClassProvider by overriding getRuleClassProvider(). Although named as an accessor ("get..."), most overrides actually construct a new RuleClassProvider inside the implementation. This is safe so long as the method is only called once, in initializeSkyframeExecutor().

However, this interracts poorly with another, less common paradigm: Testing of BzlLoadFunction inlining. This is done in StarlarkIntegrationTest by defining a @Before hook (initializeLookupFunctions()) that grabs the current Skyframe executor's PackageFunction and tells it to use a newly constructed BzlLoadFunction -- a BzlLoadFunction that obtains its RuleClassProvider by... you guessed it, calling getRuleClassProvider(). Yet this too is safe, so long as the default implementation of getRuleClassProvider() is used, which memoizes its result deep inside the AnalysisMock.

But combining these two features together leads to disaster. You end up with bzl loading machinery whose native rule definitions (`native.cc_library`, etc.) are cloned versions of those in the rest of the Skyframe machinery. This can lead to infuriatingly subtle failures down the line.

Solution:

BuildViewTestCase's getRuleClassProvider() is renamed to createRuleClassProvider() to make it clear that it is not freely repeatable. A true accessor, getRuleClassProvider(), is introduced to take its place and declared `final`.

initializeLookupFunctions() is factored into BuildViewTestCase. Subclasses can simply override usesInliningBzlLoadFunction() to request inlining.

Thanks to shreyax@ for the debugging assistance.

Cleanup work on way toward #11437.

RELNOTES: None
PiperOrigin-RevId: 315329662