Skip to content

Include explicit text on literal option origin #1019

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 2 commits into from
Feb 18, 2025
Merged

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Feb 17, 2025

On today's call, we decided to merge #1016, and to add text to the main part of the spec clarifying the need to carry the information about an option value's origin in its resolved value. That information is here added in three places:

  • The resolved value definition, with a normative MUST requirement.
  • The resolved value example.
  • The option resolution algorithm.

@eemeli eemeli added functions Issue pertains to the default function set fast-track Editorial change permitted to use fast-track merge rules formatting Issue pertains to the formatting section of the spec labels Feb 17, 2025
@@ -369,7 +374,7 @@ Implementation-defined _functions_ SHOULD use an implementation-defined _namespa

**_<dfn>Option resolution</dfn>_** is the process of computing the _options_
for a given _expression_.
_Option resolution_ results in a mapping of string _identifiers_ to _values_.
_Option resolution_ results in a mapping of string _identifiers_ to _resolved values_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines 391 to 397
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value was set by a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should probably take greater pains to explain transitivity. For example, consider

.input {$x :number}
.local $a = {|EUR|}
.local $b = {$a}
{{Balance: {$x :currency currency=$b}.}}
  1. Per Literal Resolution, the resolved value of $a is "EUR" (and per the new text in Resolved Values, includes its literal nature).
  2. Per Variable Resolution, the resolved value of $b is just the resolved value of $a (i.e., a literal with contents "EUR")—although I cannot determine whether or not implementations are given freedom to drop that propagation by «An implementation MAY perform additional processing when resolving the value of an expression that consists only of a variable».
  3. These new steps in Option Resolution don't add literalness to the resolved value of $b, but it's already there so the :currency function will still see it.

In fact, it seems like the new steps here cannot ever have an effect, because the resolved value of a literal option is marked as such in Literal Resolution and the resolved value of a variable referencing another variable is just the resolved value of its target. So rather than changing this algorithm, the PR should instead accompany it with note(s) explaining the above.

However, more normative text is required is for explaining a variant where .local $b = {$a} is replaced by .local $b = {$a :string}, in which it is not currently clear whether or not Function Resolution grants implementations the freedom to return a result that is marked as a literal. I strongly believe that such freedom should not exist, and that literalness be preserved only by direct isolated variable references.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intent of the changes here is make it so that

{{Balance: {$x :currency currency=EUR}.}}

would have the value of the currency option marked as coming from a literal, while

.local $a = {|EUR|}
{{Balance: {$x :currency currency=$a}.}}

would not.

To make that clearer, maybe the new normative text in Resolved Values needs some work? I'll add a suggestion there.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @eemeli that what he has is sufficient for now.

The stated problems were where the resolved value of a currency option depended on an input parameter, directly or indirectly. However, requiring detection of that finer-grained information, while possible, seems like a lot of overkill. For the use cases we are concerned about in v47, it is sufficient to enable and allow functions to decline variables, but allow literals.

We can examine later whether we need to refine that further, after 47.

Copy link
Member

Choose a reason for hiding this comment

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

I think the text could be misinterpreted. Is there any reason not to say:

      1. If the _option_ value consists of a _literal_:
         1. Include that information in `rv`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That works for me.

@gibson042 Would this and the change suggested for line 142 address your concerns?

Suggested change
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value was set by a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value consists of a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the algorithm also needs an "Else" to strip any "consists of a literal" information from non-literal option values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value was set by a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else, if the _option_ value consists of a _literal_:
1. Set `res[id]` to be `rv`.
1. Else:
1. Let `variableRV` be a copy of `rv` which identifies its source as a _variable_.
1. Set `res[id]` to be `variableRV`.
1. Return `res`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see how that's better than encoding the information "this option value consists of a literal".

Copy link
Member

Choose a reason for hiding this comment

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

I think Eemeli's wording is fine.
Moreover, this is something that can be refined after review by the CLDR TC — which is to happen tomorrow. So let's just merge this.

Comment on lines 391 to 397
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value was set by a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @eemeli that what he has is sufficient for now.

The stated problems were where the resolved value of a currency option depended on an input parameter, directly or indirectly. However, requiring detection of that finer-grained information, while possible, seems like a lot of overkill. For the use cases we are concerned about in v47, it is sufficient to enable and allow functions to decline variables, but allow literals.

We can examine later whether we need to refine that further, after 47.

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.

See if this suggestion solves the problems with the algorithm.

@aphillips aphillips added blocker Blocks the release LDML47 LDML 47 Release (Stable) labels Feb 18, 2025
Comment on lines 391 to 397
1. If `rv` is a _fallback value_:
1. If supported, emit a _Bad Option_ error.
1. Else:
1. If the _option_ value was set by a _literal_:
1. Include that information in `rv`.
1. Set `res[id]` to be `rv`.
1. Return `res`.
Copy link
Member

Choose a reason for hiding this comment

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

I think Eemeli's wording is fine.
Moreover, this is something that can be refined after review by the CLDR TC — which is to happen tomorrow. So let's just merge this.

@aphillips
Copy link
Member

I am merging this now. @gibson042 I will open an issue to track improving this text.

@aphillips aphillips merged commit 185f201 into main Feb 18, 2025
1 check passed
@aphillips aphillips deleted the literally-optional branch February 18, 2025 22:52
eemeli added a commit that referenced this pull request Feb 18, 2025
aphillips pushed a commit that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Blocks the release fast-track Editorial change permitted to use fast-track merge rules formatting Issue pertains to the formatting section of the spec functions Issue pertains to the default function set LDML47 LDML 47 Release (Stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants