[gen_l10n] Improvements to `gen_l10n` (#116202)

* init

* fix tests

* fix lint

* extra changes

* oops missed some merge conflicts

* fix lexer add tests

* consistent warnings and errors

* throw error at the end

* improve efficiency, improve code generation

* fix

* nit

* fix test

* remove helper method class

* two d's

* oops

* empty commit as google testing won't pass :(

packages/flutter_tools/lib/src/commands/generate_localizations.dart

@@ -201,6 +201,10 @@ class GenerateLocalizationsCommand extends FlutterCommand {
             'contained within pairs of single quotes as normal strings and treat all '
             'consecutive pairs of single quotes as a single quote character.',
     );
+    argParser.addFlag(
+      'suppress-warnings',
+      help: 'When specified, all warnings will be suppressed.\n'
+    );
   }
 
   final FileSystem _fileSystem;
@@ -258,6 +262,7 @@ class GenerateLocalizationsCommand extends FlutterCommand {
       final bool areResourceAttributesRequired = boolArgDeprecated('required-resource-attributes');
       final bool usesNullableGetter = boolArgDeprecated('nullable-getter');
       final bool useEscaping = boolArgDeprecated('use-escaping');
+      final bool suppressWarnings = boolArgDeprecated('suppress-warnings');
 
       precacheLanguageAndRegionTags();
 
@@ -281,6 +286,7 @@ class GenerateLocalizationsCommand extends FlutterCommand {
           usesNullableGetter: usesNullableGetter,
           useEscaping: useEscaping,
           logger: _logger,
+          suppressWarnings: suppressWarnings,
         )
           ..loadResources()
           ..writeOutputFiles())

packages/flutter_tools/lib/src/localizations/gen_l10n_templates.dart

@@ -139,33 +139,23 @@ const String methodTemplate = '''
   String @(name)(@(parameters)) {
 @(dateFormatting)
 @(numberFormatting)
-@(helperMethods)
-    return @(message);
+@(tempVars)    return @(message);
   }''';
 
-const String messageHelperTemplate = '''
-    String @(name)(@(parameters)) {
-      return @(message);
-    }''';
-
-const String pluralHelperTemplate = '''
-    String @(name)(@(parameters)) {
-      return intl.Intl.pluralLogic(
-        @(count),
-        locale: localeName,
+const String pluralVariableTemplate = '''
+    String @(varName) = intl.Intl.pluralLogic(
+      @(count),
+      locale: localeName,
 @(pluralLogicArgs)
-      );
-    }''';
-
-const String selectHelperTemplate = '''
-    String @(name)(@(parameters)) {
-      return intl.Intl.selectLogic(
-        @(choice),
-        {
+    );''';
+
+const String selectVariableTemplate = '''
+    String @(varName) = intl.Intl.selectLogic(
+      @(choice),
+      {
 @(selectCases)
-        },
-      );
-    }''';
+      },
+    );''';
 
 const String classFileTemplate = '''
 @(header)@(requiresIntlImport)import '@(fileName)';

packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart

@@ -5,6 +5,7 @@
 import 'package:intl/locale.dart';
 
 import '../base/file_system.dart';
+import '../base/logger.dart';
 import '../convert.dart';
 import 'localizations_utils.dart';
 import 'message_parser.dart';
@@ -138,17 +139,31 @@ class L10nParserException extends L10nException {
     this.messageString,
     this.charNumber
   ): super('''
-$error
-[$fileName:$messageId] $messageString
-${List<String>.filled(4 + fileName.length + messageId.length + charNumber, ' ').join()}^''');
+[$fileName:$messageId] $error
+    $messageString
+    ${List<String>.filled(charNumber, ' ').join()}^''');
 
   final String error;
   final String fileName;
   final String messageId;
   final String messageString;
+  // Position of character within the "messageString" where the error is.
   final int charNumber;
 }
 
+class L10nMissingPlaceholderException extends L10nParserException {
+  L10nMissingPlaceholderException(
+    super.error,
+    super.fileName,
+    super.messageId,
+    super.messageString,
+    super.charNumber,
+    this.placeholderName,
+  );
+
+  final String placeholderName;
+}
+
 // One optional named parameter to be used by a NumberFormat.
 //
 // Some of the NumberFormat factory constructors have optional named parameters.
@@ -319,7 +334,10 @@ class Message {
     AppResourceBundleCollection allBundles,
     this.resourceId,
     bool isResourceAttributeRequired,
-    { this.useEscaping = false }
+    {
+      this.useEscaping = false,
+      this.logger,
+    }
   ) : assert(templateBundle != null),
       assert(allBundles != null),
       assert(resourceId != null && resourceId.isNotEmpty),
@@ -335,64 +353,16 @@ class Message {
       filenames[bundle.locale] = bundle.file.basename;
       final String? translation = bundle.translationFor(resourceId);
       messages[bundle.locale] = translation;
-      parsedMessages[bundle.locale] = translation == null ? null : Parser(resourceId, bundle.file.basename, translation, useEscaping: useEscaping).parse();
-    }
-    // Using parsed translations, attempt to infer types of placeholders used by plurals and selects.
-    for (final LocaleInfo locale in parsedMessages.keys) {
-      if (parsedMessages[locale] == null) {
-        continue;
-      }
-      final List<Node> traversalStack = <Node>[parsedMessages[locale]!];
-      while (traversalStack.isNotEmpty) {
-        final Node node = traversalStack.removeLast();
-        if (node.type == ST.pluralExpr) {
-          final Placeholder? placeholder = placeholders[node.children[1].value!];
-          if (placeholder == null) {
-            throw L10nParserException(
-              'Make sure that the specified plural placeholder is defined in your arb file.',
-              filenames[locale]!,
-              resourceId,
-              messages[locale]!,
-              node.children[1].positionInMessage
-            );
-          }
-          placeholders[node.children[1].value!]!.isPlural = true;
-        }
-        if (node.type == ST.selectExpr) {
-          final Placeholder? placeholder = placeholders[node.children[1].value!];
-          if (placeholder == null) {
-            throw L10nParserException(
-              'Make sure that the specified select placeholder is defined in your arb file.',
-              filenames[locale]!,
-              resourceId,
-              messages[locale]!,
-              node.children[1].positionInMessage
-            );
-          }
-          placeholders[node.children[1].value!]!.isSelect = true;
-        }
-        traversalStack.addAll(node.children);
-      }
-    }
-    for (final Placeholder placeholder in placeholders.values) {
-      if (placeholder.isPlural && placeholder.isSelect) {
-        throw L10nException('Placeholder is used as both a plural and select in certain languages.');
-      } else if (placeholder.isPlural) {
-        if (placeholder.type == null) {
-          placeholder.type = 'num';
-        }
-        else if (!<String>['num', 'int'].contains(placeholder.type)) {
-          throw L10nException("Placeholders used in plurals must be of type 'num' or 'int'");
-        }
-      } else if (placeholder.isSelect) {
-        if (placeholder.type == null) {
-          placeholder.type = 'String';
-        } else if (placeholder.type != 'String') {
-          throw L10nException("Placeholders used in selects must be of type 'String'");
-        }
-      }
-      placeholder.type ??= 'Object';
+      parsedMessages[bundle.locale] = translation == null ? null : Parser(
+        resourceId,
+        bundle.file.basename,
+        translation,
+        useEscaping: useEscaping,
+        logger: logger
+      ).parse();
     }
+    // Infer the placeholders
+    _inferPlaceholders(filenames);
   }
 
   final String resourceId;
@@ -402,6 +372,7 @@ class Message {
   final Map<LocaleInfo, Node?> parsedMessages;
   final Map<String, Placeholder> placeholders;
   final bool useEscaping;
+  final Logger? logger;
 
   bool get placeholdersRequireFormatting => placeholders.values.any((Placeholder p) => p.requiresFormatting);
 
@@ -496,6 +467,63 @@ class Message {
       }),
     );
   }
+
+  // Using parsed translations, attempt to infer types of placeholders used by plurals and selects.
+  // For undeclared placeholders, create a new placeholder.
+  void _inferPlaceholders(Map<LocaleInfo, String> filenames) {
+    // We keep the undeclared placeholders separate so that we can sort them alphabetically afterwards.
+    final Map<String, Placeholder> undeclaredPlaceholders = <String, Placeholder>{};
+    // Helper for getting placeholder by name.
+    Placeholder? getPlaceholder(String name) => placeholders[name] ?? undeclaredPlaceholders[name];
+    for (final LocaleInfo locale in parsedMessages.keys) {
+      if (parsedMessages[locale] == null) {
+        continue;
+      }
+      final List<Node> traversalStack = <Node>[parsedMessages[locale]!];
+      while (traversalStack.isNotEmpty) {
+        final Node node = traversalStack.removeLast();
+        if (<ST>[ST.placeholderExpr, ST.pluralExpr, ST.selectExpr].contains(node.type)) {
+          final String identifier = node.children[1].value!;
+          Placeholder? placeholder = getPlaceholder(identifier);
+          if (placeholder == null) {
+            placeholder = Placeholder(resourceId, identifier, <String, Object?>{});
+            undeclaredPlaceholders[identifier] = placeholder;
+          }
+          if (node.type == ST.pluralExpr) {
+            placeholder.isPlural = true;
+          } else if (node.type == ST.selectExpr) {
+            placeholder.isSelect = true;
+          }
+        }
+        traversalStack.addAll(node.children);
+      }
+    }
+    placeholders.addEntries(
+      undeclaredPlaceholders.entries
+        .toList()
+        ..sort((MapEntry<String, Placeholder> p1, MapEntry<String, Placeholder> p2) => p1.key.compareTo(p2.key))
+    );
+
+    for (final Placeholder placeholder in placeholders.values) {
+      if (placeholder.isPlural && placeholder.isSelect) {
+        throw L10nException('Placeholder is used as both a plural and select in certain languages.');
+      } else if (placeholder.isPlural) {
+        if (placeholder.type == null) {
+          placeholder.type = 'num';
+        }
+        else if (!<String>['num', 'int'].contains(placeholder.type)) {
+          throw L10nException("Placeholders used in plurals must be of type 'num' or 'int'");
+        }
+      } else if (placeholder.isSelect) {
+        if (placeholder.type == null) {
+          placeholder.type = 'String';
+        } else if (placeholder.type != 'String') {
+          throw L10nException("Placeholders used in selects must be of type 'String'");
+        }
+      }
+      placeholder.type ??= 'Object';
+    }
+  }
 }
 
 // Represents the contents of one ARB file.
@@ -834,50 +862,3 @@ final Set<String> _iso639Languages = <String>{
   'zh',
   'zu',
 };
-
-// Used in LocalizationsGenerator._generateMethod.generateHelperMethod.
-class HelperMethod {
-  HelperMethod(this.dependentPlaceholders, {this.helper, this.placeholder, this.string }):
-    assert((() {
-      // At least one of helper, placeholder, string must be nonnull.
-      final bool a = helper == null;
-      final bool b = placeholder == null;
-      final bool c = string == null;
-      return (!a && b && c) || (a && !b && c) || (a && b && !c);
-    })());
-
-  Set<Placeholder> dependentPlaceholders;
-  String? helper;
-  Placeholder? placeholder;
-  String? string;
-
-  String get helperOrPlaceholder {
-    if (helper != null) {
-      return '$helper($methodArguments)';
-    } else if (string != null) {
-      return '$string';
-    } else {
-      if (placeholder!.requiresFormatting) {
-        return '${placeholder!.name}String';
-      } else {
-        return placeholder!.name;
-      }
-    }
-  }
-
-  String get methodParameters {
-    assert(helper != null);
-    return dependentPlaceholders.map((Placeholder placeholder) =>
-      (placeholder.requiresFormatting)
-        ? 'String ${placeholder.name}String'
-        : '${placeholder.type} ${placeholder.name}').join(', ');
-  }
-
-  String get methodArguments {
-    assert(helper != null);
-    return dependentPlaceholders.map((Placeholder placeholder) =>
-      (placeholder.requiresFormatting)
-        ? '${placeholder.name}String'
-        : placeholder.name).join(', ');
-  }
-}

packages/flutter_tools/lib/src/localizations/localizations_utils.dart

@@ -297,25 +297,23 @@ String generateString(String value) {
 
 /// Given a list of strings, placeholders, or helper function calls, concatenate
 /// them into one expression to be returned.
-String generateReturnExpr(List<HelperMethod> helpers) {
-  if (helpers.isEmpty) {
+/// If isSingleStringVar is passed, then we want to convert "'$expr'" to simply "expr".
+String generateReturnExpr(List<String> expressions, { bool isSingleStringVar = false }) {
+  if (expressions.isEmpty) {
     return "''";
-  } else if (
-    helpers.length == 1
-      && helpers[0].string == null
-      && (helpers[0].placeholder?.type == 'String' || helpers[0].helper != null)
-  ) {
-    return helpers[0].helperOrPlaceholder;
+  } else if (isSingleStringVar) {
+    // If our expression is "$varName" where varName is a String, this is equivalent to just varName.
+    return expressions[0].substring(1);
   } else {
-    final String string = helpers.reversed.fold<String>('', (String string, HelperMethod helper) {
-      if (helper.string != null) {
-        return generateString(helper.string!) + string;
+    final String string = expressions.reversed.fold<String>('', (String string, String expression) {
+      if (expression[0] != r'$') {
+        return generateString(expression) + string;
       }
       final RegExp alphanumeric = RegExp(r'^([0-9a-zA-Z]|_)+$');
-      if (alphanumeric.hasMatch(helper.helperOrPlaceholder) && !(string.isNotEmpty && alphanumeric.hasMatch(string[0]))) {
-        return '\$${helper.helperOrPlaceholder}$string';
+      if (alphanumeric.hasMatch(expression.substring(1)) && !(string.isNotEmpty && alphanumeric.hasMatch(string[0]))) {
+        return '$expression$string';
       } else {
-        return '\${${helper.helperOrPlaceholder}}$string';
+        return '\${${expression.substring(1)}}$string';
       }
     });
     return "'$string'";
@@ -340,6 +338,7 @@ class LocalizationOptions {
     this.usesNullableGetter = true,
     this.format = false,
     this.useEscaping = false,
+    this.suppressWarnings = false,
   }) : assert(useSyntheticPackage != null);
 
   /// The `--arb-dir` argument.
@@ -416,6 +415,11 @@ class LocalizationOptions {
   ///
   /// Whether or not the ICU escaping syntax is used.
   final bool useEscaping;
+
+  /// The `suppress-warnings` argument.
+  ///
+  /// Whether or not to suppress warnings.
+  final bool suppressWarnings;
 }
 
 /// Parse the localizations configuration options from [file].
@@ -450,8 +454,9 @@ LocalizationOptions parseLocalizationsOptions({
     useSyntheticPackage: _tryReadBool(yamlNode, 'synthetic-package', logger) ?? true,
     areResourceAttributesRequired: _tryReadBool(yamlNode, 'required-resource-attributes', logger) ?? false,
     usesNullableGetter: _tryReadBool(yamlNode, 'nullable-getter', logger) ?? true,
-    format: _tryReadBool(yamlNode, 'format', logger) ?? true,
+    format: _tryReadBool(yamlNode, 'format', logger) ?? false,
     useEscaping: _tryReadBool(yamlNode, 'use-escaping', logger) ?? false,
+    suppressWarnings: _tryReadBool(yamlNode, 'suppress-warnings', logger) ?? false,
   );
 }
 

packages/flutter_tools/lib/src/localizations/message_parser.dart

@@ -6,6 +6,7 @@
 // See https://flutter.dev/go/icu-message-parser.
 
 // Symbol Types
+import '../base/logger.dart';
 import 'gen_l10n_types.dart';
 
 enum ST {
@@ -181,13 +182,17 @@ class Parser {
     this.messageId,
     this.filename,
     this.messageString,
-    { this.useEscaping = false }
+    {
+      this.useEscaping = false,
+      this.logger
+    }
   );
 
   final String messageId;
   final String messageString;
   final String filename;
   final bool useEscaping;
+  final Logger? logger;
 
   static String indentForError(int position) {
     return '${List<String>.filled(position, ' ').join()}^';
@@ -297,6 +302,11 @@ class Parser {
           // Do not add whitespace as a token.
           startIndex = match.end;
           continue;
+        } else if (<ST>[ST.plural, ST.select].contains(matchedType) && tokens.last.type == ST.openBrace) {
+          // Treat "plural" or "select" as identifier if it comes right after an open brace.
+          tokens.add(Node(ST.identifier, startIndex, value: match.group(0)));
+          startIndex = match.end;
+          continue;
         } else {
           tokens.add(Node(matchedType!, startIndex, value: match.group(0)));
           startIndex = match.end;
@@ -566,8 +576,13 @@ class Parser {
   }
 
   Node parse() {
-    final Node syntaxTree = compress(parseIntoTree());
-    checkExtraRules(syntaxTree);
-    return syntaxTree;
+    try {
+      final Node syntaxTree = compress(parseIntoTree());
+      checkExtraRules(syntaxTree);
+      return syntaxTree;
+    } on L10nParserException catch (error) {
+      logger?.printError(error.toString());
+      return Node(ST.empty, 0, value: '');
+    }
   }
 }

packages/flutter_tools/test/general.shard/message_parser_test.dart

@@ -218,6 +218,14 @@ void main() {
     ]));
   });
 
+  testWithoutContext('lexer identifier names can be "select" or "plural"', () {
+    final List<Node> tokens = Parser('keywords', 'app_en.arb', '{ select } { plural, select, singular{test} other{hmm} }').lexIntoTokens();
+    expect(tokens[1].value, equals('select'));
+    expect(tokens[1].type, equals(ST.identifier));
+    expect(tokens[5].value, equals('plural'));
+    expect(tokens[5].type, equals(ST.identifier));
+  });
+
   testWithoutContext('lexer: lexically correct but syntactically incorrect', () {
     final List<Node> tokens = Parser(
       'syntax',
@@ -242,9 +250,9 @@ void main() {
   testWithoutContext('lexer unmatched single quote', () {
     const String message = "here''s an unmatched single quote: '";
     const String expectedError = '''
-ICU Lexing Error: Unmatched single quotes.
-[app_en.arb:escaping] here''s an unmatched single quote: '
-                                                         ^''';
+[app_en.arb:escaping] ICU Lexing Error: Unmatched single quotes.
+    here''s an unmatched single quote: '
+                                       ^''';
     expect(
       () => Parser('escaping', 'app_en.arb', message, useEscaping: true).lexIntoTokens(),
       throwsA(isA<L10nException>().having(
@@ -257,9 +265,9 @@ ICU Lexing Error: Unmatched single quotes.
   testWithoutContext('lexer unexpected character', () {
     const String message = '{ * }';
     const String expectedError = '''
-ICU Lexing Error: Unexpected character.
-[app_en.arb:lex] { * }
-                   ^''';
+[app_en.arb:lex] ICU Lexing Error: Unexpected character.
+    { * }
+      ^''';
     expect(
       () => Parser('lex', 'app_en.arb', message).lexIntoTokens(),
       throwsA(isA<L10nException>().having(
@@ -460,11 +468,11 @@ ICU Lexing Error: Unexpected character.
   testWithoutContext('parser unexpected token', () {
     // unexpected token
     const String expectedError1 = '''
-ICU Syntax Error: Expected "}" but found "=".
-[app_en.arb:unexpectedToken] { placeholder =
-                                           ^''';
+[app_en.arb:unexpectedToken] ICU Syntax Error: Expected "}" but found "=".
+    { placeholder =
+                  ^''';
     expect(
-      () => Parser('unexpectedToken', 'app_en.arb', '{ placeholder =').parse(),
+      () => Parser('unexpectedToken', 'app_en.arb', '{ placeholder =').parseIntoTree(),
       throwsA(isA<L10nException>().having(
         (L10nException e) => e.message,
         'message',
@@ -472,11 +480,11 @@ ICU Syntax Error: Expected "}" but found "=".
     )));
 
     const String expectedError2 = '''
-ICU Syntax Error: Expected "number" but found "}".
-[app_en.arb:unexpectedToken] { count, plural, = }
-                                                ^''';
+[app_en.arb:unexpectedToken] ICU Syntax Error: Expected "number" but found "}".
+    { count, plural, = }
+                       ^''';
     expect(
-      () => Parser('unexpectedToken', 'app_en.arb', '{ count, plural, = }').parse(),
+      () => Parser('unexpectedToken', 'app_en.arb', '{ count, plural, = }').parseIntoTree(),
       throwsA(isA<L10nException>().having(
         (L10nException e) => e.message,
         'message',
@@ -484,11 +492,11 @@ ICU Syntax Error: Expected "number" but found "}".
     )));
 
     const String expectedError3 = '''
-ICU Syntax Error: Expected "identifier" but found ",".
-[app_en.arb:unexpectedToken] { , plural , = }
-                               ^''';
+[app_en.arb:unexpectedToken] ICU Syntax Error: Expected "identifier" but found ",".
+    { , plural , = }
+      ^''';
     expect(
-      () => Parser('unexpectedToken', 'app_en.arb', '{ , plural , = }').parse(),
+      () => Parser('unexpectedToken', 'app_en.arb', '{ , plural , = }').parseIntoTree(),
       throwsA(isA<L10nException>().having(
         (L10nException e) => e.message,
         'message',