From 2b9a2e79d57fbae57337f2afdabb44bdad7d33f4 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 17 Oct 2024 08:13:08 -0700 Subject: [PATCH] Fix implicit input file creation for symbolic macros Previously we weren't creating input files for labels used in the attribute of a top-level symbolic macro. This meant that if you wrote ``` my_macro( name = "foo", srcs = ["input.txt"], ) ``` you'd get an error message claiming that input.txt is not declared and requires an `exports_files()`. (The usages of input.txt by targets declared inside of foo don't count, because implicit input file creation only works for usages of labels outside of symbolic macro bodies.) This CL fixes this behavior, refactors the relevant logic in Package.java, and adds more test coverage. Package.java: - Factor the big for loop from `beforeBuild()` to `createAssumedInputFiles()`. Leave the original for loop in place for test suite accumulation, and pull the `if (discover...)` out of the new call. - Factor the meat of the logic into a helper, `maybeCreateAssumedInputFile()`, so we can reuse it for both rules and macros (previously there was no loop over references in macros). - Add tedious javadoc to this tedious logic. SymbolicMacroTest.java - Add explanatory comment to `macroCanReferToInputFile()`. - Expand `macroCannotForceCreationOfImplicitInputFileOnItsOwn()` to check that the target is still not created when the inner usage is in an attr of a MacroInstance rather than a Rule. PackageFactoryTest.java - Expand comment in `testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespace()`, include coverage for when the reference comes from within a target or submacro inside the symbolic macro itself. - New test case for the behavior of this CL, `..._macroInstantiationCanForceImplicitCreationOfInputFile()`. - Drive-by: Add a test case for when a badly named target in a symbolic macro clashes with an implicitly created input file. This behavior may be affected by lazy macro evaluation in the future. Confirmed that this CL works for the case I originally discovered it on while writing examples documentation. Fixes #24007. PiperOrigin-RevId: 686919555 Change-Id: I6313070f76456c0b0b5d5458ca35c89d1d6da33b --- .../devtools/build/lib/packages/Package.java | 149 ++++++++++++------ .../build/lib/analysis/SymbolicMacroTest.java | 29 +++- .../lib/packages/PackageFactoryTest.java | 117 +++++++++++++- 3 files changed, 238 insertions(+), 57 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 63bb4de6f73b27..864f9432b4b61f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -1525,6 +1525,103 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru } } + /** + * Creates and returns input files for targets that have been referenced but not explicitly + * declared in this package. + * + *

Precisely: If L is a label that points within the current package, and L appears in a + * label-typed attribute of some declaration (target or symbolic macro) D in this package, then + * we create an {@code InputFile} corresponding to L and return it in this map (keyed by its + * name), provided that all of the following are true: + * + *

    + *
  1. The package does not otherwise declare a target for L. + *
  2. D is not itself declared inside a symbolic macro. + *
  3. L is not within the namespace of any symbolic macro in the package. + *
+ * + * The second condition ensures that we can know all implicitly created input files without + * having to evaluate any symbolic macros. The third condition ensures that we don't need to + * expand a symbolic macro to decide whether it defines a target that conflicts with an + * implicitly created input file (except for the case where the target doesn't satisfy the + * macro's naming requirements, in which case it would be unusable anyway). + */ + private static Map createAssumedInputFiles( + Package pkg, TargetRecorder recorder, boolean noImplicitFileExport) { + Map implicitlyCreatedInputFiles = new HashMap<>(); + + for (Rule rule : recorder.getRules()) { + if (!recorder.isRuleCreatedInMacro(rule)) { + for (Label label : recorder.getRuleLabels(rule)) { + maybeCreateAssumedInputFile( + implicitlyCreatedInputFiles, + pkg, + recorder, + noImplicitFileExport, + label, + rule.getLocation()); + } + } + } + + for (MacroInstance macro : recorder.getMacroMap().values()) { + if (macro.getParent() == null) { + macro.visitExplicitAttributeLabels( + label -> + maybeCreateAssumedInputFile( + implicitlyCreatedInputFiles, + pkg, + recorder, + noImplicitFileExport, + label, + // TODO(bazel-team): We don't save a MacroInstance's location information yet, + // but when we do, use that here. + Location.BUILTIN)); + } + } + + return implicitlyCreatedInputFiles; + } + + /** + * Adds an implicitly created input file to the given map if the label points within the current + * package, there is no existing target for that label, and the label does not lie within any + * macro's namespace. + */ + private static void maybeCreateAssumedInputFile( + Map implicitlyCreatedInputFiles, + Package pkg, + TargetRecorder recorder, + boolean noImplicitFileExport, + Label label, + Location loc) { + String name = label.getName(); + if (!label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) { + return; + } + if (recorder.getTargetMap().containsKey(name) + || implicitlyCreatedInputFiles.containsKey(name)) { + return; + } + // TODO(#19922): This conflict check is quadratic complexity -- the number of candidate inputs + // to create times the number of macros in the package (top-level or nested). We can optimize + // by only checking against top-level macros, since child macro namespaces are contained + // within their parents' namespace. We can also use a trie data structure to zoom in on the + // relevant conflicting macro if it exists, since you can't be in a macro's namespace unless + // you suffix its name (at least, under current namespacing rules). + for (MacroInstance macro : recorder.getMacroMap().values()) { + if (TargetRecorder.nameIsWithinMacroNamespace(name, macro.getName())) { + return; + } + } + + implicitlyCreatedInputFiles.put( + name, + noImplicitFileExport + ? new PrivateVisibilityInputFile(pkg, label, loc) + : new InputFile(pkg, label, loc)); + } + @CanIgnoreReturnValue private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPackageException { // For correct semantics, we refuse to build a package that has declared symbolic macros that @@ -1573,58 +1670,18 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack // Clear tests before discovering them again in order to keep this method idempotent - // otherwise we may double-count tests if we're called twice due to a skyframe restart, etc. testSuiteImplicitTestsAccumulator.clearAccumulatedTests(); - - Map newInputFiles = new HashMap<>(); for (Rule rule : recorder.getRules()) { - if (discoverAssumedInputFiles) { - // Labels mentioned by a rule that refer to an unknown target in the current package are - // assumed to be InputFiles, unless they overlap a namespace owned by a macro. Create - // these InputFiles now. But don't do this for rules created within a symbolic macro, - // since we don't want the evaluation of the macro to affect the semantics of whether or - // not this target was created (i.e. all implicitly created files are knowable without - // necessarily evaluating symbolic macros). - if (recorder.isRuleCreatedInMacro(rule)) { - continue; - } - // We use a temporary map, newInputFiles, to avoid concurrent modification to this.targets - // while iterating (via getRules() above). - List