Skip to content

Commit 752ffcc

Browse files
aehligcopybara-github
aehlig
authored and
copybara-github
committed
Implement --incompatible_visibility_private_attributes_at_definition
If enabled, change visibility verification for implict attributes: For implicit attributes of a rule, i.e., attributes fixed in the rule definition and not setable by the user of the rule, verify that the rule definition can see the attribute, rather than the usage of the rule. Change-Id: I6fb6e288885c8e0432e94de129882e71064c669b PiperOrigin-RevId: 277683498
1 parent 1b3eb42 commit 752ffcc

File tree

8 files changed

+230
-14
lines changed

8 files changed

+230
-14
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public void validate(
4141
ConfiguredTargetAndData prerequisite,
4242
Attribute attribute) {
4343
validateDirectPrerequisiteLocation(contextBuilder, prerequisite);
44-
validateDirectPrerequisiteVisibility(contextBuilder, prerequisite, attribute.getName());
44+
validateDirectPrerequisiteVisibility(contextBuilder, prerequisite, attribute);
4545
validateDirectPrerequisiteForTestOnly(contextBuilder, prerequisite);
4646
validateDirectPrerequisiteForDeprecation(
4747
contextBuilder, contextBuilder.getRule(), prerequisite, contextBuilder.forAspect());
@@ -66,18 +66,43 @@ public abstract boolean isSameLogicalPackage(
6666
protected abstract boolean allowExperimentalDeps(RuleContext.Builder context);
6767

6868
private void validateDirectPrerequisiteVisibility(
69-
RuleContext.Builder context, ConfiguredTargetAndData prerequisite, String attrName) {
69+
RuleContext.Builder context, ConfiguredTargetAndData prerequisite, Attribute attribute) {
70+
String attrName = attribute.getName();
7071
Rule rule = context.getRule();
7172
Target prerequisiteTarget = prerequisite.getTarget();
73+
7274
// We don't check the visibility of late-bound attributes, because it would break some
7375
// features.
7476
if (!isSameLogicalPackage(
7577
rule.getLabel().getPackageIdentifier(),
7678
AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget())
7779
.getPackageIdentifier())
78-
&& !context.isVisible(prerequisite.getConfiguredTarget())
7980
&& !Attribute.isLateBound(attrName)) {
80-
handleVisibilityConflict(context, prerequisite, rule);
81+
82+
// Determine if we should use the new visibility rules for tools.
83+
boolean toolCheckAtDefinition = false;
84+
try {
85+
toolCheckAtDefinition =
86+
context.getStarlarkSemantics().incompatibleVisibilityPrivateAttributesAtDefinition();
87+
} catch (InterruptedException e) {
88+
Thread.currentThread().interrupt();
89+
}
90+
91+
if (!toolCheckAtDefinition
92+
|| !attribute.isImplicit()
93+
|| rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null) {
94+
// Default check: The attribute must be visible from the target.
95+
if (!context.isVisible(prerequisite.getConfiguredTarget())) {
96+
handleVisibilityConflict(context, prerequisite, rule.getLabel());
97+
}
98+
} else {
99+
// For implicit attributes, check if the prerequisite is visible from the location of the
100+
// rule definition
101+
Label implicitDefinition = rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel();
102+
if (!RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
103+
handleVisibilityConflict(context, prerequisite, implicitDefinition);
104+
}
105+
}
81106
}
82107

83108
if (prerequisiteTarget instanceof PackageGroup) {
@@ -107,8 +132,8 @@ private void validateDirectPrerequisiteVisibility(
107132
}
108133

109134
private void handleVisibilityConflict(
110-
RuleContext.Builder context, ConfiguredTargetAndData prerequisite, Rule rule) {
111-
if (packageUnderExperimental(rule.getLabel().getPackageIdentifier())
135+
RuleContext.Builder context, ConfiguredTargetAndData prerequisite, Label rule) {
136+
if (packageUnderExperimental(rule.getPackageIdentifier())
112137
&& !checkVisibilityForExperimental(context)) {
113138
return;
114139
}
@@ -118,17 +143,15 @@ private void handleVisibilityConflict(
118143
String.format(
119144
"Target '%s' violates visibility of "
120145
+ "%s. Continuing because --nocheck_visibility is active",
121-
rule.getLabel(),
122-
AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND));
146+
rule, AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND));
123147
context.ruleWarning(errorMessage);
124148
} else {
125149
String errorMessage =
126150
String.format(
127151
"%s is not visible from target '%s'. Check "
128152
+ "the visibility declaration of the former target if you think "
129153
+ "the dependency is legitimate",
130-
AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND),
131-
rule.getLabel());
154+
AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND), rule);
132155
context.ruleError(errorMessage);
133156
}
134157
}

‎src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,20 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
398398
+ "instead return a list of provider instances.")
399399
public boolean incompatibleDisallowStructProviderSyntax;
400400

401+
@Option(
402+
name = "incompatible_visibility_private_attributes_at_definition",
403+
defaultValue = "false",
404+
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
405+
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
406+
metadataTags = {
407+
OptionMetadataTag.INCOMPATIBLE_CHANGE,
408+
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
409+
},
410+
help =
411+
"If set to true, the visibility of private rule attributes is checked with respect "
412+
+ "to the rule definition, rather than the rule usage.")
413+
public boolean incompatibleVisibilityPrivateAttributesAtDefinition;
414+
401415
/** Controls legacy arguments to ctx.actions.Args#add. */
402416
@Option(
403417
name = "incompatible_disallow_old_style_args_add",
@@ -689,6 +703,8 @@ public StarlarkSemantics toSkylarkSemantics() {
689703
.incompatibleRestrictNamedParams(incompatibleRestrictNamedParams)
690704
.incompatibleRunShellCommandString(incompatibleRunShellCommandString)
691705
.incompatibleStringJoinRequiresStrings(incompatibleStringJoinRequiresStrings)
706+
.incompatibleVisibilityPrivateAttributesAtDefinition(
707+
incompatibleVisibilityPrivateAttributesAtDefinition)
692708
.internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary)
693709
.incompatibleDoNotSplitLinkingCmdline(incompatibleDoNotSplitLinkingCmdline)
694710
.incompatibleDepsetForLibrariesToLinkGetter(incompatibleDepsetForLibrariesToLinkGetter)

‎src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
201201

202202
public abstract boolean incompatibleStringJoinRequiresStrings();
203203

204+
public abstract boolean incompatibleVisibilityPrivateAttributesAtDefinition();
205+
204206
public abstract boolean internalSkylarkFlagTestCanary();
205207

206208
public abstract boolean incompatibleDoNotSplitLinkingCmdline();
@@ -285,6 +287,7 @@ public static Builder builderWithDefaults() {
285287
.incompatibleRunShellCommandString(false)
286288
.incompatibleRestrictNamedParams(true)
287289
.incompatibleStringJoinRequiresStrings(true)
290+
.incompatibleVisibilityPrivateAttributesAtDefinition(false)
288291
.internalSkylarkFlagTestCanary(false)
289292
.incompatibleDoNotSplitLinkingCmdline(true)
290293
.incompatibleDepsetForLibrariesToLinkGetter(true)
@@ -373,6 +376,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo
373376

374377
public abstract Builder incompatibleStringJoinRequiresStrings(boolean value);
375378

379+
public abstract Builder incompatibleVisibilityPrivateAttributesAtDefinition(boolean value);
380+
376381
public abstract Builder internalSkylarkFlagTestCanary(boolean value);
377382

378383
public abstract Builder incompatibleDoNotSplitLinkingCmdline(boolean value);

‎src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
// Copyright 2019 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.devtools.build.lib.analysis;
16+
17+
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
19+
20+
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.junit.runners.JUnit4;
24+
25+
/** Test for visibility of targets. */
26+
@RunWith(JUnit4.class)
27+
public class VisibilityTest extends AnalysisTestCase {
28+
29+
void setupArgsScenario() throws Exception {
30+
scratch.file("tool/tool.sh", "#!/bin/sh", "echo Hello > $2", "cat $1 >> $2");
31+
scratch.file("rule/BUILD");
32+
scratch.file(
33+
"rule/rule.bzl",
34+
"def _impl(ctx):",
35+
" output = ctx.actions.declare_file(ctx.label.name + '.out')",
36+
" ctx.actions.run(",
37+
" inputs = ctx.files._tool + ctx.files.data,",
38+
" executable = ctx.files._tool[0].path,",
39+
" arguments = [f.path for f in ctx.files.data] + [output.path],",
40+
" outputs = [output],",
41+
" )",
42+
"",
43+
"greet = rule(",
44+
" implementation = _impl,",
45+
" attrs = {",
46+
" 'data' : attr.label(allow_files=True),",
47+
" '_tool' : attr.label(cfg='host', allow_files=True,",
48+
" default = Label('//tool:tool.sh')),",
49+
" },",
50+
" outputs = {'out' : '%{name}.out'},",
51+
")");
52+
scratch.file("data/data.txt", "World");
53+
scratch.file(
54+
"use/BUILD",
55+
"load('//rule:rule.bzl', 'greet')",
56+
"",
57+
"greet(",
58+
" name = 'world',",
59+
" data = '//data:data.txt',",
60+
")");
61+
}
62+
63+
@Test
64+
public void testToolVisibilityRuleCheckAtRule() throws Exception {
65+
setupArgsScenario();
66+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
67+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//rule:__pkg__'])");
68+
useConfiguration("--incompatible_visibility_private_attributes_at_definition");
69+
update("//use:world");
70+
assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse();
71+
}
72+
73+
@Test
74+
public void testToolVisibilityRuleCheckAtUse() throws Exception {
75+
setupArgsScenario();
76+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
77+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//rule:__pkg__'])");
78+
useConfiguration("--noincompatible_visibility_private_attributes_at_definition");
79+
80+
reporter.removeHandler(failFastHandler);
81+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
82+
}
83+
84+
@Test
85+
public void testToolVisibilityUseCheckAtUse() throws Exception {
86+
setupArgsScenario();
87+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
88+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//use:__pkg__'])");
89+
useConfiguration("--noincompatible_visibility_private_attributes_at_definition");
90+
update("//use:world");
91+
assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse();
92+
}
93+
94+
@Test
95+
public void testToolVisibilityUseCheckAtRule() throws Exception {
96+
setupArgsScenario();
97+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
98+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//use:__pkg__'])");
99+
useConfiguration("--incompatible_visibility_private_attributes_at_definition");
100+
101+
reporter.removeHandler(failFastHandler);
102+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
103+
}
104+
105+
@Test
106+
public void testToolVisibilityPrivateCheckAtUse() throws Exception {
107+
setupArgsScenario();
108+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
109+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:private'])");
110+
useConfiguration("--noincompatible_visibility_private_attributes_at_definition");
111+
112+
reporter.removeHandler(failFastHandler);
113+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
114+
}
115+
116+
@Test
117+
public void testToolVisibilityPrivateCheckAtRule() throws Exception {
118+
setupArgsScenario();
119+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
120+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:private'])");
121+
useConfiguration("--incompatible_visibility_private_attributes_at_definition");
122+
123+
reporter.removeHandler(failFastHandler);
124+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
125+
}
126+
127+
@Test
128+
public void testDataVisibilityUseCheckPrivateAtUse() throws Exception {
129+
setupArgsScenario();
130+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//use:__pkg__'])");
131+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:public'])");
132+
useConfiguration("--noincompatible_visibility_private_attributes_at_definition");
133+
update("//use:world");
134+
assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse();
135+
}
136+
137+
@Test
138+
public void testDataVisibilityUseCheckPrivateAtRule() throws Exception {
139+
setupArgsScenario();
140+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//use:__pkg__'])");
141+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:public'])");
142+
useConfiguration("--incompatible_visibility_private_attributes_at_definition");
143+
update("//use:world");
144+
assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse();
145+
}
146+
147+
@Test
148+
public void testDataVisibilityPrivateCheckPrivateAtRule() throws Exception {
149+
setupArgsScenario();
150+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:private'])");
151+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:public'])");
152+
useConfiguration("--incompatible_visibility_private_attributes_at_definition");
153+
154+
reporter.removeHandler(failFastHandler);
155+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
156+
}
157+
158+
@Test
159+
public void testDataVisibilityPrivateCheckPrivateAtUse() throws Exception {
160+
setupArgsScenario();
161+
scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:private'])");
162+
scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//visibility:public'])");
163+
useConfiguration("--noincompatible_visibility_private_attributes_at_definition");
164+
165+
reporter.removeHandler(failFastHandler);
166+
assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
167+
}
168+
}

‎src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ public RuleContext getRuleContextForTesting(
463463
targetConfig.extendedSanityChecks(),
464464
targetConfig.allowAnalysisFailures(),
465465
eventHandler,
466-
/*env=*/ null);
466+
skyframeExecutor.getSkyFunctionEnvironmentForTesting(eventHandler));
467467
return getRuleContextForTesting(eventHandler, target, env, configurations);
468468
}
469469

‎src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,8 @@ public void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Ex
258258
getOutputPath().createDirectoryAndParents();
259259
ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues =
260260
ImmutableList.of(
261+
PrecomputedValue.injected(
262+
PrecomputedValue.STARLARK_SEMANTICS, StarlarkSemantics.DEFAULT_SEMANTICS),
261263
PrecomputedValue.injected(PrecomputedValue.REPO_ENV, ImmutableMap.<String, String>of()),
262264
PrecomputedValue.injected(
263265
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES,
@@ -582,7 +584,7 @@ public SkyFunctionName functionName() {
582584
/*extendedSanityChecks=*/ false,
583585
/*allowAnalysisFailures=*/ false,
584586
reporter,
585-
/* env= */ null);
587+
skyframeExecutor.getSkyFunctionEnvironmentForTesting(reporter));
586588
}
587589

588590
/**
@@ -2000,7 +2002,7 @@ public SkyFunction.Environment getSkyframeEnv() {
20002002

20012003
@Override
20022004
public StarlarkSemantics getSkylarkSemantics() {
2003-
throw new UnsupportedOperationException();
2005+
return starlarkSemanticsOptions.toSkylarkSemantics();
20042006
}
20052007

20062008
@Override

‎src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
160160
"--incompatible_restrict_named_params=" + rand.nextBoolean(),
161161
"--incompatible_run_shell_command_string=" + rand.nextBoolean(),
162162
"--incompatible_string_join_requires_strings=" + rand.nextBoolean(),
163+
"--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(),
163164
"--incompatible_restrict_string_escapes=" + rand.nextBoolean(),
164165
"--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(),
165166
"--incompatible_disallow_hashing_frozen_mutables=" + rand.nextBoolean(),
@@ -213,6 +214,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
213214
.incompatibleRestrictNamedParams(rand.nextBoolean())
214215
.incompatibleRunShellCommandString(rand.nextBoolean())
215216
.incompatibleStringJoinRequiresStrings(rand.nextBoolean())
217+
.incompatibleVisibilityPrivateAttributesAtDefinition(rand.nextBoolean())
216218
.incompatibleRestrictStringEscapes(rand.nextBoolean())
217219
.incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean())
218220
.incompatibleDisallowHashingFrozenMutables(rand.nextBoolean())

‎src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ public RuleContext getRuleContextForActionTesting(ConfiguredTarget dummyTarget)
241241
targetConfig.extendedSanityChecks(),
242242
targetConfig.allowAnalysisFailures(),
243243
eventHandler,
244-
null),
244+
skyframeExecutor.getSkyFunctionEnvironmentForTesting(eventHandler)),
245245
new BuildConfigurationCollection(
246246
ImmutableList.of(dummy.getConfiguration()), dummy.getHostConfiguration()));
247247
}

0 commit comments

Comments
 (0)