From becb6bd00aab3e1a904eeb7fe9a19b57e8cfda86 Mon Sep 17 00:00:00 2001 From: Tae Hyung Kim Date: Mon, 13 Feb 2023 15:05:57 -0800 Subject: [PATCH] Fix message type inconsistency between locales (#120129) * init * fix error handling * fix issue * lint? * error handling tests * lint --- .../lib/src/localizations/gen_l10n.dart | 20 +-- .../lib/src/localizations/gen_l10n_types.dart | 22 ++- .../lib/src/localizations/message_parser.dart | 15 +- .../generate_localizations_test.dart | 128 ++++++++++++++++++ 4 files changed, 158 insertions(+), 27 deletions(-) diff --git a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart index 820940a58b9..959ab990024 100644 --- a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart +++ b/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 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 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) 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 85c333281c0..0f2ff1b4ced 100644 --- a/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart +++ b/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 placeholders; final bool useEscaping; final Logger? logger; + bool hadErrors = false; bool get placeholdersRequireFormatting => placeholders.values.any((Placeholder p) => p.requiresFormatting); diff --git a/packages/flutter_tools/lib/src/localizations/message_parser.dart b/packages/flutter_tools/lib/src/localizations/message_parser.dart index c2bd7d982f2..a7198ef9786 100644 --- a/packages/flutter_tools/lib/src/localizations/message_parser.dart +++ b/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; } } 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 d40174abb2f..77656d34de9 100644 --- a/packages/flutter_tools/test/general.shard/generate_localizations_test.dart +++ b/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)