1525869 - Switch browser_misused_characters_in_strings.js to use fluent-syntax

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[Francesco Lodolo [:flod]]

Bug 1517496 added checks for FTL files, but we should rely on the fluent-syntax package as soon as it's available in tree, instead of using the run-time parser.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Giving this a priority to take it out of the triage list - adjust as necessary.

Comment 2

[Staś Małolepszy :stas]

FluentSyntax.jsm is in the tree now and it provides a Visitor class which helps run logic on the AST nodes: https://searchfox.org/mozilla-central/source/intl/l10n/FluentSyntax.jsm#1887. It's inspired by the Python ast modules NodeVisitor: https://docs.python.org/2/library/ast.html#ast.NodeVisitor. It's meant to be subclassed, and the subclass can add methods like visitTextElement(node).

Comment 3

[Francesco Lodolo [:flod]]

I think the plan is more around using compare-locales for this? We already have the l10n linter in place, so it's a matter of adding checks, and I have the feeling Axel already has a POC somewhere (given he filed bug 1588176 a while ago).

Comment 4

[Staś Małolepszy :stas]

Ah, great. The linter is a much more scalable solution to this, I agree. I wouldn't mind removing FluentSyntax.jsm, if it's not used anywhere else.

As Zibi mentioned on Slack, bug 1560038 will make FluentResource opaque with no way to inspect the runtime AST, which is why I came to this bug and commented about the Visitor. But moving the test to the linter will obviously be a solution, too.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can you confirm that you're intending to replace that with c-l? If so, is there a bug for that? I'd like to wontfix this in such case.

Comment 6

[Axel Hecht [:Pike]]

I've got a POC for c-l locally, but there's no bug, nor is there a timeline.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm making progress on remaining loose ends in bug 1560038. Axel, do you have any ETA for this?

Comment 8

[Axel Hecht [:Pike]]

I don't have an ETA, I've put that branch on the backburner after filing bug 1588176.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Since Axel doesn't have any ETA for his work, and this is going to become a blocker for fluent-rs switch, can we discuss options to move forward?

Stas landed FluentSyntax.jsm and suggested switching to it. With fluent-rs landing we also get access to fluent-syntax crate which would allow us to do this in Rust.

How important is this test? Do we need to keep this as a blocker to bug 1560038?

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #9)

How important is this test? Do we need to keep this as a blocker to bug 1560038?

The test was added for a reason (consistency), and it's important. I'd like to avoid going back to manually comment every single patch, and risk missing some in the process. It's also not great code, but it does what it's supposed to do (most of the time).

Having said that, I'm not the one who can (or should) answer about the blocking status, or how to prioritize work to fix it.

Comment 11

[:Gijs (he/him)]

We should replace the test with a linter, and we should avoid losing coverage. It seems there was a linter in bug 1588176. What's the status of that work / why hasn't that landed?

Comment 12

[Axel Hecht [:Pike]]

It's a POC for, say, half the work. I can find chars as part of a linter, and I can exclude some parts of HTML syntax that like real apostrophes. I haven't spent time on what I should do with this outside of Firefox and outside of en-US.

Comment 13

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #12)

It's a POC for, say, half the work. I can find chars as part of a linter, and I can exclude some parts of HTML syntax that like real apostrophes. I haven't spent time on what I should do with this outside of Firefox and outside of en-US.

I would like to land a polished version of this into the fluent linter on m-c, to unblock this bug and unblock bug 1560038. Is there any reason not to do so? If not, can you put up what you have so we can find someone to drive this over the line, on the assumption that you don't have time to do so yourself?

Comment 14

[Axel Hecht [:Pike]]

I've looked at my code, and I hate it. I pushed it up on https://phabricator.services.mozilla.com/D61809 and explained why. Not sure when I'd have cycles to actually do that work myself, but I'm happy to see someone else tackle it, now that I know why I'd r- it ;-) .

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Could this benefit from https://searchfox.org/mozilla-central/source/intl/locale/cldr-quotes.inc ?

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1525869 - Use FluentSyntax for misused characters in strings test.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

this bug blocks bug 1560038 and it doesn't seem like there's any better solution coming out soon, so I wrote a patch that migrates us to use FluentSyntax.jsm in a fairly plain way.

I'm basically composing every Pattern to skip any non-text-elements and combine the TextElements into a single string that is then tested just as before.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29241bff5209
Use FluentSyntax for misused characters in strings test. r=stas

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/29241bff5209