Skip to content

Add Missing Selector Annotation error #431

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

Merged
merged 5 commits into from
Jul 24, 2023

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Jul 19, 2023

See #425 for context.

Adds this requirement to the spec, and a corresponding data model error:

Each _selector_ MUST have an _annotation_,
or contain a _variable_ that directly or indirectly references a _declaration_ with an _annotation_.

Edit: Added "directly or indirectly" to the original proposal, to account for messages like:

let $one = {|The one| :func}
let $two = {$one}
match {$two}
when 1 {Value is one}
when * {Value is not one}

@eemeli eemeli requested review from aphillips, stasm and mihnita July 19, 2023 13:08
@aphillips
Copy link
Member

aphillips commented Jul 19, 2023

Meh. I don't think we should do this. I would rather do one of the following:

(a) Make :select (or :equals or whatever we call it) the default selector and normatively say that no other selectors can be inferred (other selectors must be explicitly declared). This ensures that processing is not programming language or runtime dependent, which helps translation tools do consistent stuff:

match {$two}
when 1 {two is a string with only U+0031 in it}
when * {two is anything else}

(b) Syntactically require the annotation, e.g.:

switch = matcher *variant
matcher = match 1*selector
; annotation is not optional unlike in expression
selector = "{" [s] (operand s annotation)/(annotation) [s] "}"

This makes the selector function explicit and makes it a parse error not to say which one to use. It also means that you can't use a declaration for the selector (only as the value of a selector), which I agree is kind of odd.


I'll also call out: in our discussion of mutable vs. immutable variables, @mihnita's "13/42" example comes into play here. If we say that this evaluates as "13 and 42":

let $a = {13}
let $b = {$a}
let $a = 42
{{$a} and {$b}}

... but then say that a selector is late binding:

let $a = {$count :plural}
match {$a}
when 1 {exactly 1}
when one {one plural rule}
when * {other plural rule}

... that seems like an inconsistency in favor of "42 42"

@eemeli
Copy link
Collaborator Author

eemeli commented Jul 19, 2023

Meh. I don't think we should do this. I would rather do one of the following:

(a) Make :select (or :equals or whatever we call it) the default selector and normatively say that no other selectors can be inferred (other selectors must be explicitly declared). This ensures that processing is not programming language or runtime dependent, which helps translation tools do consistent stuff:

match {$two}
when 1 {two is a string with only U+0031 in it}
when * {two is anything else}

I don't think this kind of message is at all common, so I would find it odd to have it be the default behaviour. What is the benefit we get from not requiring a selector here?

(b) Syntactically require the annotation, e.g.:

switch = matcher *variant
matcher = match 1*selector
; annotation is not optional unlike in expression
selector = "{" [s] (operand s annotation)/(annotation) [s] "}"

This makes the selector function explicit and makes it a parse error not to say which one to use. It also means that you can't use a declaration for the selector (only as the value of a selector), which I agree is kind of odd.

I don't like this. This would mean we'd need two different expression parsers, which seems really cumbersome. Requiring an annotation would also guide developers towards using different options bags for the selection vs. formatting.

I'll also call out: in our discussion of mutable vs. immutable variables, @mihnita's "13/42" example comes into play here. If we say that this evaluates as "13 and 42":

let $a = {13}
let $b = {$a}
let $a = 42
{{$a} and {$b}}

... but then say that a selector is late binding:

let $a = {$count :plural}
match {$a}
when 1 {exactly 1}
when one {one plural rule}
when * {other plural rule}

... that seems like an inconsistency in favor of "42 42"

The way that I would read this is that we first create a "plural" instance and assign it to $a. Then we use that as a selector. I don't really see how that qualifies as late binding?

@stasm
Copy link
Collaborator

stasm commented Jul 20, 2023

See #425 for context.

#425 has gotten so huge that it's difficult to see what exactly this PR is trying to address. Can you summarize what problem do you think we'd solve by adding this restriction?

@eemeli
Copy link
Collaborator Author

eemeli commented Jul 20, 2023

#425 has gotten so huge that it's difficult to see what exactly this PR is trying to address. Can you summarize what problem do you think we'd solve by adding this restriction?

That's fair. 😅

The main beneficiary of requiring annotations on selectors is translation tooling, which can then better handle locale-sensitive selectors such as plurals.

A secondary benefit is the avoidance of apparently-but-not-really valid messages, such as the one presented by @macchiati in #425 (comment):

match {$count}
when one {...}
when * {...}

@eemeli eemeli requested a review from stasm July 21, 2023 17:51
@eemeli
Copy link
Collaborator Author

eemeli commented Jul 21, 2023

@aphillips @macchiati @mihnita @stasm I've now updated this proposal following our discussion, to also account for transitive references to annotated expressions.

@eemeli eemeli added the Agenda+ Requested for upcoming teleconference label Jul 21, 2023
Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requested.

Co-authored-by: Addison Phillips <[email protected]>
@aphillips
Copy link
Member

Note: I filed #433 to track choosing a name for :string. I think we should merge this as reflecting consensus (assuming it does in tomorrow's call) and follow it up with a name change (if we decide to change the name) separately.

@aphillips aphillips added syntax Issues related with syntax or ABNF formatting Issue pertains to the formatting section of the spec labels Jul 24, 2023
Copy link
Collaborator

@stasm stasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after dropping the "at most one jump" requirement on annotations in local variables.

@aphillips aphillips merged commit 812c6dd into unicode-org:main Jul 24, 2023
@eemeli eemeli deleted the select-annotate branch July 24, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda+ Requested for upcoming teleconference formatting Issue pertains to the formatting section of the spec syntax Issues related with syntax or ABNF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants