Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SelectExpression as a selector for another SelectExpression #416

Closed
SirNickolas opened this issue Aug 2, 2019 · 9 comments
Closed

SelectExpression as a selector for another SelectExpression #416

SirNickolas opened this issue Aug 2, 2019 · 9 comments

Comments

@SirNickolas
Copy link

test = {{0 ->
    [0] x
   *[other] y
} ->
    [x] It’s X.
   *[other] It’s not X.
}

According to the spec (abstract.js), this is well-formed yet not valid Fluent. However, this case is not enforced in getExpression, and thus the code is correctly parsed and evaluates to It’s X..

I’m currently implementing Fluent in D Programming Language and wonder whether it is a bug in the spec or the implementation (i.e. whether I need to support it or not). Could you clarify it, please?

@zbraniecki
Copy link
Collaborator

According to the spec (abstract.js), this is well-formed yet not valid Fluent

Why is it not valid?

The value of your message is a AST::Placeable, which is an AST::Expression::SelectExpression which has a selector AST::InlineExpression::Placeable which in turn is an AST::Expression::SelectExpression.

All according to the spec.

In Rust we use https://github.com/projectfluent/fluent-rs/blob/master/fluent-syntax/src/ast.rs and have the AST inspector: https://github.com/projectfluent/fluent-rs/tree/master/fluent-cli

You can launch it with cargo run --bin parser ./test.ftl and it'll show you the AST.

@SirNickolas
Copy link
Author

You’re right, it is allowed by the grammar. But it will be rejected during validation step, since selector may only be a StringLiteral, NumberLiteral, VariableReference, FunctionReference, or an attribute of a TermReference.

@zbraniecki
Copy link
Collaborator

Ah, you're right! Wondering if we should tighten the ast or relax the resolver :) I'd be down to allow for placeable as selector

@SirNickolas
Copy link
Author

Inability to put a placeable here seems like a purely synthetic limitation to me. A user can work around this by moving the inner SelectExpression to an intermediate Term. So a codegen must be able to handle such construct anyway.

What for this:

extra-braces = {{0} ->
   *[0] x
}

—it’s just syntax sugar. Pretty useless, I agree, but still cannot see a reason to forbid it. For the sake of completeness.

@SirNickolas
Copy link
Author

Oh, I’ve just found out that it will allow to bypass restriction that Messages and Terms cannot be used as selectors.

switch-my-msg = {{0 ->
   *[0] {my-msg}
} ->
    [msg_value_0] ...
    [msg_value_1] ...
   *[other] ...
}

However, it’s also possible to do right now:

-temp-term = .
    .a = {my-msg}

switch-my-msg = {-temp-term.a ->
    [msg_value_0] ...
    [msg_value_1] ...
   *[other] ...
}

By the way, could you tell me why this restriction exists? An extra measure of safety against typos (because a user will unlikely want to do this)?

@Pike
Copy link
Contributor

Pike commented Aug 3, 2019

Hi Nickolas, welcome to the world of Fluent.

Did you start publishing your D implementation yet? We're generally curious to see how other folks implement Fluent. Also, I'd be happy to link to your implementation on https://github.com/projectfluent/fluent#other-implementations.

To your question, you're encountering an unspecified specified unspecified behavior. I like to create confusing sequences of words. Anyway, the @fluent/bundle implementation is built with the Mozilla ecosystem in mind, and we have tools that check for invalid Fluent at build and even at localization time. Thus we've chosen to allow the js implementation to have undefined behavior in cases where a specification conformant parser would create parser errors. That we do so isn't written down anywhere, outside of issues and pull requests. Doing so is #301.

In your implementation, you should avoid those shortcuts, if possible.

To this concrete point, changing the specification for select expressions would be a change to the Fluent syntax specification. That's quite a hefty hammer, which I'd rather not wield.

As for the reason to reduce the options of selectors, that conversation was in projectfluent/fluent#97.

For this particular question, using messages as selectors is not allowed to protect localizers from developers. Developers would probably use that to create delicate contraptions, which we don't expect localizers to re-implement in their language. This is along the lines of WET over DRY, that we detail in https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers#prefer-wet-over-dry.

AFAICT, we also don't have a test for this in the spec repo, which means that our implementations might not catch this the way they should. At least I didn't see it in the rust impl that zibi linked to, and @fluent/syntax also doesn't, if I look at python's fluent.syntax. I filed projectfluent/fluent#279 on that.

@SirNickolas
Copy link
Author

Thank you for such a detailed reply!

To sum up, it is invalid in Fluent 1.0 and might or might not become part of Fluent 1.1. The spec is an authoritative source of information and should be preferred in the event of discrepancy.

Since you are asking, I pushed the repo to GitHub. There’s no much to look at yet, though. The parser isn’t even finished: (I hope) it correctly parses everything except Expressions. Haven’t integrated syntax tests either.

Pike added a commit to Pike/fluent.js that referenced this issue Aug 5, 2019
…uent#416

This uplifts the tests from the spec PR,
projectfluent/fluent#280
and implements the selector validation to follow what `abstract.js`
does in the spec.
Pike added a commit to Pike/fluent.js that referenced this issue Aug 5, 2019
…uent#416

This uplifts the tests from the spec PR,
projectfluent/fluent#280
and implements the selector validation to follow what `abstract.js`
does in the spec.

In @fluent/bundle, the invalid selectors are just accepted.
Might be worth revisiting.
@Pike
Copy link
Contributor

Pike commented Aug 5, 2019

Put up #417 now, and also the PR on tests on the spec. Thanks for digging in, this bug is actually pretty wide spread.

I've glanced at the repo, and I'll raise questions there.

Forgot to ask, have you come across https://discourse.mozilla.org/c/fluent ? That's where we have cross-implementation conversations.

@SirNickolas
Copy link
Author

Glad I was able to help.

Thanks for the link. No, I hadn’t discovered that.

@Pike Pike closed this as completed in 4312437 Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants