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.

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

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Mar 7, 2024

As noted in #704 (review), this was already fixed once in #577, but mistakenly reverted in the merge of #593.

@eemeli eemeli added fast-track Editorial change permitted to use fast-track merge rules editorial Issue is non-normative formatting Issue pertains to the formatting section of the spec labels Mar 7, 2024
@eemeli eemeli requested a review from aphillips March 7, 2024 05:02
@@ -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

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:

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:
_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:

_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.

@aphillips
Copy link
Member

Need one more approval to commit

Copy link
Collaborator

@catamorphism catamorphism left a comment

Choose a reason for hiding this comment

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

Instead of deleting the text, I'd like to replace it with something like "assert that the option doesn't already exist". However, that only makes sense if #703 lands, since otherwise, the implementation would be free to recover from the previously-emitted data model error. So this is fine, but I added a note to that effect in #703.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

As I've said, I think the current spec is much too proscriptive about the exact "phase" in which an error must be emitted, because requires a particular implementation model.

So removing these kinds of lines is definitely the right direction.

@aphillips aphillips merged commit afc7ff8 into main Mar 12, 2024
@aphillips aphillips deleted the duplicate-option-name-as-data-model-error branch March 12, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Issue is non-normative fast-track Editorial change permitted to use fast-track merge rules formatting Issue pertains to the formatting section of the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants