Skip to content

Commit

Permalink
Remove the flag --incompatible_restrict_string_escapes
Browse files Browse the repository at this point in the history
This allows us to simplify the code and remove the options from the lexer and parser.

#8380

PiperOrigin-RevId: 404799093
  • Loading branch information
laurentlb authored and copybara-github committed Oct 21, 2021
1 parent 912ef5d commit 21b4b1a
Show file tree
Hide file tree
Showing 15 changed files with 18 additions and 92 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -494,15 +494,6 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ "returns a depset instead.")
public boolean incompatibleDepsetForLibrariesToLinkGetter;

@Option(
name = "incompatible_restrict_string_escapes",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help = "If set to true, unknown string escapes like `\\a` become rejected.")
public boolean incompatibleRestrictStringEscapes;

@Option(
name = "incompatible_linkopts_to_linklibs",
defaultValue = "true",
Expand Down Expand Up @@ -618,7 +609,6 @@ public StarlarkSemantics toStarlarkSemantics() {
INCOMPATIBLE_DEPSET_FOR_LIBRARIES_TO_LINK_GETTER,
incompatibleDepsetForLibrariesToLinkGetter)
.setBool(INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API, incompatibleRequireLinkerInputCcApi)
.setBool(INCOMPATIBLE_RESTRICT_STRING_ESCAPES, incompatibleRestrictStringEscapes)
.setBool(INCOMPATIBLE_LINKOPTS_TO_LINKLIBS, incompatibleLinkoptsToLinklibs)
.set(MAX_COMPUTATION_STEPS, maxComputationSteps)
.set(NESTED_SET_DEPTH_LIMIT, nestedSetDepthLimit)
Expand Down Expand Up @@ -683,8 +673,6 @@ public StarlarkSemantics toStarlarkSemantics() {
"-incompatible_no_rule_outputs_param";
public static final String INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API =
"+incompatible_require_linker_input_cc_api";
public static final String INCOMPATIBLE_RESTRICT_STRING_ESCAPES =
"+incompatible_restrict_string_escapes";
public static final String INCOMPATIBLE_RUN_SHELL_COMMAND_STRING =
"+incompatible_run_shell_command_string";
public static final String INCOMPATIBLE_STRUCT_HAS_NO_METHODS =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.RootedPath;
Expand Down Expand Up @@ -167,8 +166,6 @@ static BzlCompileValue computeInline(
// statements whose bindings are intended to be visible in all BUILD
// files. The loadBindsGlobally flag allows us to retrieve them.
.loadBindsGlobally(key.isBuildPrelude())
.restrictStringEscapes(
semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES))
.build();
StarlarkFile file = StarlarkFile.parse(input, options);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1456,8 +1456,6 @@ private CompiledBuildFile compileBuildFile(
// as in .bzl files. One can always use a renaming load statement.
.loadBindsGlobally(true)
.allowToplevelRebinding(true)
.restrictStringEscapes(
semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES))
.build();

// parse
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import com.google.devtools.build.lib.packages.WorkspaceFactory;
import com.google.devtools.build.lib.packages.WorkspaceFileValue;
import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.rules.repository.ResolvedFileValue;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
Expand Down Expand Up @@ -136,9 +135,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
// WORKSPACE files followed BUILD norms, but this should
// probably be flipped.
.allowToplevelRebinding(true)
.restrictStringEscapes(
starlarkSemantics.getBool(
BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES))
.build();

// Accumulate workspace files (prefix + main + suffix).
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/starlark/java/eval/Starlark.java
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@ public static Object eval(
*/
private static StarlarkFunction newExprFunction(
ParserInput input, FileOptions options, Module module) throws SyntaxError.Exception {
Expression expr = Expression.parse(input, options);
Expression expr = Expression.parse(input);
Program prog = Program.compileExpr(expr, module, options);
Resolver.Function rfn = prog.getResolvedFunction();
int[] globalIndex = module.getIndicesOfGlobals(rfn.getGlobals()); // see execFileProgram
Expand Down
10 changes: 2 additions & 8 deletions src/main/java/net/starlark/java/syntax/Expression.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,9 @@ public enum Kind {
*/
public abstract Kind kind();

/** Parses an expression with the default options. */
public static Expression parse(ParserInput input) throws SyntaxError.Exception {
return parse(input, FileOptions.DEFAULT);
}

/** Parses an expression. */
public static Expression parse(ParserInput input, FileOptions options)
throws SyntaxError.Exception {
return Parser.parseExpression(input, options);
public static Expression parse(ParserInput input) throws SyntaxError.Exception {
return Parser.parseExpression(input);
}

}
12 changes: 0 additions & 12 deletions src/main/java/net/starlark/java/syntax/FileOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,6 @@ public abstract class FileOptions {
/** The default options for Starlark static processing. New clients should use these defaults. */
public static final FileOptions DEFAULT = builder().build();

// Options are presented in phase order: scanner, parser, resolver, compiler.

// --- scanner options ---

/** Disallow ineffective escape sequences such as {@code \a} when scanning string literals. */
public abstract boolean restrictStringEscapes();

// --- resolver options ---

/**
* During resolution, permit load statements to access private names such as {@code _x}. <br>
* (Required for continued support of Bazel "WORKSPACE.resolved" files.)
Expand Down Expand Up @@ -82,7 +73,6 @@ public abstract class FileOptions {
public static Builder builder() {
// These are the DEFAULT values.
return new AutoValue_FileOptions.Builder()
.restrictStringEscapes(true)
.allowLoadPrivateSymbols(false)
.allowToplevelRebinding(false)
.loadBindsGlobally(false)
Expand All @@ -95,8 +85,6 @@ public static Builder builder() {
@AutoValue.Builder
public abstract static class Builder {
// AutoValue why u make me say it 3 times?
public abstract Builder restrictStringEscapes(boolean value);

public abstract Builder allowLoadPrivateSymbols(boolean value);

public abstract Builder allowToplevelRebinding(boolean value);
Expand Down
18 changes: 2 additions & 16 deletions src/main/java/net/starlark/java/syntax/Lexer.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ final class Lexer {
private final char[] buffer;
private int pos;

private final FileOptions options;

// The stack of enclosing indentation levels in spaces.
// The first (outermost) element is always zero.
private final Stack<Integer> indentStack = new Stack<>();
Expand Down Expand Up @@ -93,9 +91,8 @@ final class Lexer {

// Constructs a lexer which tokenizes the parser input.
// Errors are appended to errors.
Lexer(ParserInput input, FileOptions options, List<SyntaxError> errors) {
Lexer(ParserInput input, List<SyntaxError> errors) {
this.locs = FileLocations.create(input.getContent(), input.getFile());
this.options = options;
this.buffer = input.getContent();
this.pos = 0;
this.errors = errors;
Expand Down Expand Up @@ -362,20 +359,9 @@ private void escapedStringLiteral(char quot, boolean isRaw) {
case 'u':
case 'U':
case 'v':
case 'x':
// exists in Python but not implemented in Blaze => error
error("invalid escape sequence: \\" + c, pos - 1);
break;
default:
// unknown char escape => "\literal"
if (options.restrictStringEscapes()) {
error(
"invalid escape sequence: \\"
+ c
+ ". You can enable unknown escape sequences by passing the flag"
+ " --incompatible_restrict_string_escapes=false",
pos - 1);
}
error("invalid escape sequence: \\" + c + ". Use '\\\\' to insert '\\'.", pos - 1);
literal.append('\\');
literal.append(c);
break;
Expand Down
9 changes: 4 additions & 5 deletions src/main/java/net/starlark/java/syntax/Parser.java
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ private static String tokenString(TokenKind kind, @Nullable Object value) {
}

// Main entry point for parsing a file.
static ParseResult parseFile(ParserInput input, FileOptions options) {
static ParseResult parseFile(ParserInput input) {
List<SyntaxError> errors = new ArrayList<>();
Lexer lexer = new Lexer(input, options, errors);
Lexer lexer = new Lexer(input, errors);
Parser parser = new Parser(lexer, errors);

StarlarkFile.ParseProfiler profiler = Parser.profiler;
Expand Down Expand Up @@ -207,10 +207,9 @@ private void parseStatement(ImmutableList.Builder<Statement> list) {
}

/** Parses an expression, possibly followed by newline tokens. */
static Expression parseExpression(ParserInput input, FileOptions options)
throws SyntaxError.Exception {
static Expression parseExpression(ParserInput input) throws SyntaxError.Exception {
List<SyntaxError> errors = new ArrayList<>();
Lexer lexer = new Lexer(input, options, errors);
Lexer lexer = new Lexer(input, errors);
Parser parser = new Parser(lexer, errors);
Expression result = null;
try {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/starlark/java/syntax/StarlarkFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ public Resolver.Function getResolvedFunction() {
* </pre>
*/
public static StarlarkFile parse(ParserInput input, FileOptions options) {
Parser.ParseResult result = Parser.parseFile(input, options);
Parser.ParseResult result = Parser.parseFile(input);
return new StarlarkFile(
result.locs, result.statements, options, result.comments, result.errors);
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/starlark/java/syntax/StringLiteral.java
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public static String unquote(String s) {
// independent of the Lexer, which should only validate string literals
// but not unquote them. Clients (e.g. the compiler) can unquote on demand.
ArrayList<SyntaxError> errors = new ArrayList<>();
Lexer lexer = new Lexer(ParserInput.fromLines(s), FileOptions.DEFAULT, errors);
Lexer lexer = new Lexer(ParserInput.fromLines(s), errors);
lexer.nextToken();
if (!errors.isEmpty()) {
throw new IllegalArgumentException(errors.get(0).message());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
"--incompatible_struct_has_no_methods=" + rand.nextBoolean(),
"--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(),
"--incompatible_require_linker_input_cc_api=" + rand.nextBoolean(),
"--incompatible_restrict_string_escapes=" + rand.nextBoolean(),
"--incompatible_use_cc_configure_from_rules_cc=" + rand.nextBoolean(),
"--internal_starlark_flag_test_canary=" + rand.nextBoolean(),
"--max_computation_steps=" + rand.nextLong());
Expand Down Expand Up @@ -197,7 +196,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION,
rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES, rand.nextBoolean())
.setBool(
BuildLanguageOptions.INCOMPATIBLE_USE_CC_CONFIGURE_FROM_RULES_CC, rand.nextBoolean())
.setBool(StarlarkSemantics.PRINT_TEST_MARKER, rand.nextBoolean())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3387,8 +3387,6 @@ public void testDictGetUnhashableForbidden() throws Exception {

@Test
public void testUnknownStringEscapesForbidden() throws Exception {
setBuildLanguageOptions("--incompatible_restrict_string_escapes=true");

scratch.file("test/extension.bzl", "y = \"\\z\"");

scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");
Expand All @@ -3398,17 +3396,6 @@ public void testUnknownStringEscapesForbidden() throws Exception {
assertContainsEvent("invalid escape sequence: \\z");
}

@Test
public void testUnknownStringEscapes() throws Exception {
setBuildLanguageOptions("--incompatible_restrict_string_escapes=false");

scratch.file("test/extension.bzl", "y = \"\\z\"");

scratch.file("test/BUILD", "load('//test:extension.bzl', 'y')", "cc_library(name = 'r')");

getConfiguredTarget("//test:r");
}

@Test
public void testSplitEmptySeparatorForbidden() throws Exception {
scratch.file("test/extension.bzl", "y = 'abc'.split('')");
Expand Down
17 changes: 6 additions & 11 deletions src/test/java/net/starlark/java/syntax/LexerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class LexerTest {
private Lexer createLexer(String input) {
ParserInput inputSource = ParserInput.fromString(input, "");
errors.clear();
return new Lexer(inputSource, FileOptions.DEFAULT, errors);
return new Lexer(inputSource, errors);
}

private static class Token {
Expand Down Expand Up @@ -357,18 +357,16 @@ public void testStringEscapes() throws Exception {
checkErrors(
"'x\\hx'", //
"STRING(x\\hx) NEWLINE EOF",
" ^ invalid escape sequence: \\h. You can enable unknown escape sequences by passing the"
+ " flag --incompatible_restrict_string_escapes=false");
" ^ invalid escape sequence: \\h. Use '\\\\' to insert '\\'.");
checkErrors(
"'\\$$'", //
"STRING(\\$$) NEWLINE EOF",
" ^ invalid escape sequence: \\$. You can enable unknown escape sequences by passing the"
+ " flag --incompatible_restrict_string_escapes=false");
" ^ invalid escape sequence: \\$. Use '\\\\' to insert '\\'.");
check("'a\\\nb'", "STRING(ab) NEWLINE EOF"); // escape end of line
checkErrors(
"\"ab\\ucd\"", //
"STRING(abcd) NEWLINE EOF",
" ^ invalid escape sequence: \\u");
"STRING(ab\\ucd) NEWLINE EOF",
" ^ invalid escape sequence: \\u. Use '\\\\' to insert '\\'.");
}

@Test
Expand Down Expand Up @@ -645,10 +643,7 @@ public void testStringLiteralUnquote() {
assertUnquoteError("'", "unclosed string literal");
assertUnquoteError("\"", "unclosed string literal");
assertUnquoteError("'abc", "unclosed string literal");
assertUnquoteError(
"'\\g'",
"invalid escape sequence: \\g. You can enable unknown escape sequences by passing the flag"
+ " --incompatible_restrict_string_escapes=false"); // this temporary hint is a lie
assertUnquoteError("'\\g'", "invalid escape sequence: \\g. Use '\\\\' to insert '\\'.");
}

private static void assertUnquoteEquals(String literal, String value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ public void comment() throws SyntaxError.Exception {
ParserInput.fromLines(
"# foo", //
"expr # bar");
Parser.ParseResult r = Parser.parseFile(input, FileOptions.DEFAULT);
Parser.ParseResult r = Parser.parseFile(input);
Comment c0 = r.comments.get(0);
assertIndentedPrettyMatches(c0, " # foo");
assertTostringMatches(c0, "# foo");
Expand Down

1 comment on commit 21b4b1a

@davidxia
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laurentlb I noticed this breaking change wasn't in CHANGELOG and confused me. I made a PR for that here. If that's helpful, PTAL and merge. 🙏

Please sign in to comment.