fixes bug 1451450, parse VariantExpressions with a MessageReference inside

[Pike]

This switches the props of VariantExpression from having a
string id to a MessageReference of

https://bugzilla.mozilla.org/show_bug.cgi?id=1451450

[stasm]

Can you change this to serialize_expression(expr.of)?

[Pike]

Follow-up for this landed.

[stasm]

@Pike, @zbraniecki - do you have opinions on what we should call this? It should fit both MessageReference (now) as well as as ExternalArgument (later).

I looked up a few examples: a.b parses as Attribute(value=Name(id="a"), attr="b") in Python and as MemberExpression(object=Identifier(name="a"), property=Identifier(name="b")) in JavaScript.

[zbraniecki]

no strong opinion, I prefer both Python and JS naming scheme over of.

[stasm]

I'd suggest object as it's generic enough to fit a wide range of expressions. Another idea: how about reference, or ref?

• -term[varname] would parse as VariantExpression(ref=MessageReference(id=Identifier(name="-term")), key=VariantName(name="varname")).

• $arg[varname] would parse as VariantExpression(ref=ExternalArgument(id=Identifier(name="-term")), key=VariantName(name="varname")).

[Pike]

ref works for me.

[stasm]

@Pike - I think we should also do this for AttributeExpressions, for the same reasons. It also matters for the compare-locales check.

[Pike]

@Pike - I think we should also do this for AttributeExpressions, for the same reasons. It also matters for the compare-locales check.

Actually, I think that'd make it harder to do attribute checks. There's a profound semantic difference between {close-me}, {close-me.label} and {close-me.accesskey}, so I don't think we should take one for the other. If they'd be MessageReferences, the check would need to find MessageReference, but not if it's in an AttributeExpression

[stasm]

Sorry, I didn't make it clear. By "the same reasons" I meant the fact that we'll need to support AttributeExpressions on both MessageReferences and ExternalArguments. We'll make that change in the implementation of Dynamic References. This PR is making ground for it and I'd like it to do it symmetrically for both VariantExpressions and AttributeExpressions, please.

For compare-locales it sounds like we should have a separate check for symmetrical AttributeExpressions of Messages (not Terms).

In fact, perhaps we should consider introducing TermReferences in a follow-up to make this easier?

[Pike]

I took a look at AttributeExpression, and the impact of adding a MessageReference in there isn't clear to me at all in the light of compare-locales.

I'd rather not block fixing the term variant fix we have in Firefox today by having to do full attribute support.

[stasm]

OK, I see. I'm a bit worried about this fix then. I think it's a sound design for VariantExpression and it should be extended to AttributeExpression but I see how the latter would make some static analysis harder. Perhaps we need a sister method to BaseNode.traverse then, with a slightly different mechanics, e.g. return True if the traversal should continue deeper.

[Pike]

I really liked https://github.com/estools/esquery when doing my js ast jazz for the react editors, I've contemplated doing something similar. I also found traverse to be pretty excessive, i.e., it goes to each number in spans, which I don't need in compare-locales checks at all ;-)

[stasm]

Let's decide what the name of the of field should be and let's land this to unblock Firefox.

[stasm]

r=me with the s/of/ref change.