diff options
author | laurentlb <laurentlb@google.com> | 2017-06-12 15:22:37 +0200 |
---|---|---|
committer | Yun Peng <pcloudy@google.com> | 2017-06-12 17:39:57 +0200 |
commit | bde7c41c8b93c0f36806fed649021784700aaedd (patch) | |
tree | b07b2198af18771b285a22a15476e84f5933a761 /src/main/java/com/google | |
parent | d333e811f9a30f74375295cf12afaf3b16a1de7a (diff) |
Cleanup in ValidationEnvironment, provide static methods, reduce visibility.
Call sites were creating a ValidationEnvironment object with no other purpose
than calling validateAst(). Simplify the code so that callers don't have to
worry about it.
RELNOTES: None.
PiperOrigin-RevId: 158705853
Diffstat (limited to 'src/main/java/com/google')
3 files changed, 42 insertions, 57 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java index d3a75d8cfd..30a9c08b16 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java @@ -21,7 +21,6 @@ import com.google.devtools.build.lib.syntax.BuildFileAST; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkSemanticsOptions; -import com.google.devtools.build.lib.syntax.ValidationEnvironment; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; @@ -111,19 +110,18 @@ public class ASTFileLookupFunction implements SkyFunction { try { long astFileSize = fileValue.getSize(); try (Mutability mutability = Mutability.create("validate")) { - ValidationEnvironment validationEnv = - new ValidationEnvironment( - ruleClassProvider - .createSkylarkRuleClassEnvironment( - fileLabel, - mutability, - skylarkSemantics, - env.getListener(), - // the two below don't matter for extracting the ValidationEnvironment: - /*astFileContentHashCode=*/ null, - /*importMap=*/ null) - .setupDynamic(Runtime.PKG_NAME, Runtime.NONE) - .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE)); + com.google.devtools.build.lib.syntax.Environment validationEnv = + ruleClassProvider + .createSkylarkRuleClassEnvironment( + fileLabel, + mutability, + skylarkSemantics, + env.getListener(), + // the two below don't matter for extracting the ValidationEnvironment: + /*astFileContentHashCode=*/ null, + /*importMap=*/ null) + .setupDynamic(Runtime.PKG_NAME, Runtime.NONE) + .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE); ast = BuildFileAST.parseSkylarkFile(path, astFileSize, env.getListener()); ast = ast.validate(validationEnv, env.getListener()); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java index b1f079a14b..44f1c158d5 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java @@ -315,8 +315,8 @@ public class BuildFileAST extends ASTNode { * * @return a new AST (or the same), with the containsErrors flag updated. */ - public BuildFileAST validate(ValidationEnvironment validationEnv, EventHandler eventHandler) { - boolean valid = validationEnv.validateAst(stmts, eventHandler); + public BuildFileAST validate(Environment env, EventHandler eventHandler) { + boolean valid = ValidationEnvironment.validateAst(env, stmts, eventHandler); if (valid || containsErrors) { return this; } @@ -384,8 +384,7 @@ public class BuildFileAST extends ASTNode { public static BuildFileAST parseAndValidateSkylarkString(Environment env, String[] input) throws EvalException { BuildFileAST ast = parseSkylarkString(env.getEventHandler(), input); - ValidationEnvironment valid = new ValidationEnvironment(env); - valid.validateAst(ast.getStatements()); + ValidationEnvironment.validateAst(env, ast.getStatements()); return ast; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java index b66e0d6528..f06c15fa6c 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java @@ -47,10 +47,8 @@ public final class ValidationEnvironment { // branches of if-else statements. private final Stack<Set<String>> futureReadOnlyVariables = new Stack<>(); - /** - * Create a ValidationEnvironment for a given global Environment. - */ - public ValidationEnvironment(Environment env) { + /** Create a ValidationEnvironment for a given global Environment. */ + ValidationEnvironment(Environment env) { Preconditions.checkArgument(env.isGlobal()); parent = null; Set<String> builtinVariables = env.getVariableNames(); @@ -59,27 +57,20 @@ public final class ValidationEnvironment { semantics = env.getSemantics(); } - /** - * Creates a local ValidationEnvironment to validate user defined function bodies. - */ - public ValidationEnvironment(ValidationEnvironment parent) { + /** Creates a local ValidationEnvironment to validate user defined function bodies. */ + ValidationEnvironment(ValidationEnvironment parent) { // Don't copy readOnlyVariables: Variables may shadow global values. this.parent = parent; semantics = parent.semantics; } - /** - * Returns true if this ValidationEnvironment is top level i.e. has no parent. - */ - public boolean isTopLevel() { + /** Returns true if this ValidationEnvironment is top level i.e. has no parent. */ + boolean isTopLevel() { return parent == null; } - /** - * Declare a variable and add it to the environment. - */ - public void declare(String varname, Location location) - throws EvalException { + /** Declare a variable and add it to the environment. */ + void declare(String varname, Location location) throws EvalException { checkReadonly(varname, location); if (parent == null) { // top-level values are immutable readOnlyVariables.add(varname); @@ -101,16 +92,14 @@ public final class ValidationEnvironment { } } - /** - * Returns true if the symbol exists in the validation environment. - */ - public boolean hasSymbolInEnvironment(String varname) { + /** Returns true if the symbol exists in the validation environment. */ + boolean hasSymbolInEnvironment(String varname) { return variables.contains(varname) || (parent != null && topLevel().variables.contains(varname)); } /** Returns the set of all accessible symbols (both local and global) */ - public Set<String> getAllSymbols() { + Set<String> getAllSymbols() { Set<String> all = new HashSet<>(); all.addAll(variables); if (parent != null) { @@ -128,14 +117,12 @@ public final class ValidationEnvironment { * This is useful to validate control flows like if-else when we know that certain parts of the * code cannot both be executed. */ - public void startTemporarilyDisableReadonlyCheckSession() { + void startTemporarilyDisableReadonlyCheckSession() { futureReadOnlyVariables.add(new HashSet<String>()); } - /** - * Finishes the session with temporarily disabled readonly checking. - */ - public void finishTemporarilyDisableReadonlyCheckSession() { + /** Finishes the session with temporarily disabled readonly checking. */ + void finishTemporarilyDisableReadonlyCheckSession() { Set<String> variables = futureReadOnlyVariables.pop(); readOnlyVariables.addAll(variables); if (!futureReadOnlyVariables.isEmpty()) { @@ -143,15 +130,13 @@ public final class ValidationEnvironment { } } - /** - * Finishes a branch of temporarily disabled readonly checking. - */ - public void finishTemporarilyDisableReadonlyCheckBranch() { + /** Finishes a branch of temporarily disabled readonly checking. */ + void finishTemporarilyDisableReadonlyCheckBranch() { readOnlyVariables.removeAll(futureReadOnlyVariables.peek()); } /** Throws EvalException if a load() appears after another kind of statement. */ - private void checkLoadAfterStatement(List<Statement> statements) throws EvalException { + private static void checkLoadAfterStatement(List<Statement> statements) throws EvalException { Location firstStatement = null; for (Statement statement : statements) { @@ -181,7 +166,7 @@ public final class ValidationEnvironment { } /** Throws EvalException if a `if` statement appears at the top level. */ - private void checkToplevelIfStatement(List<Statement> statements) throws EvalException { + private static void checkToplevelIfStatement(List<Statement> statements) throws EvalException { for (Statement statement : statements) { if (statement instanceof IfStatement) { throw new EvalException( @@ -194,10 +179,8 @@ public final class ValidationEnvironment { } } - /** - * Validates the AST and runs static checks. - */ - public void validateAst(List<Statement> statements) throws EvalException { + /** Validates the AST and runs static checks. */ + void validateAst(List<Statement> statements) throws EvalException { // Check that load() statements are on top. if (semantics.incompatibleBzlDisallowLoadAfterStatement) { checkLoadAfterStatement(statements); @@ -223,9 +206,14 @@ public final class ValidationEnvironment { } } - public boolean validateAst(List<Statement> statements, EventHandler eventHandler) { + public static void validateAst(Environment env, List<Statement> statements) throws EvalException { + new ValidationEnvironment(env).validateAst(statements); + } + + public static boolean validateAst( + Environment env, List<Statement> statements, EventHandler eventHandler) { try { - validateAst(statements); + validateAst(env, statements); return true; } catch (EvalException e) { if (!e.isDueToIncompleteAST()) { |