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.

Sign up for GitHub

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

Include explicit text on literal option origin #1019

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b3b570
Include explicit text on literal option origin
eemeli Feb 17, 2025
57a1a10
Clarify normative requirement
eemeli Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion spec/formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ as well as a flag to indicate whether
its formatted representation requires isolation
from the surrounding text.

To allow for _function handlers_ to ensure that certain _option_ values are set by _literals_,
the _resolved value_ of each _option_ value MUST include information about its source
as either a _literal_ or _variable_.

The form that _resolved values_ take is implementation-dependent,
and different implementations MAY choose to perform different levels of resolution.

Expand All @@ -153,6 +157,7 @@ and different implementations MAY choose to perform different levels of resoluti
> selectKeys(keys: string[]): string[]
> directionality(): 'LTR' | 'RTL' | 'unknown'
> isolate(): boolean
> isLiteralOptionValue(): boolean
> }
> ```
>
Expand Down Expand Up @@ -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

@gibson042 gibson042 Feb 17, 2025

Choose a reason for hiding this comment

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

👍

The order of _options_ MUST NOT be significant.

> For example, the following _message_ treats both both placeholders identically:
Expand All @@ -386,6 +391,8 @@ For each _option_:
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`.
Comment on lines 391 to 397
Copy link
Collaborator

@gibson042 gibson042 Feb 17, 2025

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

@eemeli eemeli Feb 18, 2025

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

@macchiati macchiati Feb 18, 2025

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 Feb 18, 2025

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

@eemeli eemeli Feb 18, 2025

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

@gibson042 gibson042 Feb 18, 2025

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

@gibson042 gibson042 Feb 18, 2025

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

@eemeli eemeli Feb 18, 2025

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

@macchiati macchiati Feb 18, 2025

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.


Expand Down