diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md index f157061b516804..fa8bae16756cf6 100644 --- a/site/docs/skylark/backward-compatibility.md +++ b/site/docs/skylark/backward-compatibility.md @@ -15,6 +15,7 @@ Full, authorative list of incompatible changes is [GitHub issues with General Starlark * [Dictionary concatenation](#dictionary-concatenation) +* [String escapes](#string-escapes) * [Load must appear at top of file](#load-must-appear-at-top-of-file) * [Depset is no longer iterable](#depset-is-no-longer-iterable) * [Depset union](#depset-union) @@ -74,6 +75,16 @@ with Python. A possible workaround is to use the `.update` method instead. * Default: `true` * Tracking issue: [#6461](https://github.com/bazelbuild/bazel/issues/6461) +### String escapes + +We are restricting unrecognized escape sequences. Trying to include escape +sequences like `\a`, `\b` or any other escape sequence that is unknown to +Starlark will result in a syntax error. + +* Flag: `--incompatible_restrict_escape_sequences` +* Default: `false` +* Tracking issue: [#8380](https://github.com/bazelbuild/bazel/issues/8380) + ### Load must appear at top of file Previously, the `load` statement could appear anywhere in a `.bzl` file so long diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index ab878fc6964881..1388b6774021c2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -612,6 +612,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "returns a depset instead.") public boolean incompatibleDepsetForLibrariesToLinkGetter; + @Option( + name = "incompatible_restrict_string_escapes", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If set to true, unknown string escapes like `\\a` become rejected.") + public boolean incompatibleRestrictStringEscapes; + /** Constructs a {@link StarlarkSemantics} object corresponding to this set of option values. */ public StarlarkSemantics toSkylarkSemantics() { return StarlarkSemantics.builder() @@ -662,6 +674,7 @@ public StarlarkSemantics toSkylarkSemantics() { .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) .incompatibleDoNotSplitLinkingCmdline(incompatibleDoNotSplitLinkingCmdline) .incompatibleDepsetForLibrariesToLinkGetter(incompatibleDepsetForLibrariesToLinkGetter) + .incompatibleRestrictStringEscapes(incompatibleRestrictStringEscapes) .build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java index da2ee67f84b7cb..fe334f1be2f113 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java @@ -541,6 +541,7 @@ private Extension createExtension( public static void execAndExport(BuildFileAST ast, Label extensionLabel, EventHandler eventHandler, com.google.devtools.build.lib.syntax.Environment extensionEnv) throws InterruptedException { + ast.replayLexerEvents(extensionEnv, eventHandler); ImmutableList statements = ast.getStatements(); for (Statement statement : statements) { ast.execTopLevelStatement(statement, extensionEnv, eventHandler); 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 8902c8d09275a9..fad5f40adf75b9 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 @@ -49,6 +49,8 @@ public class BuildFileAST extends ASTNode { */ private final boolean containsErrors; + private final List stringEscapeEvents; + @Nullable private final String contentHashCode; private BuildFileAST( @@ -57,13 +59,15 @@ private BuildFileAST( String contentHashCode, Location location, ImmutableList comments, - @Nullable ImmutableList imports) { + @Nullable ImmutableList imports, + List stringEscapeEvents) { this.statements = statements; this.containsErrors = containsErrors; this.contentHashCode = contentHashCode; this.comments = comments; this.setLocation(location); this.imports = imports; + this.stringEscapeEvents = stringEscapeEvents; } private static BuildFileAST create( @@ -98,7 +102,8 @@ private static BuildFileAST create( contentHashCode, result.location, ImmutableList.copyOf(result.comments), - skylarkImports.second); + skylarkImports.second, + result.stringEscapeEvents); } private static BuildFileAST create( @@ -135,7 +140,8 @@ public BuildFileAST subTree(int firstStatement, int lastStatement) { null, this.statements.get(firstStatement).getLocation(), ImmutableList.of(), - imports.build()); + imports.build(), + stringEscapeEvents); } /** @@ -202,6 +208,15 @@ public ImmutableList getRawImports() { return imports.build(); } + /** Returns true if there was no error event. */ + public boolean replayLexerEvents(Environment env, EventHandler eventHandler) { + if (env.getSemantics().incompatibleRestrictStringEscapes() && !stringEscapeEvents.isEmpty()) { + Event.replayEventsOn(eventHandler, stringEscapeEvents); + return false; + } + return true; + } + /** * Executes this build file in a given Environment. * @@ -371,7 +386,8 @@ public static BuildFileAST parseSkylarkFileWithoutImports( /* contentHashCode= */ null, result.location, ImmutableList.copyOf(result.comments), - /* imports= */ null); + /* imports= */ null, + result.stringEscapeEvents); } /** @@ -384,7 +400,7 @@ public BuildFileAST validate(Environment env, EventHandler eventHandler) { if (valid || containsErrors) { return this; } - return new BuildFileAST(statements, true, contentHashCode, getLocation(), comments, imports); + return new BuildFileAST(statements, true, contentHashCode, getLocation(), comments, imports, stringEscapeEvents); } public static BuildFileAST parseString(EventHandler eventHandler, String... content) { @@ -446,6 +462,7 @@ public static Object eval(Environment env, String... input) public static BuildFileAST parseAndValidateSkylarkString(Environment env, String[] input) throws EvalException { BuildFileAST ast = parseString(env.getEventHandler(), input); + ast.replayLexerEvents(env, env.getEventHandler()); ValidationEnvironment.validateAst(env, ast.getStatements()); return ast; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java index c5a5c1aa41aa8f..2989aa6686f72a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java @@ -103,6 +103,14 @@ private static class LocationInfo { private int dents; // number of saved INDENT (>0) or OUTDENT (<0) tokens to return + /** + * StringEscapeEvents contains the errors related to invalid escape sequences like "\a". + * This is not handled by the normal eventHandler. Instead, it is passed to the parser and + * then the AST. During the evaluation, we can decide to show the events based on a flag + * in StarlarkSemantics. This code is temporary, during the migration. + */ + private List stringEscapeEvents = new ArrayList<>(); + /** * Constructs a lexer which tokenizes the contents of the specified InputBuffer. Any errors during * lexing are reported on "handler". @@ -129,6 +137,10 @@ List getComments() { return comments; } + List getStringEscapeEvents() { + return stringEscapeEvents; + } + /** * Returns the filename from which the lexer's input came. Returns an empty value if the input * came from a string. @@ -457,10 +469,14 @@ private void escapedStringLiteral(char quot, boolean isRaw) { case 'v': case 'x': // exists in Python but not implemented in Blaze => error - error("escape sequence not implemented: \\" + c, literalStartPos, pos); + error("invalid escape sequence: \\" + c, literalStartPos, pos); break; default: // unknown char escape => "\literal" + stringEscapeEvents.add(Event.error( + createLocation(pos - 1, pos), "invalid escape sequence: \\" + c) + ); + literal.append('\\'); literal.append(c); break; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java index 497a5266cf01dd..8ee5449e4f1338 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java @@ -62,17 +62,21 @@ public static final class ParseResult { /** Whether the file contained any errors. */ public final boolean containsErrors; + public final List stringEscapeEvents; + public ParseResult( List statements, List comments, Location location, - boolean containsErrors) { + boolean containsErrors, + List stringEscapeEvents) { // No need to copy here; when the object is created, the parser instance is just about to go // out of scope and be garbage collected. this.statements = Preconditions.checkNotNull(statements); this.comments = Preconditions.checkNotNull(comments); this.location = location; this.containsErrors = containsErrors; + this.stringEscapeEvents = stringEscapeEvents; } } @@ -205,7 +209,7 @@ public static ParseResult parseFile(ParserInputSource input, EventHandler eventH } boolean errors = parser.errorsCount > 0 || lexer.containsErrors(); return new ParseResult( - statements, lexer.getComments(), locationFromStatements(lexer, statements), errors); + statements, lexer.getComments(), locationFromStatements(lexer, statements), errors, lexer.getStringEscapeEvents()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index b8763973e3ab0f..6136247d1b5aba 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -203,6 +203,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleDepsetForLibrariesToLinkGetter(); + public abstract boolean incompatibleRestrictStringEscapes(); + /** Returns a {@link Builder} initialized with the values of this instance. */ public abstract Builder toBuilder(); @@ -260,6 +262,7 @@ public static Builder builderWithDefaults() { .internalSkylarkFlagTestCanary(false) .incompatibleDoNotSplitLinkingCmdline(true) .incompatibleDepsetForLibrariesToLinkGetter(false) + .incompatibleRestrictStringEscapes(false) .build(); /** Builder for {@link StarlarkSemantics}. All fields are mandatory. */ @@ -352,6 +355,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo public abstract Builder incompatibleDepsetForLibrariesToLinkGetter(boolean value); + public abstract Builder incompatibleRestrictStringEscapes(boolean value); + public abstract StarlarkSemantics build(); } } 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 2f3c40104425b3..01ebea97756244 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 @@ -165,6 +165,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_restrict_named_params=" + rand.nextBoolean(), "--incompatible_static_name_resolution_in_build_files=" + rand.nextBoolean(), "--incompatible_string_join_requires_strings=" + rand.nextBoolean(), + "--incompatible_restrict_string_escapes=" + rand.nextBoolean(), "--internal_skylark_flag_test_canary=" + rand.nextBoolean()); } @@ -218,6 +219,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleRestrictNamedParams(rand.nextBoolean()) .incompatibleStaticNameResolutionInBuildFiles(rand.nextBoolean()) .incompatibleStringJoinRequiresStrings(rand.nextBoolean()) + .incompatibleRestrictStringEscapes(rand.nextBoolean()) .internalSkylarkFlagTestCanary(rand.nextBoolean()) .build(); } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index d3a853f99a516e..f07fc4fd6d8181 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -2994,4 +2994,28 @@ public void testRecursiveImport2() throws Exception { + "//test/skylark:ext3.bzl, //test/skylark:ext4.bzl]"); } } + + @Test + public void testUnknownStringEscapesForbidden() throws Exception { + setSkylarkSemanticsOptions("--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')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:r"); + assertContainsEvent("invalid escape sequence: \\z"); + } + + @Test + public void testUnknownStringEscapes() throws Exception { + setSkylarkSemanticsOptions("--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"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java index 7226d1c2453bea..fe7af59cf8ef99 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java @@ -281,7 +281,7 @@ public void testStringEscapes() throws Exception { .isEqualTo("STRING(ab) NEWLINE EOF"); // escape end of line assertThat(values(tokens("\"ab\\ucd\""))).isEqualTo("STRING(abcd) NEWLINE EOF"); assertThat(lastError.toString()) - .isEqualTo("/some/path.txt:1: escape sequence not implemented: \\u"); + .isEqualTo("/some/path.txt:1: invalid escape sequence: \\u"); } @Test diff --git a/src/test/starlark/testdata/string_misc.sky b/src/test/starlark/testdata/string_misc.sky index 21ecc298ca6dee..904b9ab7b73dd6 100644 --- a/src/test/starlark/testdata/string_misc.sky +++ b/src/test/starlark/testdata/string_misc.sky @@ -148,3 +148,13 @@ assert_eq('a1'.isalpha(), False) assert_eq('a '.isalpha(), False) assert_eq('A'.isalpha(), True) assert_eq('AbZ'.isalpha(), True) + +# escape sequences +assert_eq("\"", '"') +--- +"\777" ### octal escape sequence out of range (maximum is \377) +--- +"\" ### unterminated string literal at eol +--- +""" ### unterminated string literal at eof +---