Skip to content

Clarify (again) that Duplicate Option Name is a data model error #710

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 1 commit into from
Mar 12, 2024
Merged

Clarify (again) that Duplicate Option Name is a data model error #710

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions spec/formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,6 @@ The result of resolving _option_ values is a mapping of string identifiers to va
For each _option_:

- Resolve the _identifier_ of the _option_.
- If the _option_'s _identifier_ already exists in the resolved mapping of _options_,
emit a _Duplicate Option Name_ error.
Copy link
Member

@aphillips aphillips Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Duplicate Option Name error is a data model error? This is where an implementation might detect it.

Also, note that other text a bit later seems to suggest that this isn't fatal:

Errors MAY be emitted during option resolution, but it always resolves to some mapping of string identifiers to values. This mapping can be empty.

"always resolves to some mapping" means that the option array is returned. For at least some messages, the results might be recoverable (i.e. only replacing the expression with the fallback rather than the whole message)

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 current formatting spec language assumes that it's dealing with a valid message, i.e. one with no data model errors. Duplicate Option Name errors are primarily defined in errors.md, but for messages coming from syntax, they're also defined here:

message-format-wg/spec/syntax.md

Lines 567 to 568 in e761964

Each _option_'s _identifier_ MUST be unique within the _annotation_:
an _annotation_ with duplicate _option_ _identifiers_ is not valid.

and as with all other data model errors they're required to be detected and emitted before formatting:

message-format-wg/spec/errors.md

Lines 11 to 14 in e761964

_Syntax Errors_ and _Data Model Errors_ apply to all message processors,
and MUST be emitted as soon as possible.
The other error categories are only emitted during formatting,
but it might be possible to detect them with validation tools.

So in order to get to this point in formatting, the current spec text must presume that this error has already been checked, and that therefore the condition for triggering it can't happen. So this statement should be removed to remove the current uncertainty it presents.

We also explicitly only allow for Resolution and Formatting errors to be ignored in expressions that aren't being formatted, and not data model errors:

message-format-wg/spec/errors.md

Lines 19 to 21 in e761964

_Resolution Errors_ and _Formatting Errors_ in _expressions_ that are not used
in _pattern selection_ or _formatting_ MAY be ignored,
as they do not affect the output of the formatter.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @eemeli. You're right, we do have that text in errors.md, etc.

I guess what I'm grappling with is: did we make the right choice here. @catamorphism and @mihnita might weigh in about how they've implemented the checks required by data model (that is, when do they check for duplicate options or missing variant keys or duplicate declarations). I'm fine with blowing up the message for all of these with the possible exception of duplicate option names (and even there I still want to blow up)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I mentioned in #703 (just repeating it here to make the threads easier to follow) my implementation does a "static checking" pass for data model errors that are not checked during parsing, which is consistent with what @eemeli is saying.

- If the _option_'s right-hand side successfully resolves to a value,
bind the _identifier_ of the _option_ to the resolved value in the mapping.
- Otherwise, bind the _identifier_ of the _option_ to an unresolved value in the mapping.
Expand Down