Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request to Merge #4

Open
wants to merge 1 commit into
base: parent2b9a2
Choose a base branch
from
Open

Request to Merge #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 103 additions & 46 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>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:
*
* <ol>
* <li>The package does not otherwise declare a target for L.
* <li>D is not itself declared inside a symbolic macro.
* <li>L is not within the namespace of any symbolic macro in the package.
* </ol>
*
* 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<String, InputFile> createAssumedInputFiles(
Package pkg, TargetRecorder recorder, boolean noImplicitFileExport) {
Map<String, InputFile> 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<String, InputFile> 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
Expand Down Expand Up @@ -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<String, InputFile> 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<Label> labels = recorder.getRuleLabels(rule);
for (Label label : labels) {
String name = label.getName();
if (label.getPackageIdentifier().equals(metadata.packageIdentifier())
&& !recorder.getTargetMap().containsKey(name)
&& !newInputFiles.containsKey(name)) {
// Check for collision with a macro namespace. Currently this is a linear loop over
// all symbolic macros in the package.
// TODO(#19922): This is quadratic complexity, optimize with a trie or similar if
// needed.
boolean macroConflictsFound = false;
for (MacroInstance macro : recorder.getMacroMap().values()) {
macroConflictsFound |=
TargetRecorder.nameIsWithinMacroNamespace(name, macro.getName());
}
if (!macroConflictsFound) {
Location loc = rule.getLocation();
newInputFiles.put(
name,
// Targets added this way are not in any macro, so
// copyAppendingCurrentMacroLocation() munging isn't applicable.
noImplicitFileExport
? new PrivateVisibilityInputFile(pkg, label, loc)
: new InputFile(pkg, label, loc));
}
}
}
}

testSuiteImplicitTestsAccumulator.processRule(rule);
}

// Make sure all accumulated values are sorted for determinism.
testSuiteImplicitTestsAccumulator.sortTests();

for (InputFile file : newInputFiles.values()) {
recorder.addInputFileUnchecked(file);
if (discoverAssumedInputFiles) {
Map<String, InputFile> newInputFiles =
createAssumedInputFiles(pkg, recorder, noImplicitFileExport);
for (InputFile file : newInputFiles.values()) {
recorder.addInputFileUnchecked(file);
}
}

return this;
Expand Down
29 changes: 24 additions & 5 deletions src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,13 @@ public void macroCanReferToInputFile() throws Exception {
def _impl(name, visibility):
native.cc_library(
name = name,
srcs = ["explicit_input.cc", "implicit_input.cc"],
srcs = [
"explicit_input.cc",
# This usage does not cause implicit_input.cc to be created since we're inside
# a symbolic macro. We force the input's creation by referring to it from bar
# in the BUILD file.
"implicit_input.cc",
],
)
my_macro = macro(implementation=_impl)
""");
Expand All @@ -419,11 +425,20 @@ public void macroCannotForceCreationOfImplicitInputFileOnItsOwn() throws Excepti
scratch.file(
"pkg/foo.bzl",
"""
def _sub_impl(name, visibility):
native.cc_library(
name = name + "_target",
srcs = ["implicit_input.cc"],
)

my_submacro = macro(implementation=_sub_impl)

def _impl(name, visibility):
native.cc_library(
name = name,
name = name + "_target",
srcs = ["implicit_input.cc"],
)
my_submacro(name = name + "_submacro")
my_macro = macro(implementation=_impl)
""");
scratch.file(
Expand All @@ -434,10 +449,14 @@ def _impl(name, visibility):
""");

Package pkg = getPackage("pkg");
// Confirm that implicit_input.cc is not a target of the package.
// It'd be an execution time error to build :abc, but the package still loads just fine.
// Confirm that implicit_input.cc is not a target of the package, despite being used inside a
// symbolic macro (and not by anything at the top level) for both a target and a submacro.
//
// It'd be an execution time error to attempt to build the declared rule targets, but the
// package still loads just fine.
assertPackageNotInError(pkg);
assertThat(pkg.getTargets()).containsKey("abc");
assertThat(pkg.getTargets()).containsKey("abc_target");
assertThat(pkg.getTargets()).containsKey("abc_submacro_target");
assertThat(pkg.getTargets()).doesNotContainKey("implicit_input.cc");
}

Expand Down
117 changes: 111 additions & 6 deletions src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,26 +1348,131 @@ public void testSymbolicMacro_macroAndInputClash_inputDeclaredFirst() throws Exc
@Test
public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespaces()
throws Exception {
defineEmptyMacroBzl();
// We don't implicitly create InputFile targets whose names lie inside symbolic macros'
// namespaces, no matter where the file is referred to from. This avoids having to force
// evaluation of the macro when depending on the input file, to determine whether the macro
// declares a conflicting target.
//
// Create a macro instance named "foo" and try to refer to "foo_input" from various places.
// Ensure that "foo_input" does not in fact get created. (You could still used an
// exports_files() to declare it explicitly if you wanted.)
scratch.file(
"pkg/my_macro.bzl",
"""
def _sub_impl(name, visibility):
native.cc_library(
name = name + "_target",
srcs = ["foo_input"],
)
my_submacro = macro(implementation = _sub_impl)

def _impl(name, visibility):
native.cc_library(
name = name + "_target",
srcs = ["foo_input"],
)
my_submacro(name = name + "_submacro")
my_macro = macro(implementation = _impl)
""");
scratch.file(
"pkg/BUILD",
"""
load(":my_macro.bzl", "my_macro")
my_macro(name = "foo")
cc_library(
name = "lib",
srcs = ["foo", "foo_", "foo_bar", "baz"],
name = "toplevel_target",
srcs = [
"foo_input",
# Also try other name patterns.
"foo", # conflicts, not created
"foo_", # not in namespace, created
"baz", # not in namespace, created
],
)
""");
// You can't implicitly make an input file with a name that foo could've defined. (You can still
// have an explicit exports_files() do it.)

Package pkg = loadPackageAndAssertSuccess("pkg");
assertThat(pkg.getTargets()).doesNotContainKey("foo_input");
assertThat(pkg.getTargets()).doesNotContainKey("foo");
assertThat(pkg.getTargets()).doesNotContainKey("foo_bar");
assertThat(pkg.getTarget("foo_")).isInstanceOf(InputFile.class);
assertThat(pkg.getTarget("baz")).isInstanceOf(InputFile.class);
}

@Test
public void testSymbolicMacro_macroInstantiationCanForceImplicitCreationOfInputFile()
throws Exception {
// Referring to an input file when instantiating a top-level symbolic macro causes it to be
// implicitly created, even though no targets refer to it. Referring to an input when
// instantiating a submacro does not by itself cause creation.
scratch.file(
"pkg/my_macro.bzl",
"""
def _sub_impl(name, visibility, src):
pass
my_submacro = macro(
implementation = _sub_impl,
attrs = {"src": attr.label()},
)

def _impl(name, visibility, src):
my_submacro(
name = name + "_submacro",
src = "//pkg:does_not_exist",
)
my_macro = macro(
implementation = _impl,
attrs = {"src": attr.label()},
)
""");
scratch.file(
"pkg/BUILD",
"""
load(":my_macro.bzl", "my_macro")
my_macro(
name = "foo",
src = "//pkg:input",
)
""");

Package pkg = loadPackageAndAssertSuccess("pkg");
assertThat(pkg.getTarget("input")).isInstanceOf(InputFile.class);
assertThat(pkg.getTargets()).doesNotContainKey("does_not_exist");
}

@Test
public void testSymbolicMacro_failsGracefullyWhenInputFileClashesWithMisnamedMacroTarget()
throws Exception {
// Symbolic macros can't define usable targets outside their namespace, and BUILD files can't
// implicitly create input files inside a macro's namespace. But we could still have a conflict
// between an *unusable* ill-named macro target and an implicitly created input file. Make sure
// we don't crash at least.
//
// If symbolic macros are evaluated either synchronously with their instantiation or deferred to
// the end of the BUILD file, the target declared by the macro wins because it happens before
// implicit input file creation. But under lazy macro evaluation, the implicit input file will
// win and we should see a conflict if the macro is expanded.
// TODO: #23852 - Test behavior under lazy macro evaluation when implemented.
scratch.file(
"pkg/my_macro.bzl",
"""
def _impl(name, visibility):
native.cc_library(name="conflicting_name")
my_macro = macro(implementation=_impl)
""");
scratch.file(
"pkg/BUILD",
"""
load(":my_macro.bzl", "my_macro")
my_macro(name="foo")
cc_library(
name = "bar",
srcs = [":conflicting_name"],
)
""");
Package pkg = loadPackageAndAssertSuccess("pkg");
assertThat(pkg.getTarget("conflicting_name")).isInstanceOf(Rule.class);
}

@Test
public void testSymbolicMacro_implicitCreationOfInputFilesIsNotTriggeredByMacros()
throws Exception {
Expand Down