diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index e80097649472f8..4f5afc9c421ad6 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -1529,10 +1529,7 @@ private ClassObject newNativeModule() { return StructProvider.STRUCT.create(builder.build(), "no native function or rule '%s'"); } - private void buildPkgEnv( - Environment pkgEnv, - PackageContext context, - PackageIdentifier packageId) { + private void buildPkgEnv(Environment pkgEnv, PackageContext context) { // TODO(bazel-team): remove the naked functions that are redundant with the nativeModule, // or if not possible, at least make them straight copies from the native module variant. // or better, use a common Environment.Frame for these common bindings @@ -1569,10 +1566,6 @@ private void buildPkgEnv( } pkgEnv.setupDynamic(PKG_CONTEXT, context); - if (!pkgEnv.getSemantics().incompatiblePackageNameIsAFunction()) { - pkgEnv.setupDynamic(Runtime.PKG_NAME, packageId.getPackageFragment().getPathString()); - pkgEnv.setupDynamic(Runtime.REPOSITORY_NAME, packageId.getRepository().toString()); - } } /** @@ -1647,7 +1640,7 @@ public Package.Builder evaluateBuildFile( PackageContext context = new PackageContext( pkgBuilder, globber, eventHandler, ruleFactory.getAttributeContainerFactory()); - buildPkgEnv(pkgEnv, context, packageId); + buildPkgEnv(pkgEnv, context); if (!validatePackageIdentifier(packageId, buildFileAST.getLocation(), eventHandler)) { pkgBuilder.setContainsErrors(); diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 653a99de732ae4..008f8923059999 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java @@ -421,20 +421,6 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable + "symbols introduced by load are not implicitly re-exported.") public boolean incompatibleNoTransitiveLoads; - @Option( - name = "incompatible_package_name_is_a_function", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, - effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, - metadataTags = { - OptionMetadataTag.INCOMPATIBLE_CHANGE, - OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES - }, - help = - "If set to true, the values PACKAGE_NAME and REPOSITORY_NAME are not available. " - + "Use the package_name() or repository_name() functions instead.") - public boolean incompatiblePackageNameIsAFunction; - @Option( name = "incompatible_range_type", defaultValue = "true", @@ -490,21 +476,6 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable + "will be available") public boolean incompatibleRemoveNativeMavenJar; - @Option( - name = "incompatible_static_name_resolution", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, - effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, - metadataTags = { - OptionMetadataTag.INCOMPATIBLE_CHANGE, - OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES - }, - help = - "If set to true, the interpreter follows the semantics related to name resolution, " - + "scoping, and shadowing, as defined in " - + "https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md") - public boolean incompatibleStaticNameResolution; - @Option( name = "incompatible_strict_argument_ordering", defaultValue = "false", @@ -576,12 +547,10 @@ public SkylarkSemantics toSkylarkSemantics() { .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs) .incompatibleNoTargetOutputGroup(incompatibleNoTargetOutputGroup) .incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads) - .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction) .incompatibleRangeType(incompatibleRangeType) .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository) .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive) .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar) - .incompatibleStaticNameResolution(incompatibleStaticNameResolution) .incompatibleStricArgumentOrdering(incompatibleStricArgumentOrdering) .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable) .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index 81c48832396147..b8d20f53d66410 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java @@ -541,26 +541,17 @@ private static final class Continuation { /** The global Frame of the caller. */ final GlobalFrame globalFrame; - /** - * The set of known global variables of the caller. - * - *

TODO(laurentlb): Remove this when we use static name resolution. - */ - @Nullable final LinkedHashSet knownGlobalVariables; - Continuation( @Nullable Continuation continuation, BaseFunction function, @Nullable FuncallExpression caller, Frame lexicalFrame, - GlobalFrame globalFrame, - @Nullable LinkedHashSet knownGlobalVariables) { + GlobalFrame globalFrame) { this.continuation = continuation; this.function = function; this.caller = caller; this.lexicalFrame = lexicalFrame; this.globalFrame = globalFrame; - this.knownGlobalVariables = knownGlobalVariables; } } @@ -766,15 +757,6 @@ public int hashCode() { */ private final Map importedExtensions; - /** - * When in a lexical (Skylark) Frame, this set contains the variable names that are global, as - * determined not by global declarations (not currently supported), but by previous lookups that - * ended being global or dynamic. This is necessary because if in a function definition something - * reads a global variable after which a local variable with the same name is assigned an - * Exception needs to be thrown. - */ - @Nullable private LinkedHashSet knownGlobalVariables; - /** * When in a lexical (Skylark) frame, this lists the names of the functions in the call stack. We * currently use it to artificially disable recursion. @@ -801,12 +783,9 @@ void enterScope( Frame lexical, @Nullable FuncallExpression caller, GlobalFrame globals) { - continuation = - new Continuation( - continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables); + continuation = new Continuation(continuation, function, caller, lexicalFrame, globalFrame); lexicalFrame = lexical; globalFrame = globals; - knownGlobalVariables = new LinkedHashSet<>(); } /** Exits a scope by restoring state from the current continuation */ @@ -814,7 +793,6 @@ void exitScope() { Preconditions.checkNotNull(continuation); lexicalFrame = continuation.lexicalFrame; globalFrame = continuation.globalFrame; - knownGlobalVariables = continuation.knownGlobalVariables; continuation = continuation.continuation; } @@ -1087,14 +1065,6 @@ public Environment updateAndExport(String varname, Object value) throws EvalExce */ public Environment update(String varname, Object value) throws EvalException { Preconditions.checkNotNull(value, "trying to assign null to '%s'", varname); - if (isKnownGlobalVariable(varname)) { - throw new EvalException( - null, - String.format( - "Variable '%s' is referenced before assignment. " - + "The variable is defined in the global scope.", - varname)); - } try { lexicalFrame.put(this, varname, value); } catch (MutabilityException e) { @@ -1158,14 +1128,7 @@ public Object moduleLookup(String varname) { /** Returns the value of a variable defined in the Universe scope (builtins). */ public Object universeLookup(String varname) { // TODO(laurentlb): look only at globalFrame.universe. - Object result = globalFrame.get(varname); - - if (result == null) { - // TODO(laurentlb): Remove once PACKAGE_NAME and REPOSITOYRY_NAME are removed (they are the - // only two user-visible values that use the dynamicFrame). - return dynamicLookup(varname); - } - return result; + return globalFrame.get(varname); } /** Returns the value of a variable defined with setupDynamic. */ @@ -1186,17 +1149,10 @@ Object lookup(String varname) { return lexicalValue; } Object globalValue = globalFrame.get(varname); - Object dynamicValue = dynamicFrame.get(varname); - if (globalValue == null && dynamicValue == null) { + if (globalValue == null) { return null; } - if (knownGlobalVariables != null) { - knownGlobalVariables.add(varname); - } - if (globalValue != null) { - return globalValue; - } - return dynamicValue; + return globalValue; } /** @@ -1209,16 +1165,6 @@ public Map getRestrictedBindings() { return globalFrame.restrictedBindings; } - /** - * Returns true if varname is a known global variable (i.e., it has been read in the context of - * the current function). - */ - boolean isKnownGlobalVariable(String varname) { - return !semantics.incompatibleStaticNameResolution() - && knownGlobalVariables != null - && knownGlobalVariables.contains(varname); - } - public SkylarkSemantics getSemantics() { return semantics; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java index dcd39b6e65a6f0..98e7d5b69ba855 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java @@ -81,7 +81,7 @@ void setScope(ValidationEnvironment.Scope scope) { @Override Object doEval(Environment env) throws EvalException { Object result; - if (scope == null || !env.getSemantics().incompatibleStaticNameResolution()) { + if (scope == null) { // Legacy behavior, to be removed. result = env.lookup(name); if (result == null) { @@ -107,12 +107,9 @@ Object doEval(Environment env) throws EvalException { if (result == null) { // Since Scope was set, we know that the variable is defined in the scope. // However, the assignment was not yet executed. - EvalException e = getSpecialException(); - throw e != null - ? e - : new EvalException( - getLocation(), - scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); + throw new EvalException( + getLocation(), + scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); } return result; @@ -128,39 +125,11 @@ public Kind kind() { return Kind.IDENTIFIER; } - /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */ - private EvalException getSpecialException() { - if (name.equals("PACKAGE_NAME")) { - return new EvalException( - getLocation(), - "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', " - + "please use the latter (" - + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). " - + "You can temporarily allow the old name " - + "by using --incompatible_package_name_is_a_function=false"); - } - if (name.equals("REPOSITORY_NAME")) { - return new EvalException( - getLocation(), - "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', " - + "please use the latter (" - + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name)." - + " You can temporarily allow the old name " - + "by using --incompatible_package_name_is_a_function=false"); - } - return null; - } - EvalException createInvalidIdentifierException(Set symbols) { if (name.equals("$error$")) { return new EvalException(getLocation(), "contains syntax error(s)", true); } - EvalException e = getSpecialException(); - if (e != null) { - return e; - } - String suggestion = SpellChecker.didYouMean(name, symbols); return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index ff92277a6e16e1..ac3fa3a75e4932 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java @@ -168,8 +168,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleNoTransitiveLoads(); - public abstract boolean incompatiblePackageNameIsAFunction(); - public abstract boolean incompatibleRangeType(); public abstract boolean incompatibleRemoveNativeGitRepository(); @@ -178,8 +176,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleRemoveNativeMavenJar(); - public abstract boolean incompatibleStaticNameResolution(); - public abstract boolean incompatibleStricArgumentOrdering(); public abstract boolean incompatibleStringIsNotIterable(); @@ -229,12 +225,10 @@ public static Builder builderWithDefaults() { .incompatibleNoSupportToolsInActionInputs(false) .incompatibleNoTargetOutputGroup(false) .incompatibleNoTransitiveLoads(false) - .incompatiblePackageNameIsAFunction(true) .incompatibleRangeType(true) .incompatibleRemoveNativeGitRepository(true) .incompatibleRemoveNativeHttpArchive(true) .incompatibleRemoveNativeMavenJar(false) - .incompatibleStaticNameResolution(true) .incompatibleStricArgumentOrdering(false) .incompatibleStringIsNotIterable(false) .internalSkylarkFlagTestCanary(false) @@ -301,8 +295,6 @@ public abstract static class Builder { public abstract Builder incompatibleNoTransitiveLoads(boolean value); - public abstract Builder incompatiblePackageNameIsAFunction(boolean value); - public abstract Builder incompatibleRangeType(boolean value); public abstract Builder incompatibleRemoveNativeGitRepository(boolean value); @@ -311,8 +303,6 @@ public abstract static class Builder { public abstract Builder incompatibleRemoveNativeMavenJar(boolean value); - public abstract Builder incompatibleStaticNameResolution(boolean value); - public abstract Builder incompatibleStricArgumentOrdering(boolean value); public abstract Builder incompatibleStringIsNotIterable(boolean value); diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java index 4da043689fa4d5..d99c5f84005ce8 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java @@ -238,34 +238,6 @@ public void testPrefixBetweenRules2() throws Exception { "output file 'a' of rule 'kiwi2' conflicts " + "with output file 'a/b' of rule 'kiwi1'"); } - @Test - public void testPackageConstant() throws Exception { - Path buildFile = - scratch.file("/pina/BUILD", "cc_library(name=PACKAGE_NAME + '-colada')"); - - Package pkg = - packages.createPackage( - "pina", - RootedPath.toRootedPath(root, buildFile), - "--incompatible_package_name_is_a_function=false"); - events.assertNoWarningsOrErrors(); - assertThat(pkg.containsErrors()).isFalse(); - assertThat(pkg.getRule("pina-colada")).isNotNull(); - assertThat(pkg.getRule("pina-colada").containsErrors()).isFalse(); - assertThat(Sets.newHashSet(pkg.getTargets(Rule.class)).size()).isSameAs(1); - } - - @Test - public void testPackageConstantIsForbidden() throws Exception { - events.setFailFast(false); - Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=PACKAGE_NAME + '-colada')"); - packages.createPackage( - "pina", - RootedPath.toRootedPath(root, buildFile), - "--incompatible_package_name_is_a_function=true"); - events.assertContainsError("The value 'PACKAGE_NAME' has been removed"); - } - @Test public void testPackageNameFunction() throws Exception { Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=package_name() + '-colada')"); @@ -278,36 +250,6 @@ public void testPackageNameFunction() throws Exception { assertThat(Sets.newHashSet(pkg.getTargets(Rule.class)).size()).isSameAs(1); } - @Test - public void testPackageConstantInExternalRepository() throws Exception { - Path buildFile = - scratch.file( - "/external/a/b/BUILD", - "genrule(name='c', srcs=[], outs=['ao'], cmd=REPOSITORY_NAME + ' ' + PACKAGE_NAME)"); - Package pkg = - packages.createPackage( - PackageIdentifier.create("@a", PathFragment.create("b")), - RootedPath.toRootedPath(root, buildFile), - events.reporter(), - "--incompatible_package_name_is_a_function=false"); - Rule c = pkg.getRule("c"); - assertThat(AggregatingAttributeMapper.of(c).get("cmd", Type.STRING)).isEqualTo("@a b"); - } - - @Test - public void testPackageConstantInExternalRepositoryIsForbidden() throws Exception { - events.setFailFast(false); - Path buildFile = - scratch.file( - "/external/a/b/BUILD", "genrule(name='c', srcs=[], outs=['ao'], cmd=REPOSITORY_NAME)"); - packages.createPackage( - PackageIdentifier.create("@a", PathFragment.create("b")), - RootedPath.toRootedPath(root, buildFile), - events.reporter(), - "--incompatible_package_name_is_a_function=true"); - events.assertContainsError("The value 'REPOSITORY_NAME' has been removed"); - } - @Test public void testPackageFunctionInExternalRepository() throws Exception { Path buildFile = diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index fe41bf7fa4a424..ac6fc3958a9907 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -149,12 +149,10 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(), "--incompatible_no_target_output_group=" + rand.nextBoolean(), "--incompatible_no_transitive_loads=" + rand.nextBoolean(), - "--incompatible_package_name_is_a_function=" + rand.nextBoolean(), "--incompatible_range_type=" + rand.nextBoolean(), "--incompatible_remove_native_git_repository=" + rand.nextBoolean(), "--incompatible_remove_native_http_archive=" + rand.nextBoolean(), "--incompatible_remove_native_maven_jar=" + rand.nextBoolean(), - "--incompatible_static_name_resolution=" + rand.nextBoolean(), "--incompatible_strict_argument_ordering=" + rand.nextBoolean(), "--incompatible_string_is_not_iterable=" + rand.nextBoolean(), "--internal_skylark_flag_test_canary=" + rand.nextBoolean()); @@ -196,12 +194,10 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) { .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean()) .incompatibleNoTargetOutputGroup(rand.nextBoolean()) .incompatibleNoTransitiveLoads(rand.nextBoolean()) - .incompatiblePackageNameIsAFunction(rand.nextBoolean()) .incompatibleRangeType(rand.nextBoolean()) .incompatibleRemoveNativeGitRepository(rand.nextBoolean()) .incompatibleRemoveNativeHttpArchive(rand.nextBoolean()) .incompatibleRemoveNativeMavenJar(rand.nextBoolean()) - .incompatibleStaticNameResolution(rand.nextBoolean()) .incompatibleStricArgumentOrdering(rand.nextBoolean()) .incompatibleStringIsNotIterable(rand.nextBoolean()) .internalSkylarkFlagTestCanary(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java index bbad014fca6e62..152480c19f52ef 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java @@ -201,9 +201,7 @@ public void testFrozen() throws Exception { @Test public void testBuiltinsCanBeShadowed() throws Exception { - Environment env = - newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true") - .setup("special_var", 42); + Environment env = newEnvironmentWithSkylarkOptions().setup("special_var", 42); BuildFileAST.eval(env, "special_var = 41"); assertThat(env.moduleLookup("special_var")).isEqualTo(41); } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index e2325cef2f8bd9..e7f386172839dd 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -1807,7 +1807,7 @@ public void testLocalVariableDefinedBelow() throws Exception { @Test public void testShadowisNotInitialized() throws Exception { - new SkylarkTest("--incompatible_static_name_resolution=true") + new SkylarkTest() .testIfErrorContains( /* error message */ "local variable 'gl' is referenced before assignment", "gl = 5", @@ -1817,23 +1817,9 @@ public void testShadowisNotInitialized() throws Exception { "res = foo()"); } - @Test - public void testLegacyGlobalVariableNotShadowed() throws Exception { - new SkylarkTest("--incompatible_static_name_resolution=false") - .setUp( - "gl = 5", - "def foo():", - " if False: gl = 2", - // The legacy behavior is that the global variable is returned. - // With --incompatible_static_name_resolution set to true, this becomes an error. - " return gl", - "res = foo()") - .testLookup("res", 5); - } - @Test public void testShadowBuiltin() throws Exception { - new SkylarkTest("--incompatible_static_name_resolution=true") + new SkylarkTest() .testIfErrorContains( "global variable 'len' is referenced before assignment", "x = len('abc')", @@ -1841,13 +1827,6 @@ public void testShadowBuiltin() throws Exception { "y = x + len"); } - @Test - public void testLegacyShadowBuiltin() throws Exception { - new SkylarkTest("--incompatible_static_name_resolution=false") - .setUp("x = len('abc')", "len = 2", "y = x + len") - .testLookup("y", 5); - } - @Test public void testFunctionCallRecursion() throws Exception { new SkylarkTest().testIfErrorContains("Recursion was detected when calling 'f' from 'g'", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java index c8e1be5803f8c2..ea5819338b594a 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java @@ -133,13 +133,13 @@ public void testFunctionDefinedBelow() { @Test public void testGlobalDefinedBelow() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + env = newEnvironmentWithSkylarkOptions(); parse("def bar(): return x", "x = 5\n"); } @Test public void testLocalVariableDefinedBelow() throws Exception { - env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + env = newEnvironmentWithSkylarkOptions(); parse( "def bar():", " for i in range(5):",