From c6b93b2db7fa20acf7af4be6cd3bfcead655dead Mon Sep 17 00:00:00 2001 From: Tae Hyung Kim Date: Tue, 18 Jul 2023 13:59:48 -0700 Subject: [PATCH] Relax syntax for gen-l10n (#130736) To preserve backward compatibility with the old parser which would ignore syntax errors, this PR introduces a way to treat the special characters `{` and `}` in the following way: 1. If we encounter a `{` which searching for a string token and this `{` is not followed by a valid placeholder, then we treat the `{` as a string and continue lexing for strings. 2. If we encounter a `}` while not within some expression (i.e. placeholders, arguments, plurals, or selects), then we treat the `}` as a string and continue lexing for strings. This makes it so that ``` "helloWorld": "{ } { placeholder }", "@@helloWorld": { "placeholders": { "placeholder" {} } } ``` treats the `{ }` as a string while `{ placeholder } ` is treated as a placeholder. Fixes https://github.com/flutter/flutter/issues/122404. --- .../src/commands/generate_localizations.dart | 7 +++ .../lib/src/localizations/gen_l10n.dart | 15 +++++- .../lib/src/localizations/gen_l10n_types.dart | 9 +++- .../localizations/localizations_utils.dart | 15 +++++- .../lib/src/localizations/message_parser.dart | 37 ++++++++++++-- .../generate_localizations_test.dart | 18 +++++++ .../general.shard/message_parser_test.dart | 48 +++++++++++++++++++ 7 files changed, 143 insertions(+), 6 deletions(-) diff --git a/packages/flutter_tools/lib/src/commands/generate_localizations.dart b/packages/flutter_tools/lib/src/commands/generate_localizations.dart index 452dc19a0bb..f7dd479fbe4 100644 --- a/packages/flutter_tools/lib/src/commands/generate_localizations.dart +++ b/packages/flutter_tools/lib/src/commands/generate_localizations.dart @@ -200,6 +200,13 @@ class GenerateLocalizationsCommand extends FlutterCommand { 'suppress-warnings', help: 'When specified, all warnings will be suppressed.\n' ); + argParser.addFlag( + 'relax-syntax', + help: 'When specified, the syntax will be relaxed so that the special character ' + '"{" is treated as a string if it is not followed by a valid placeholder ' + 'and "}" is treated as a string if it does not close any previous "{" ' + 'that is treated as a special character.', + ); } final FileSystem _fileSystem; diff --git a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart index 4288d5fdd16..84d37b5e33d 100644 --- a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart +++ b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart @@ -72,6 +72,7 @@ Future generateLocalizations({ useEscaping: options.useEscaping, logger: logger, suppressWarnings: options.suppressWarnings, + useRelaxedSyntax: options.relaxSyntax, ) ..loadResources() ..writeOutputFiles(isFromYaml: true, useCRLF: useCRLF); @@ -494,6 +495,7 @@ class LocalizationsGenerator { bool useEscaping = false, required Logger logger, bool suppressWarnings = false, + bool useRelaxedSyntax = false, }) { final Directory? projectDirectory = projectDirFromPath(fileSystem, projectPathString); final Directory inputDirectory = inputDirectoryFromPath(fileSystem, inputPathString, projectDirectory); @@ -517,6 +519,7 @@ class LocalizationsGenerator { useEscaping: useEscaping, logger: logger, suppressWarnings: suppressWarnings, + useRelaxedSyntax: useRelaxedSyntax, ); } @@ -541,6 +544,7 @@ class LocalizationsGenerator { required this.logger, this.useEscaping = false, this.suppressWarnings = false, + this.useRelaxedSyntax = false, }); final FileSystem _fs; @@ -617,6 +621,9 @@ class LocalizationsGenerator { /// from calling [_generateMethod]. bool hadErrors = false; + /// Whether to use relaxed syntax. + bool useRelaxedSyntax = false; + /// The list of all arb path strings in [inputDirectory]. List get arbPathStrings { return _allBundles.bundles.map((AppResourceBundle bundle) => bundle.file.path).toList(); @@ -908,7 +915,13 @@ class LocalizationsGenerator { } // The call to .toList() is absolutely necessary. Otherwise, it is an iterator and will call Message's constructor again. _allMessages = _templateBundle.resourceIds.map((String id) => Message( - _templateBundle, _allBundles, id, areResourceAttributesRequired, useEscaping: useEscaping, logger: logger, + _templateBundle, + _allBundles, + id, + areResourceAttributesRequired, + useEscaping: useEscaping, + logger: logger, + useRelaxedSyntax: useRelaxedSyntax, )).toList(); hadErrors = _allMessages.any((Message message) => message.hadErrors); if (inputsAndOutputsListFile != null) { diff --git a/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart b/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart index a978e437943..0ea3050e615 100644 --- a/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart +++ b/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart @@ -336,6 +336,7 @@ class Message { this.resourceId, bool isResourceAttributeRequired, { + this.useRelaxedSyntax = false, this.useEscaping = false, this.logger, } @@ -352,13 +353,18 @@ class Message { filenames[bundle.locale] = bundle.file.basename; final String? translation = bundle.translationFor(resourceId); messages[bundle.locale] = translation; + List? validPlaceholders; + if (useRelaxedSyntax) { + validPlaceholders = placeholders.entries.map((MapEntry e) => e.key).toList(); + } try { parsedMessages[bundle.locale] = translation == null ? null : Parser( resourceId, bundle.file.basename, translation, useEscaping: useEscaping, - logger: logger + placeholders: validPlaceholders, + logger: logger, ).parse(); } on L10nParserException catch (error) { logger?.printError(error.toString()); @@ -378,6 +384,7 @@ class Message { final Map parsedMessages; final Map placeholders; final bool useEscaping; + final bool useRelaxedSyntax; final Logger? logger; bool hadErrors = false; diff --git a/packages/flutter_tools/lib/src/localizations/localizations_utils.dart b/packages/flutter_tools/lib/src/localizations/localizations_utils.dart index 329f425a9bc..84bfa862516 100644 --- a/packages/flutter_tools/lib/src/localizations/localizations_utils.dart +++ b/packages/flutter_tools/lib/src/localizations/localizations_utils.dart @@ -354,6 +354,7 @@ class LocalizationOptions { bool? format, bool? useEscaping, bool? suppressWarnings, + bool? relaxSyntax, }) : templateArbFile = templateArbFile ?? 'app_en.arb', outputLocalizationFile = outputLocalizationFile ?? 'app_localizations.dart', outputClass = outputClass ?? 'AppLocalizations', @@ -363,7 +364,8 @@ class LocalizationOptions { nullableGetter = nullableGetter ?? true, format = format ?? false, useEscaping = useEscaping ?? false, - suppressWarnings = suppressWarnings ?? false; + suppressWarnings = suppressWarnings ?? false, + relaxSyntax = relaxSyntax ?? false; /// The `--arb-dir` argument. /// @@ -455,6 +457,16 @@ class LocalizationOptions { /// /// Whether or not to suppress warnings. final bool suppressWarnings; + + /// The `relax-syntax` argument. + /// + /// Whether or not to relax the syntax. When specified, the syntax will be + /// relaxed so that the special character "{" is treated as a string if it is + /// not followed by a valid placeholder and "}" is treated as a string if it + /// does not close any previous "{" that is treated as a special character. + /// This was added in for backward compatibility and is not recommended + /// as it may mask errors. + final bool relaxSyntax; } /// Parse the localizations configuration options from [file]. @@ -498,6 +510,7 @@ LocalizationOptions parseLocalizationsOptionsFromYAML({ format: _tryReadBool(yamlNode, 'format', logger), useEscaping: _tryReadBool(yamlNode, 'use-escaping', logger), suppressWarnings: _tryReadBool(yamlNode, 'suppress-warnings', logger), + relaxSyntax: _tryReadBool(yamlNode, 'relax-syntax', logger), ); } diff --git a/packages/flutter_tools/lib/src/localizations/message_parser.dart b/packages/flutter_tools/lib/src/localizations/message_parser.dart index 49a04fff485..6591d9da236 100644 --- a/packages/flutter_tools/lib/src/localizations/message_parser.dart +++ b/packages/flutter_tools/lib/src/localizations/message_parser.dart @@ -198,7 +198,8 @@ class Parser { this.messageString, { this.useEscaping = false, - this.logger + this.logger, + this.placeholders, } ); @@ -207,6 +208,7 @@ class Parser { final String filename; final bool useEscaping; final Logger? logger; + final List? placeholders; static String indentForError(int position) { return '${List.filled(position, ' ').join()}^'; @@ -216,12 +218,16 @@ class Parser { // every instance of "{" and "}" toggles the isString boolean and every // instance of "'" toggles the isEscaped boolean (and treats a double // single quote "''" as a single quote "'"). When !isString and !isEscaped - // delimit tokens by whitespace and special characters. + // delimit tokens by whitespace and special characters. When placeholders + // is passed, relax the syntax so that "{" and "}" can be used as strings in + // certain cases. List lexIntoTokens() { + final bool useRelaxedLexer = placeholders != null; final List tokens = []; bool isString = true; // Index specifying where to match from int startIndex = 0; + int depth = 0; // At every iteration, we should be able to match a new token until we // reach the end of the string. If for some reason we don't match a @@ -267,9 +273,28 @@ class Parser { } match = brace.matchAsPrefix(messageString, startIndex); if (match != null) { + final String matchedBrace = match.group(0)!; + if (useRelaxedLexer) { + final Match? whitespaceMatch = whitespace.matchAsPrefix(messageString, match.end); + final int endOfWhitespace = whitespaceMatch?.group(0) == null ? match.end : whitespaceMatch!.end; + final Match? identifierMatch = alphanumeric.matchAsPrefix(messageString, endOfWhitespace); + // If we match a "}" and the depth is 0, treat it as a string. + // If we match a "{" and the next token is not a valid placeholder, treat it as a string. + if (matchedBrace == '}' && depth == 0) { + tokens.add(Node.string(startIndex, matchedBrace)); + startIndex = match.end; + continue; + } + if (matchedBrace == '{' && (identifierMatch == null || !placeholders!.contains(identifierMatch.group(0)))) { + tokens.add(Node.string(startIndex, matchedBrace)); + startIndex = match.end; + continue; + } + } tokens.add(Node.brace(startIndex, match.group(0)!)); isString = false; startIndex = match.end; + depth += 1; continue; } // Theoretically, we only reach this point because of unmatched single quotes because @@ -299,9 +324,15 @@ class Parser { if (match == null) { match = brace.matchAsPrefix(messageString, startIndex); if (match != null) { - tokens.add(Node.brace(startIndex, match.group(0)!)); + final String matchedBrace = match.group(0)!; + tokens.add(Node.brace(startIndex, matchedBrace)); isString = true; startIndex = match.end; + if (matchedBrace == '{') { + depth += 1; + } else { + depth -= 1; + } continue; } // This should only happen when there are special characters we are unable to match. diff --git a/packages/flutter_tools/test/general.shard/generate_localizations_test.dart b/packages/flutter_tools/test/general.shard/generate_localizations_test.dart index 67cc9667a27..fa35cfd510b 100644 --- a/packages/flutter_tools/test/general.shard/generate_localizations_test.dart +++ b/packages/flutter_tools/test/general.shard/generate_localizations_test.dart @@ -95,6 +95,7 @@ void main() { bool useEscaping = false, bool areResourceAttributeRequired = false, bool suppressWarnings = false, + bool relaxSyntax = false, void Function(Directory)? setup, } ) { @@ -126,6 +127,7 @@ void main() { useEscaping: useEscaping, areResourceAttributesRequired: areResourceAttributeRequired, suppressWarnings: suppressWarnings, + useRelaxedSyntax: relaxSyntax, ) ..loadResources() ..writeOutputFiles(isFromYaml: isFromYaml); @@ -1475,6 +1477,22 @@ import 'output-localization-file_en.dart' deferred as output-localization-file_e expect(getGeneratedFileContent(locale: 'en'), contains('String helloWorld(Object name) {')); expect(getGeneratedFileContent(locale: 'es'), contains('String helloWorld(Object name) {')); }); + + testWithoutContext('braces are ignored as special characters if placeholder does not exist', () { + setupLocalizations({ + 'en': ''' +{ + "helloWorld": "Hello {name}", + "@@helloWorld": { + "placeholders": { + "names": {} + } + } +}''' + }, relaxSyntax: true); + final String content = getGeneratedFileContent(locale: 'en'); + expect(content, contains("String get helloWorld => 'Hello {name}'")); + }); }); group('DateTime tests', () { diff --git a/packages/flutter_tools/test/general.shard/message_parser_test.dart b/packages/flutter_tools/test/general.shard/message_parser_test.dart index efc9680adf2..3b716cd3c05 100644 --- a/packages/flutter_tools/test/general.shard/message_parser_test.dart +++ b/packages/flutter_tools/test/general.shard/message_parser_test.dart @@ -293,6 +293,54 @@ void main() { ))); }); + testWithoutContext('relaxed lexer', () { + final List tokens1 = Parser( + 'string', + 'app_en.arb', + '{ }', + placeholders: [], + ).lexIntoTokens(); + expect(tokens1, equals([ + Node(ST.string, 0, value: '{'), + Node(ST.string, 1, value: ' '), + Node(ST.string, 2, value: '}') + ])); + + final List tokens2 = Parser( + 'string', + 'app_en.arb', + '{ notAPlaceholder }', + placeholders: ['isAPlaceholder'], + ).lexIntoTokens(); + expect(tokens2, equals([ + Node(ST.string, 0, value: '{'), + Node(ST.string, 1, value: ' notAPlaceholder '), + Node(ST.string, 18, value: '}') + ])); + + final List tokens3 = Parser( + 'string', + 'app_en.arb', + '{ isAPlaceholder }', + placeholders: ['isAPlaceholder'], + ).lexIntoTokens(); + expect(tokens3, equals([ + Node(ST.openBrace, 0, value: '{'), + Node(ST.identifier, 2, value: 'isAPlaceholder'), + Node(ST.closeBrace, 17, value: '}') + ])); + }); + + testWithoutContext('relaxed lexer complex', () { + const String message = '{ notPlaceholder } {count,plural, =0{Hello} =1{Hello World} =2{Hello two worlds} few{Hello {count} worlds} many{Hello all {count} worlds} other{Hello other {count} worlds}}'; + final List tokens = Parser( + 'string', + 'app_en.arb', + message, + placeholders: ['count'], + ).lexIntoTokens(); + expect(tokens[0].type, equals(ST.string)); + }); testWithoutContext('parser basic', () { expect(Parser('helloWorld', 'app_en.arb', 'Hello {name}').parse(), equals(