Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Code simplification in the environment, remove old incompatible flags
Remove --incompatible_static_name_resolution and --incompatible_package_name_is_a_function

#5827
#5637

RELNOTES: None.
PiperOrigin-RevId: 224140026
  • Loading branch information
laurentlb authored and Copybara-Service committed Dec 5, 2018
1 parent f2b3bec commit 2b3ad18
Show file tree
Hide file tree
Showing 10 changed files with 16 additions and 234 deletions.
Expand Up @@ -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
Expand Down Expand Up @@ -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());
}
}

/**
Expand Down Expand Up @@ -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();
Expand Down
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down
Expand Up @@ -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.
*
* <p>TODO(laurentlb): Remove this when we use static name resolution.
*/
@Nullable final LinkedHashSet<String> knownGlobalVariables;

Continuation(
@Nullable Continuation continuation,
BaseFunction function,
@Nullable FuncallExpression caller,
Frame lexicalFrame,
GlobalFrame globalFrame,
@Nullable LinkedHashSet<String> knownGlobalVariables) {
GlobalFrame globalFrame) {
this.continuation = continuation;
this.function = function;
this.caller = caller;
this.lexicalFrame = lexicalFrame;
this.globalFrame = globalFrame;
this.knownGlobalVariables = knownGlobalVariables;
}
}

Expand Down Expand Up @@ -766,15 +757,6 @@ public int hashCode() {
*/
private final Map<String, Extension> 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<String> 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.
Expand All @@ -801,20 +783,16 @@ 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 */
void exitScope() {
Preconditions.checkNotNull(continuation);
lexicalFrame = continuation.lexicalFrame;
globalFrame = continuation.globalFrame;
knownGlobalVariables = continuation.knownGlobalVariables;
continuation = continuation.continuation;
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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. */
Expand All @@ -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;
}

/**
Expand All @@ -1209,16 +1165,6 @@ public Map<String, FlagGuardedValue> 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;
}
Expand Down
39 changes: 4 additions & 35 deletions src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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<String> 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);
}
Expand Down
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 2b3ad18

Please sign in to comment.