Skip to content
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

We need a list of possible error codes #706

Closed
mihnita opened this issue Mar 6, 2024 · 12 comments · Fixed by #850
Closed

We need a list of possible error codes #706

mihnita opened this issue Mar 6, 2024 · 12 comments · Fixed by #850
Labels
errors Issues related to the errors section of the spec LDML46 LDML46 Release (Tech Preview - October 2024) Preview-Feedback Feedback gathered during the technical preview

Comments

@mihnita
Copy link
Collaborator

mihnita commented Mar 6, 2024

I thought we have one, but apparently not.
I grepped for errors in the test/ folder (grep -i -r error ./test/) and what these are the values we have there:
"bad-input", "bad-option", "unresolved-var", "not-formattable"

./test/README.md mentions "Syntax Error" and "Data Model Error".

So I looked for those values everywhere else (from the root of the repo):

grep -r -i 'bad\S*input' .
grep -r -i 'bad\S*option' .
grep -r -i 'unresolved\S*var' .
grep -r -i 'not\S*formattable' .

The only instances are the ones in the test files.

Should we create error codes (to return in the implementation) from the headings in ./spec/errors.md ?

For example ### Duplicate Declaration => duplicate-declaration, ## Formatting Errors => formatting-error?

@aphillips aphillips added Preview-Feedback Feedback gathered during the technical preview errors Issues related to the errors section of the spec labels Mar 6, 2024
@aphillips
Copy link
Member

Isn't this more of an implementation detail?

The types of errors are somewhat general and we don't say exactly what they entail. Partly this is because different programming languages have widely varying needs. You may be right that having a nice succinct token is easier to deal with than our prose-y names. But equally an implementation might assign numbers or assign names IN_VARIOUS-distinct_and_locally_Idiosyncratic_Forms 😄


I think that your implementation experience will be valuable here. Are the categories we have reasonable when implementing? When there are multiple error types that could be thrown for a given error, is what the spec says what you find most usable? Is there an error you encounter that we don't have (as I found a couple weeks ago)? I have questions about error handling that implementation experience should inform.

For example, resolve selectors says to emit a Selection Error if the resolved value of a selector is not "selectable". One reason that a selector would not be selectable is that it is missing its annotation, but Missing Selector Annotation is (correctly, I think?) a data model error, not a selection error.

@macchiati
Copy link
Member

macchiati commented Mar 6, 2024 via email

@aphillips
Copy link
Member

@macchiati

The first one would be a Selection Error. A concrete example in LDML45 would be .match {$count :datetime} because :datetime is not a valid selector.

The second one is a Selection Error also, because :number says it is in registry.md. (Note that a different function might just ignore an unsupported value or rank it lower). It's not clear whether we should have sub-species of Selection Error yet. None of these is a Data Model Error because those are evaluated without the need for the registry.

Here are the DM errors:

.match {$count}
* {{... is a Missing Selector Annotation error}}

.match{$count :number}
* * {{... is a Variant Key Mismatch error (should only have one key)}}

.match{$count :number}
1 {{... is fine as a variant, but this message produces a Missing Fallback Variant because it has no *}}

.match {$count :number minimumFractionDigits=2 minimumFractionDigits=3}
* {{... produces a Duplicate Option Name error, but may execute if the impl wants to continue}}

.input {$var :number}
.local $var = {$foo :number}
{{... produces a Duplicate Declaration error.}}

I think (but the spec is silent about it) that an implementation could produce different error types that group as "Selection Errors". The key thing about a Selection Error is that it means that a pattern cannot be selected. This means that we emit the fallback pattern (typically the logo, aka {�}).

@macchiati
Copy link
Member

macchiati commented Mar 7, 2024 via email

@eemeli
Copy link
Collaborator

eemeli commented Mar 7, 2024

Isn't this more of an implementation detail?

This has been discussed in a few of our recent meetings, and I agree with @mihnita that common error codes would be useful to implementers and users. The ones currently in the test suite come from the JS implementation, where e.g. the :number implementation emits bad-input here, and bad-option here.

Each of those is a Resolution Error, but they're not currently explicitly included in errors.md. OTOH unresolved-var corresponds to Unresolved Variable, and not-formattable is a generic Formatting Error.

I think (but the spec is silent about it) that an implementation could produce different error types that group as "Selection Errors". The key thing about a Selection Error is that it means that a pattern cannot be selected. This means that we emit the fallback pattern (typically the logo, aka {�}).

Yes, an implementation or a custom function could emit other Selection Errors, but no, that would not result in formatting to a {�}. Instead, the algorithm selects the catch-all pattern.

This persisting mis-reading of our pattern selection logic suggests to me that we might want to reconsider first-choice selection during the tech preview. Its algorithmic spec language would be significantly simpler and easier to understand.

@catamorphism
Copy link
Collaborator

Isn't this more of an implementation detail?

I don't think so -- the tests are part of the spec and the expected error names are part of the tests.

@macchiati
Copy link
Member

macchiati commented Mar 9, 2024 via email

@mihnita
Copy link
Collaborator Author

mihnita commented Mar 9, 2024

we might want to reconsider firstchoice selection during the tech preview.

I am completely against making any such change only days before we have to submit the changes to ICU.

And this is something we discussed, more than once, we voted on it.
Changing it to first match is also incompatible with MF1.


That has also nothing to do with the error codes, discussed here.

Some of the errors are obvious, have nothing to do with the selection algorithm, or the specific selector functions:

Three selectors, one key:

.match {$foo} {$baz} {$bar}
   red {{ ... }
   * {{ ... }}

Double key cases:

.match {$foo}
   red {{ ... }
   red {{ ... }
   * {{ ... }}

The exact errors might be implementation details.
And some implementations might report the same errors, but at different stages (parse vs formatting)

But if we say they are implementation details, then we should not have them in the spec (/spec/errors.md)

And should not have them in the json tests suites.

Make the whole document FYI only.
Without ever saying "MUST emit" in that doc :-)

@macchiati
Copy link
Member

macchiati commented Mar 9, 2024 via email

@aphillips aphillips added the Agenda+ Requested for upcoming teleconference label Mar 15, 2024
@aphillips aphillips added LDML46 LDML46 Release (Tech Preview - October 2024) and removed Agenda+ Requested for upcoming teleconference labels Apr 8, 2024
@catamorphism
Copy link
Collaborator

The test schema contains a list of error codes, so this issue can be closed, right?

@eemeli
Copy link
Collaborator

eemeli commented Jul 30, 2024

During the 2024-07-22 call we identified a need to document in test/README.md the relationship between the error codes we use in tests, and the errors enumerated in spec/errors.md. The PR for that ought to close this issue. I don't think anyone specifically volunteered to add that markdown table.

catamorphism added a commit to catamorphism/message-format-wg that referenced this issue Jul 30, 2024
@catamorphism
Copy link
Collaborator

I don't think anyone specifically volunteered to add that markdown table.

Submitted #850 to add the table :)

@eemeli eemeli linked a pull request Jul 30, 2024 that will close this issue
catamorphism added a commit that referenced this issue Aug 5, 2024
* Add table of errors to test README (#706)

* Update test/README.md

Co-authored-by: Eemeli Aro <[email protected]>

* Reverse table

---------

Co-authored-by: Eemeli Aro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues related to the errors section of the spec LDML46 LDML46 Release (Tech Preview - October 2024) Preview-Feedback Feedback gathered during the technical preview
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants