-
-
Notifications
You must be signed in to change notification settings - Fork 36
Add error handling #320
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
Add error handling #320
Changes from 10 commits
1a27ccd
4389ae5
55890b9
99ce4a1
da1ecaf
4d8c753
2e1f943
38e33ad
8e12260
22866fd
d46856f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,3 +18,248 @@ the local variable takes precedence. | |
|
||
It is an error for a local variable definition to | ||
refer to a local variable that's defined after it in the message. | ||
|
||
## Error Handling | ||
|
||
Errors in messages and their formatting may occur and be detected | ||
at multiple different stages of their processing. | ||
Where available, | ||
the use of validation tools is recommended, | ||
as early detection of errors makes their correction easier. | ||
|
||
During the formatting of a message, | ||
various errors may be encountered. | ||
These are divided into the following categories: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick / editorial: It would help with the "big picture", vs something spread over many paragraphs.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's how this started out, before the examples got added... Let's revisit this when we've more of the spec gathered up? The solution for this section is likely to look very similar to other sections as well. |
||
|
||
- **Syntax errors** occur when the syntax representation of a message is not well-formed. | ||
|
||
Example invalid messages resulting in a Syntax error: | ||
|
||
``` | ||
{Missing end brace | ||
``` | ||
|
||
``` | ||
{Unknown {?placeholder}} | ||
aphillips marked this conversation as resolved.
Show resolved
Hide resolved
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
``` | ||
let $var = {(no message body)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you clarify this one? Is it illegal to assign the empty string or null to a variable? I think it might be a feature in some situations... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The empty pattern is not an error, though, right? It's not a very useful message, but it's not an error, is it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's an error because it's missing the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you're saying that the empty string is not a valid pattern and results in an error? That is, the minimum a pattern string can ever be is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is what the current EBNF states. |
||
``` | ||
|
||
- **Data Model errors** occur when a message is invalid due to | ||
violating one of the semantic requirements on its structure: | ||
|
||
- **Variant Key Mismatch errors** occur when the number of keys on a Variant | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Part of the editorial cleanup I'm proposing, that would add back-ticks around keywords, and would make sure this is consistent with the spec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer leaving this as "Variant Key" for now, as it's rather likely for this to leak out as a user-visible string in the error. |
||
does not equal the number of Selectors. | ||
|
||
Example invalid messages resulting in a Variant Key Mismatch error: | ||
|
||
``` | ||
match {$one} | ||
when 1 2 {Too many} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having too few or too many keys (compared to the match expression) is clearly a syntax error, and should be in that category. It's like calling a function in C or Java with too few or too many parameters; not a semantic or data model error, but rather one that can be detected purely because of malformed syntax. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intent in differentiating this as a "data model" rather than "syntax" error is that it allows us to apply the same error check independently of the syntax used by the message. Here's what I said about this earlier in #320 (comment):
An MF2 syntax parser would be expected to emit the data model errors during its work, as this is included in the spec (line 176):
This separation also matches the differentiation between "well-formed" and "valid" messages in the syntax spec. |
||
when * {Otherwise} | ||
``` | ||
|
||
``` | ||
match {$one} {$two} | ||
when 1 2 {Two keys} | ||
when * {Missing a key} | ||
when * * {Otherwise} | ||
``` | ||
|
||
- **Missing Fallback Variant errors** occur when the message | ||
does not include a Variant with only catch-all keys. | ||
|
||
Example invalid messages resulting in a Missing Fallback Variant error: | ||
|
||
``` | ||
match {$one} | ||
when 1 {Value is one} | ||
when 2 {Value is two} | ||
``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add text to say:
(and on line 82 add "Missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the necessity for these specifiers. Are these specific examples more difficult to respond about than the others, or should we specify explicitly what's wrong in each of them? Regarding the examples in general, I at least prefer requiring the reader to at least momentarily pause on some of them to figure out for themselves what's going on, rather than making sure that no thought is required. |
||
|
||
``` | ||
match {$one} {$two} | ||
when 1 * {First is one} | ||
when * 1 {Second is one} | ||
``` | ||
|
||
- **Resolution errors** occur when the runtime value of a part of a message | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is unclear what this means. Can we say "function resolution"?
We already have unresolved as a class of errors a bit below. So what about we call this section "Function resolution"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference between the "resolution" and "formatting" errors that's proposed here is perhaps best seen by considering how a Placeholder containing an Expression is handled. Let's say that we have something like
or
where During the formatting of this, the custom code could then emit two different sorts of errors:
Would you agree that these are different error categories, and that this categorical split could lead to different error handling in user code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If If no In any case, I think I would move this item below some of the others here (perhaps to the bottom of the list), since I find myself thinking that this could also mean unresolved or formatting error when in fact this error would probably only occur later (only when the pattern is syntactically correct and all of the variables and functions have been resolved but there is still a problem). Or am I still not understanding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both The intent here is to allow for a custom formatting function to emit two different kinds of errors: Resolution and Formatting. This is meant to enable something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I don't think what I am saying here affects the spec, but the answer above. I think it is contradicting:
It is unclear what is the difference between the two: "get the value" and "resolve variable reference" But that implementation detail should not "leak" in the kind of error I am getting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note that the syntax example for the above discussion is this placeholder:
The
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cannot be determined. | ||
|
||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- **Unresolved Variable errors** occur when a variable reference cannot be resolved. | ||
|
||
For example, attempting to format either of the following messages | ||
must result in an Unresolved Variable error if done within a context that | ||
does not provide for the variable reference `$var` to be successfully resolved: | ||
|
||
``` | ||
{The value is {$var}.} | ||
``` | ||
|
||
``` | ||
match {$var} | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
when 1 {The value is one.} | ||
when * {The value is not one.} | ||
``` | ||
|
||
- **Unknown Function errors** occur when an Expression includes | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
a reference to a function which cannot be resolved. | ||
|
||
For example, attempting to format either of the following messages | ||
must result in an Unknown Function error if done within a context that | ||
does not provide for the function `:func` to be successfully resolved: | ||
|
||
``` | ||
{The value is {(horse) :func}.} | ||
``` | ||
|
||
``` | ||
match {(horse) :func} | ||
when 1 {The value is one.} | ||
when * {The value is not one.} | ||
``` | ||
|
||
- **Selection errors** occur when message selection fails. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one bullet (Selection errors) with only one sub-bullet (Selector errors) Otherwise feels redundant, like having:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave the selection + selector error categories as is for now, but adjust them later after #322 is resolved. |
||
|
||
- **Selector errors** are failures in the matching of a key to a specific selector. | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For example, attempting to format either of the following messages | ||
may result in a Selector error if done within a context that | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
uses a `:plural` selector function which requires its input to be numeric: | ||
|
||
``` | ||
match {(horse) :plural} | ||
when 1 {The value is one.} | ||
when * {The value is not one.} | ||
``` | ||
|
||
``` | ||
let $sel = {(horse) :plural} | ||
match {$sel} | ||
when 1 {The value is one.} | ||
when * {The value is not one.} | ||
``` | ||
|
||
- **Formatting errors** occur during the formatting of a resolved value, | ||
for example when encountering a value with an unsupported type | ||
or an internally inconsistent set of options. | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For example, attempting to format any of the following messages | ||
may result in a Formatting error if done within a context that | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
1. provides for the variable reference `$user` to resolve to | ||
an object `{ name: 'Kat', id: 1234 }`, | ||
2. provides for the variable reference `$field` to resolve to | ||
a string `'address'`, and | ||
3. uses a `:get` formatting function which requires its argument to be an object and | ||
an option `field` to be provided with a string value, | ||
|
||
``` | ||
{Hello, {(horse) :get field=name}!} | ||
``` | ||
|
||
``` | ||
{Hello, {$user :get}!} | ||
``` | ||
|
||
``` | ||
let $id = {$user :get field=id} | ||
{Hello, {$id :get field=name}!} | ||
``` | ||
|
||
``` | ||
{Your {$field} is {$id :get field=$field}} | ||
``` | ||
|
||
Syntax and Data Model errors must be emitted as soon as possible. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proposal: add a heading above it? Because we go from the list of error types (what we produce), with examples for each, to a section explaining how / when to produce said errors. It probably made sense when the list errors was a real list, visible at a glance. If you think it helps also add a ### level heading before error types.
|
||
|
||
During selection, an Expression handler must only emit Resolution and Selection errors. | ||
During formatting, an Expression handler must only emit Resolution and Formatting errors. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above: This phrasing doesn't explicitly mandate that errors should chain (or nest, depending how you look at it). I.e a resolution error can lead to a formatting error. Implementations should emit both to enable higher-level abstractions to react differently to these different failures. |
||
|
||
In all cases, when encountering an error, | ||
a message formatter must provide some representation of the message. | ||
An informative error or errors must also be separately provided. | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
When a message contains more than one error, | ||
or contains some error which leads to further errors, | ||
an implementation which does not emit all of the errors | ||
should prioritise Syntax and Data Model errors over others. | ||
aphillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
When an error occurs in the resolution of an Expression or Markup Option, | ||
the Expression or Markup in question is processed as if the option were not defined. | ||
Comment on lines
+189
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this lead to unresolved variable errors later that would otherwise be avoided? See my comment about allowing the empty variable above. Wouldn't it be better to have just the one error for:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's tricky, because determining whether accessing
or as
I'll add a statement clarifying that it should be the latter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... because the first error ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort of, but not quite. It's probably easiest to think of the value of This indirection allows for us to consider the fallback value and the target of the formatting separately from each other, and more easily support non-string formatting targets.
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This may allow for the fallback handling described below to be avoided, | ||
though an error must still be emitted. | ||
|
||
When an error occurs within a Selector, | ||
the selector must not match any VariantKey other than the catch-all `*` | ||
and a Resolution or Selector error is emitted. | ||
|
||
## Fallback String Representations | ||
|
||
The formatted string representation of a message with a Syntax or Data Model error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better for each error to define the fallback representation rather than trying to collect them here? For example "The fallback string representation of an unknown function error is ..." in the section on that type of error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer defining the fallback representation according to the Placeholder for which it applies, rather than the error type. This makes them more predictable and intentionally restricts implementations from trying to be too smart about the behaviour. Consider for instance something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I meant something different? I meant that instead of defining fallbacks in a separate section of the spec which then refers back to syntax and data model error types, it would potentially better to define error behavior inline with each error type. I am not suggesting that we encourage/permit different implementations to make up their own fallback behavior or to define new error types with (suspect) fallback behavior. One reason for my thinking is the example in the comment above:
If I'm writing an implementation, I probably have an assignment operator bit of code for the |
||
is the concatenation of U+007B LEFT CURLY BRACKET `{`, | ||
a fallback string, | ||
and U+007D RIGHT CURLY BRACKET `}`. | ||
If a fallback string is not defined, | ||
the U+FFFD REPLACEMENT CHARACTER `�` character is used, | ||
resulting in the string `{�}`. | ||
|
||
When an error occurs in a Placeholder that is being formatted, | ||
the fallback string representation of the Placeholder | ||
always starts with U+007B LEFT CURLY BRACKET `{` | ||
and ends with U+007D RIGHT CURLY BRACKET `}`. | ||
Comment on lines
+210
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we might have a bidi consideration here? If the string being formatted is in Arabic, we might emit an FSI before the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be covered by the text in PR #315? The intent there is to define a prefix and a suffix to a part's formatted string representation, which here would be something like |
||
Between the brackets, the following contents are used: | ||
|
||
- Expression with Literal Operand: U+0028 LEFT PARENTHESIS `(` | ||
followed by the value of the Literal, | ||
and then by U+0029 RIGHT PARENTHESIS `)` | ||
eemeli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Examples: `{(horse)}`, `{(42)}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be an error? Ever? I agree that it can make sense if a function is specified: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The examples are showing examples of the fallback string representations, so .e.g |
||
|
||
- Expression with Variable Operand: U+0024 DOLLAR SIGN `$` | ||
followed by the Variable Name of the Operand | ||
|
||
Example: `{$user}` | ||
|
||
- Expression with no Operand: U+003A COLON `:` followed by the Expression Name | ||
|
||
Example: `{:platform}` | ||
|
||
- Markup start: U+002B PLUS SIGN `+` followed by the MarkupStart Name | ||
|
||
Example: `{+tag}` | ||
|
||
- Markup end: U+002D HYPHEN-MINUS `-` followed by the MarkupEnd Name | ||
|
||
Example: `{-tag}` | ||
|
||
- Otherwise: The U+FFFD REPLACEMENT CHARACTER `�` character | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proposal: change this to "the string between So probably a syntax error (bad sigil, for example, or bad format for parameters): There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is catching anything not covered by the above. I think that's currently an empty set, as syntax errors use I would strongly prefer not including any explicit reference to syntax source here, as that would require keeping a reference to it in all implementations, and might not apply at all if the source is not an MF2 syntax message. |
||
|
||
Example: `{�}` | ||
|
||
Option names and values are not included in the fallback string representations. | ||
|
||
When an error occurs in an Expression with a Variable Operand | ||
and the Variable refers to a local variable Declaration, | ||
the fallback string is formatted based on the Expression of the Declaration, | ||
rather than the Expression of the Placeholder. | ||
|
||
For example, attempting to format either of the following messages within a context that | ||
does not provide for the function `:func` to be successfully resolved: | ||
|
||
``` | ||
let $var = {(horse) :func} | ||
{The value is {$var}.} | ||
``` | ||
|
||
``` | ||
let $var = {(horse)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I don't think the second example should be valid
Because the whole point of local variables would be reuse, mostly for consistency. Yes, not difficult to implement (just a bit worse performance). So my suggestion would be to remove the second example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is meant as a minimal example showing that the identifiers of local variables should never end up being included in the formatted output. It's not really meant to be a useful message, but it's technically valid. |
||
{The value is {$var :func}.} | ||
``` | ||
|
||
would result in both cases with this formatted string representation: | ||
|
||
``` | ||
The value is {(horse)}. | ||
``` |
Uh oh!
There was an error while loading. Please reload this page.