ICU Message Syntax Parser #112390

thkim1011 · 2022-09-26T15:07:35Z

Description

This PR implements the new parser for ICU message format. The current implementation involves doing a lot of hacky RegEx string replacements (i.e. replace placeholders to use a different syntax, RegEx match plurals and selects) and making assumptions about the messages (e.g. the original implementation assumes that messages may have at most one plural). This has a lot of limitations which include

Not able to recursively use plural and select syntax.
Incorrect behavior when placeholders, plurals, selects are mixed.
Cannot return a syntax error if the RegEx is not matched.
Hard to verify correctness of code.

We provide a new approach. The high level idea is to divide up the whole process into three steps:

Lexing the message into tokens. This is done via using RegEx to determine what the next token is. See message_parser.dart:Parser.lexIntoTokens.
Parsing the tokens into a syntax tree according to a grammar. This is done using the LL parsing algorithm. See message_parser.dart:Parser.parseIntoTree.
Code generation via tree traversal through the syntax tree. This is done using code templates in gen_l10n_templates.dart and implemented in gen_l10n.dart:LocalizationsGenerator._generateMethod.

Another feature of separating the parsing and code generation process is better error messages. For example, if there is an unexpected token, then we should see the error message

ICU Syntax Error: Expected "identifier" but found ",".
{ , plural , = }
  ^

Design Doc

Link to design doc.

Testing

Created a new message_parser_test.dart file which includes tests for up to parsing.
Ensured that all existing code generation tests in generate_localizations_test.dart succeed.
Added a nested plural/select test case to the E2E test in gen_l10n_test.dart.

Follow up work

There's a little more work left, which is a bit outside the scope of this PR. Essentially, the placeholder type inference for placeholders for plurals and selects were done by assuming that there is a unique count placeholder per message. Since we no longer make this assumption, we need to refactor the Message class to include all messages, parse all of these messages at the same time, then attempt to infer the correct type for each placeholder. This PR doesn't modify the old placeholder type inference logic, so it may not work properly if there are multiple plurals.

We're also setting aside escaping for a separate PR but that's lower priority than above.

Related Issue

Fixes #86906. Fixes #113495.

Future Work

Message Refactor ICU Message Refactoring #112709.
Add escaping syntax ICU Escaping Syntax #113455.

auto-submit · 2022-11-01T21:53:45Z

auto label is removed for flutter/flutter, pr: 112390, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2022-11-02T22:15:05Z

auto label is removed for flutter/flutter, pr: 112390, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

* cef4c2a ICU Message Syntax Parser (flutter/flutter#112390) * 07d3a64 0886c7d0b Narrow the scope of the Windows Android arm hack (flutter/engine#37125) (flutter/flutter#114751) * cf0cd06 Mark new_gallery__transition_perf as non-flaky (flutter/flutter#114694) * 5bc50c1 Fix macOS migration nothing-to-upgrade test (flutter/flutter#114703) * b1509ed 36cfa9e68 Apply the Windows friendly path solution in remaining spots (flutter/engine#37344) (flutter/flutter#114759) * b45e628 b8218eacc Roll Fuchsia Mac SDK from Ua8Jtf8Zka9uxIVdl... to sNXsQVxntMX8f42LE... (flutter/engine#37352) (flutter/flutter#114760) * 5ea5a53 6f3018a72 Roll Skia from a34882309d04 to c3c31be8729b (1 revision) (flutter/engine#37358) (flutter/flutter#114765)

florian-obernberger · 2022-11-10T00:41:49Z

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

+              dependentPlaceholders.addAll(pluralPartHelper.dependentPlaceholders);
+            } else {
+              logger.printWarning('''
+The plural part specified below is overrided by a later plural part.


I think "overrided" is a typo? Shouldn't it be "overridden"?

* cef4c2a ICU Message Syntax Parser (flutter/flutter#112390) * 07d3a64 0886c7d0b Narrow the scope of the Windows Android arm hack (flutter/engine#37125) (flutter/flutter#114751) * cf0cd06 Mark new_gallery__transition_perf as non-flaky (flutter/flutter#114694) * 5bc50c1 Fix macOS migration nothing-to-upgrade test (flutter/flutter#114703) * b1509ed 36cfa9e68 Apply the Windows friendly path solution in remaining spots (flutter/engine#37344) (flutter/flutter#114759) * b45e628 b8218eacc Roll Fuchsia Mac SDK from Ua8Jtf8Zka9uxIVdl... to sNXsQVxntMX8f42LE... (flutter/engine#37352) (flutter/flutter#114760) * 5ea5a53 6f3018a72 Roll Skia from a34882309d04 to c3c31be8729b (1 revision) (flutter/engine#37358) (flutter/flutter#114765)

edeuss · 2022-11-21T05:25:02Z

Any idea when this will be brought to stable or beta? We are currently running into some issues that this pull fixes for us.

christopherfujino · 2022-11-21T18:26:52Z

Any idea when this will be brought to stable or beta? We are currently running into some issues that this pull fixes for us.

I believe it's already in beta: https://github.com/flutter/flutter/tree/beta/packages/flutter_tools/lib/src/localizations

This reverts commit cef4c2a.

* init * code generation * improve syntax error, add tests * add tests and fix bugs * code generation fix * fix all tests :) * fix bug * init * fix all code gen issues * FIXED ALL TESTS :D * add license * remove trailing spaces * remove print * tests fix * specify type annotation * fix test * lint * fix todos * fix subclass issues * final fix; flutter gallery runs * escaping for later pr * fix comment * address PR comments * more * more descriptive errors * last fixes

* cef4c2a ICU Message Syntax Parser (flutter/flutter#112390) * 07d3a64 0886c7d0b Narrow the scope of the Windows Android arm hack (flutter/engine#37125) (flutter/flutter#114751) * cf0cd06 Mark new_gallery__transition_perf as non-flaky (flutter/flutter#114694) * 5bc50c1 Fix macOS migration nothing-to-upgrade test (flutter/flutter#114703) * b1509ed 36cfa9e68 Apply the Windows friendly path solution in remaining spots (flutter/engine#37344) (flutter/flutter#114759) * b45e628 b8218eacc Roll Fuchsia Mac SDK from Ua8Jtf8Zka9uxIVdl... to sNXsQVxntMX8f42LE... (flutter/engine#37352) (flutter/flutter#114760) * 5ea5a53 6f3018a72 Roll Skia from a34882309d04 to c3c31be8729b (1 revision) (flutter/engine#37358) (flutter/flutter#114765)

thkim1011 added 4 commits September 13, 2022 13:36

init

223454d

code generation

2f0aef5

improve syntax error, add tests

2a57f95

add tests and fix bugs

d42dd5e

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 26, 2022

thkim1011 added 6 commits September 28, 2022 11:18

code generation fix

5450ee3

fix all tests :)

7bde38d

fix bug

52838aa

init

cbeaf2a

fix all code gen issues

4644992

FIXED ALL TESTS :D

3972495

thkim1011 changed the title ~~[WIP] ICU Message Syntax Parser~~ ICU Message Syntax Parser Sep 29, 2022

thkim1011 marked this pull request as ready for review September 29, 2022 19:14

thkim1011 requested a review from HansMuller September 29, 2022 19:14

thkim1011 added 7 commits September 29, 2022 15:17

add license

386912e

remove trailing spaces

9cc6611

remove print

b850e58

Merge remote-tracking branch 'upstream/master' into tae/parser

91321f8

tests fix

dabc153

specify type annotation

a005930

fix test

b559202

thkim1011 mentioned this pull request Sep 30, 2022

ICU Message Refactoring #112709

Closed

thkim1011 added 4 commits September 30, 2022 10:30

lint

f4f4f2c

fix todos

6e96f69

Merge remote-tracking branch 'upstream/master' into tae/parser

4122736

Merge remote-tracking branch 'upstream/master' into tae/parser

5d81014

thkim1011 mentioned this pull request Oct 5, 2022

Fix new syntax for escaping with single quotes flutter/gallery#797

Merged

fix subclass issues

8311176

thkim1011 mentioned this pull request Oct 14, 2022

ICU Escaping Syntax #113455

Closed

final fix; flutter gallery runs

2f76e7f

Merge remote-tracking branch 'upstream/master' into tae/parser

79ed635

thkim1011 added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2022

Merge remote-tracking branch 'upstream/master' into tae/parser

781973a

thkim1011 added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

Merge remote-tracking branch 'upstream/master' into tae/parser

d4af6c0

thkim1011 added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2022

thkim1011 merged commit cef4c2a into flutter:master Nov 5, 2022

This was referenced Nov 6, 2022

Roll Flutter from e0e7027b6263 to 5ea5a5330169 (7 revisions) flutter/packages#2783

Merged

Roll Flutter from e0e7027b6263 to 5ea5a5330169 (7 revisions) flutter/plugins#6675

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2022

cef4c2a ICU Message Syntax Parser (flutter/flutter#112390)

0613644

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 6, 2022

cef4c2a ICU Message Syntax Parser (flutter/flutter#112390)

8dfb735

florian-obernberger reviewed Nov 10, 2022

View reviewed changes

zanderso mentioned this pull request Nov 16, 2022

i10n_gen errors/warnings improvement suggestions + need testing #115501

Closed

asashour mentioned this pull request Nov 21, 2022

[flutter_localizations] doesn't understand plural and select in arb file #115731

Closed

jonahwilliams pushed a commit to jonahwilliams/flutter that referenced this pull request Nov 28, 2022

Revert "ICU Message Syntax Parser (flutter#112390)"

d9a4425

This reverts commit cef4c2a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ICU Message Syntax Parser #112390

ICU Message Syntax Parser #112390

Uh oh!

thkim1011 commented Sep 26, 2022 •

edited

Loading

Uh oh!

auto-submit bot commented Nov 1, 2022

Uh oh!

auto-submit bot commented Nov 2, 2022

Uh oh!

florian-obernberger Nov 10, 2022 •

edited

Loading

Uh oh!

edeuss commented Nov 21, 2022

Uh oh!

christopherfujino commented Nov 21, 2022

Uh oh!

Uh oh!

ICU Message Syntax Parser #112390

ICU Message Syntax Parser #112390

Uh oh!

Conversation

thkim1011 commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Design Doc

Testing

Follow up work

Related Issue

Future Work

Uh oh!

auto-submit bot commented Nov 1, 2022

Uh oh!

auto-submit bot commented Nov 2, 2022

Uh oh!

florian-obernberger Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edeuss commented Nov 21, 2022

Uh oh!

christopherfujino commented Nov 21, 2022

Uh oh!

Uh oh!

thkim1011 commented Sep 26, 2022 •

edited

Loading

florian-obernberger Nov 10, 2022 •

edited

Loading