Skip to content

Commit

Permalink
Properly handle selects with None values for label-valued attrs in na…
Browse files Browse the repository at this point in the history
…tive.existing_rule and native.existing_rules

Label-valued attributes have a default value of null in Java. The BuildType.Selector
constructor casts None values of select branches to that default - i.e. to null; but
we must transform that null back to None when reconstructing a SelectorValue in
native.existing_rule/s for use in Starlark - or we would miss a branch.

And take the opportunity to ensure we never attempt to construct an empty
SelectorValue or SelectorList - which is a fatal error.

PiperOrigin-RevId: 656122706
Change-Id: Id236af711ba7f7c515a1b238e4be9d7d63574173
  • Loading branch information
tetromino authored and copybara-github committed Jul 25, 2024
1 parent d88433d commit 38ade70
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -809,17 +809,30 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg) {
selector.forEach(
(rawKey, rawValue) -> {
Object key = starlarkifyValue(mu, rawKey, pkg);
Object mapVal = starlarkifyValue(mu, rawValue, pkg);
// BuildType.Selector constructor transforms `None` values of selector branches into
// Java nulls if the selector original type's default value is null. We need to
// reverse this transformation.
Object mapVal =
rawValue == null && selector.getOriginalType().getDefaultValue() == null
? Starlark.NONE
: starlarkifyValue(mu, rawValue, pkg);
if (key != null && mapVal != null) {
m.put(key, mapVal);
}
});
selectors.add(new SelectorValue(((Map<?, ?>) m.build(mu)), selector.getNoMatchError()));
Dict<?, ?> selectorDict = m.build(mu);
if (!selectorDict.isEmpty()) {
selectors.add(new SelectorValue(selectorDict, selector.getNoMatchError()));
}
}
try {
return SelectorList.of(selectors);
} catch (EvalException e) {
if (selectors.isEmpty()) {
return null;
} else {
try {
return SelectorList.of(selectors);
} catch (EvalException e) {
return null;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,48 @@ def rule_dict(name):
assertThat(getConfiguredTarget("//test/getrule:x")).isNotNull();
}

// Regression test for b/355432322
@Test
public void existingRule_handlesSelectWithNoneValues_forLabelValuedAttributes() throws Exception {
scratch.file("test/starlark/BUILD");
scratch.file(
"test/starlark/rulestr.bzl",
"""
def save_dep(rule_name):
r = native.existing_rule(rule_name)
test.save("dep", r["dep"])
def _impl(ctx):
pass
my_rule = rule(
implementation = _impl,
attrs = {
"dep": attr.label(),
},
)
""");

scratch.file(
"test/getrule/BUILD",
"""
load("//test/starlark:rulestr.bzl", "my_rule", "save_dep")
my_rule(
name = "x",
dep = select({"//conditions:default": None}),
)
save_dep("x")
""");

// Parse the BUILD file, to make sure select() makes it out of native.existing_rule().
assertThat(getConfiguredTarget("//test/getrule:x")).isNotNull();

// We have to compare by stringification because SelectorValue has reference equality semantics.
assertThat(getSaved("dep").toString()).isEqualTo("select({\"//conditions:default\": None})");
}

@Test
public void existingRule_returnsNone() throws Exception {
scratch.file(
Expand Down

0 comments on commit 38ade70

Please sign in to comment.