Fix message type inconsistency between locales (#120129)

* init

* fix error handling

* fix issue

* lint?

* error handling tests

* lint

Author: thkim1011

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

@@ -877,6 +877,7 @@ class LocalizationsGenerator {
     _allMessages = _templateBundle.resourceIds.map((String id) => Message(
        _templateBundle, _allBundles, id, areResourceAttributesRequired, useEscaping: useEscaping, logger: logger,
     )).toList();
+    hadErrors = _allMessages.any((Message message) => message.hadErrors);
     if (inputsAndOutputsListFile != null) {
       _inputFileList.addAll(_allBundles.bundles.map((AppResourceBundle bundle) {
         return bundle.file.absolute.path;
@@ -915,16 +916,19 @@ class LocalizationsGenerator {
     final LocaleInfo locale,
   ) {
     final Iterable<String> methods = _allMessages.map((Message message) {
+      LocaleInfo localeWithFallback = locale;
       if (message.messages[locale] == null) {
         _addUnimplementedMessage(locale, message.resourceId);
-        return _generateMethod(
-          message,
-          _templateArbLocale,
-        );
+        localeWithFallback = _templateArbLocale;
+      }
+      if (message.parsedMessages[localeWithFallback] == null) {
+        // The message exists, but parsedMessages[locale] is null due to a syntax error.
+        // This means that we have already set hadErrors = true while constructing the Message.
+        return '';
       }
       return _generateMethod(
         message,
-        locale,
+        localeWithFallback,
       );
     });
 
@@ -953,7 +957,7 @@ class LocalizationsGenerator {
       });
 
     final Iterable<String> methods = _allMessages
-      .where((Message message) => message.messages[locale] != null)
+      .where((Message message) => message.parsedMessages[locale] != null)
       .map((Message message) => _generateMethod(message, locale));
 
     return subclassTemplate
@@ -1103,8 +1107,8 @@ class LocalizationsGenerator {
 
       final String translationForMessage = message.messages[locale]!;
       final Node node = message.parsedMessages[locale]!;
-      // If parse tree is only a string, then return a getter method.
-      if (node.children.every((Node child) => child.type == ST.string)) {
+      // If the placeholders list is empty, then return a getter method.
+      if (message.placeholders.isEmpty) {
         // Use the parsed translation to handle escaping with the same behavior.
         return getterTemplate
           .replaceAll('@(name)', message.resourceId)

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

@@ -349,13 +349,20 @@ 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,
-        logger: logger
-      ).parse();
+      try {
+        parsedMessages[bundle.locale] = translation == null ? null : Parser(
+          resourceId,
+          bundle.file.basename,
+          translation,
+          useEscaping: useEscaping,
+          logger: logger
+        ).parse();
+      } on L10nParserException catch (error) {
+        logger?.printError(error.toString());
+        // Treat it as an untranslated message in case we can't parse.
+        parsedMessages[bundle.locale] = null;
+        hadErrors = true;
+      }
     }
     // Infer the placeholders
     _inferPlaceholders(filenames);
@@ -369,6 +376,7 @@ class Message {
   final Map<String, Placeholder> placeholders;
   final bool useEscaping;
   final Logger? logger;
+  bool hadErrors = false;
 
   bool get placeholdersRequireFormatting => placeholders.values.any((Placeholder p) => p.requiresFormatting);
 

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

@@ -587,17 +587,8 @@ class Parser {
   }
 
   Node parse() {
-    try {
-      final Node syntaxTree = compress(parseIntoTree());
-      checkExtraRules(syntaxTree);
-      return syntaxTree;
-    } on L10nParserException catch (error) {
-      // For debugging purposes.
-      if (logger == null) {
-        rethrow;
-      }
-      logger?.printError(error.toString());
-      return Node(ST.empty, 0, value: '');
-    }
+    final Node syntaxTree = compress(parseIntoTree());
+    checkExtraRules(syntaxTree);
+    return syntaxTree;
   }
 }

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

@@ -1733,6 +1733,47 @@ import 'output-localization-file_en.dart' deferred as output-localization-file_e
         ).readAsStringSync();
         expect(localizationsFile, contains('String helloWorld(Object name) {'));
       });
+
+      testWithoutContext('placeholder parameter list should be consistent between languages', () {
+        const String messageEn = '''
+{
+  "helloWorld": "Hello {name}",
+  "@helloWorld": {
+    "placeholders": {
+      "name": {}
+    }
+  }
+}''';
+        const String messageEs = '''
+{
+  "helloWorld": "Hola"
+}
+''';
+        final Directory l10nDirectory = fs.currentDirectory.childDirectory('lib').childDirectory('l10n')
+          ..createSync(recursive: true);
+        l10nDirectory.childFile(defaultTemplateArbFileName)
+          .writeAsStringSync(messageEn);
+        l10nDirectory.childFile('app_es.arb')
+          .writeAsStringSync(messageEs);
+        LocalizationsGenerator(
+          fileSystem: fs,
+          inputPathString: defaultL10nPathString,
+          templateArbFileName: defaultTemplateArbFileName,
+          outputFileString: defaultOutputFileString,
+          classNameString: defaultClassNameString,
+          logger: logger,
+        )
+          ..loadResources()
+          ..writeOutputFiles();
+        final String localizationsFileEn = fs.file(
+          fs.path.join(syntheticL10nPackagePath, 'output-localization-file_en.dart'),
+        ).readAsStringSync();
+        final String localizationsFileEs = fs.file(
+          fs.path.join(syntheticL10nPackagePath, 'output-localization-file_es.dart'),
+        ).readAsStringSync();
+        expect(localizationsFileEn, contains('String helloWorld(Object name) {'));
+        expect(localizationsFileEs, contains('String helloWorld(Object name) {'));
+      });
     });
 
     group('DateTime tests', () {
@@ -2258,6 +2299,93 @@ import 'output-localization-file_en.dart' deferred as output-localization-file_e
       });
     });
 
+    // All error handling for messages should collect errors on a per-error
+    // basis and log them out individually. Then, it will throw an L10nException.
+    group('error handling tests', () {
+      testWithoutContext('syntax/code-gen errors properly logs errors per message', () {
+        // TODO(thkim1011): Fix error handling so that long indents don't get truncated.
+        // See https://github.com/flutter/flutter/issues/120490.
+        const String messagesWithSyntaxErrors = '''
+{
+  "hello": "Hello { name",
+  "plural": "This is an incorrectly formatted plural: { count, plural, zero{No frog} one{One frog} other{{count} frogs}",
+  "explanationWithLexingError": "The 'string above is incorrect as it forgets to close the brace",
+  "pluralWithInvalidCase": "{ count, plural, woohoo{huh?} other{lol} }"
+}''';
+        try {
+          final Directory l10nDirectory = fs.currentDirectory.childDirectory('lib').childDirectory('l10n')
+            ..createSync(recursive: true);
+          l10nDirectory.childFile(defaultTemplateArbFileName)
+            .writeAsStringSync(messagesWithSyntaxErrors);
+          LocalizationsGenerator(
+            fileSystem: fs,
+            inputPathString: defaultL10nPathString,
+            outputPathString: defaultL10nPathString,
+            templateArbFileName: defaultTemplateArbFileName,
+            outputFileString: defaultOutputFileString,
+            classNameString: defaultClassNameString,
+            useEscaping: true,
+            logger: logger,
+          )
+            ..loadResources()
+            ..writeOutputFiles();
+        } on L10nException {
+          expect(logger.errorText, contains('''
+[app_en.arb:hello] ICU Syntax Error: Expected "}" but found no tokens.
+    Hello { name
+                 ^
+[app_en.arb:plural] ICU Syntax Error: Expected "}" but found no tokens.
+    This is an incorrectly formatted plural: { count, plural, zero{No frog} one{One frog} other{{count} frogs}
+                                                                                          ^
+[app_en.arb:explanationWithLexingError] ICU Lexing Error: Unmatched single quotes.
+    The 'string above is incorrect as it forgets to close the brace
+        ^
+[app_en.arb:pluralWithInvalidCase] ICU Syntax Error: Plural expressions case must be one of "zero", "one", "two", "few", "many", or "other".
+    { count, plural, woohoo{huh?} other{lol} }
+                     ^'''));
+        }
+      });
+
+      testWithoutContext('errors thrown in multiple languages are all shown', () {
+        const String messageEn = '''
+{
+  "hello": "Hello { name"
+}''';
+        const String messageEs = '''
+{
+  "hello": "Hola { name"
+}''';
+        try {
+          final Directory l10nDirectory = fs.currentDirectory.childDirectory('lib').childDirectory('l10n')
+            ..createSync(recursive: true);
+          l10nDirectory.childFile(defaultTemplateArbFileName)
+            .writeAsStringSync(messageEn);
+          l10nDirectory.childFile('app_es.arb')
+            .writeAsStringSync(messageEs);
+          LocalizationsGenerator(
+            fileSystem: fs,
+            inputPathString: defaultL10nPathString,
+            outputPathString: defaultL10nPathString,
+            templateArbFileName: defaultTemplateArbFileName,
+            outputFileString: defaultOutputFileString,
+            classNameString: defaultClassNameString,
+            useEscaping: true,
+            logger: logger,
+          )
+            ..loadResources()
+            ..writeOutputFiles();
+        } on L10nException {
+          expect(logger.errorText, contains('''
+[app_en.arb:hello] ICU Syntax Error: Expected "}" but found no tokens.
+    Hello { name
+                 ^
+[app_es.arb:hello] ICU Syntax Error: Expected "}" but found no tokens.
+    Hola { name
+                ^'''));
+        }
+      });
+    });
+
     testWithoutContext('intl package import should be omitted in subclass files when no plurals are included', () {
       fs.currentDirectory.childDirectory('lib').childDirectory('l10n')..createSync(recursive: true)
         ..childFile(defaultTemplateArbFileName).writeAsStringSync(singleMessageArbFileString)