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

Update the rest of the spec to match the ABNF after adding .keywords #548

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Dec 1, 2023

This applies the changes from #529 to the rest of the specification (syntax, data model, formatting).

The ABNF is also updated editorially to include the placeholder and matcher described in the syntax.

A new Unsupported Statement error is added.

The data model changes also account for the change from let to local and input, which was not previously done.

@aphillips aphillips added syntax Issues related with MF Syntax specification formatting labels Dec 1, 2023
spec/data-model/README.md Outdated Show resolved Hide resolved
spec/data-model/README.md Outdated Show resolved Hide resolved
spec/data-model/README.md Outdated Show resolved Hide resolved
Comment on lines +137 to +142
type Expression = LiteralExpression | VariableExpression | FunctionExpression;

interface LiteralExpression {
arg: Literal;
func?: FunctionRef | UnsupportedExpression;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with the data model implied by the grammar, func fields should probably be renamed (and possibly FunctionExpression as well, in both the grammar and here).

Suggested change
type Expression = LiteralExpression | VariableExpression | FunctionExpression;
interface LiteralExpression {
arg: Literal;
func?: FunctionRef | UnsupportedExpression;
}
type Expression = LiteralExpression | VariableExpression | AnnotationExpression;
interface LiteralExpression {
arg: Literal;
annotation?: FunctionRef | UnsupportedAnnotation;
}

etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One challenge with "annotation" is that when it's in an expression without an operand, what is it "annotating"?

Copy link
Member

Choose a reason for hiding this comment

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

The expression.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To me this sounds rather tautological. I'd be happy for us to discuss this naming briefly during tomorrow's call, and to go with whatever the majority prefers (as in, spend max 5 min and either reach a conclusion or continue from there async).

Copy link
Member

Choose a reason for hiding this comment

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

If you glance at the agenda you'll notice that I have a bunch of these short highly timeboxed issues for tomorrow. I've added this one.


I'll note that the current ABNF uses the term function-expression and the data model should basically match the ABNF word-for-word to the extent possible. Keeping FunctionExpression seems fine?

I do think that this might not be right, though:

  annotation?: FunctionRef | UnsupportedAnnotation;

UnsupportedAnnotation applies to reserved-annotation but not necessarily to private-use-annotation. Private use is only supported if the implementation says it is. So we need a third bucket for private use to go into so that implementations that support the PU can consume it, e.g.:

  annotation?: FunctionRef | PrivateUseAnnotation | UnsupportedAnnotation

(it is possible for a private-use to be unsupported)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the cases where the implementation does support a specific private-use annotation, we have this:

When parsing the syntax of a _message_ that includes a _private-use_ _annotation_
supported by the implementation,
the implementation SHOULD represent it in the data model
using an interface appropriate for the semantics and meaning
that the implementation attaches to that _annotation_.

And then in the Extensions section, we specify that an implementation is allowed to extend the data model for that purpose. Or at least it's intended to do so.

So we can at this point lump all of the unsupported reserved & private-use into this one basket of the data model, rather than needing to split it as in the syntax.

Copy link
Member

Choose a reason for hiding this comment

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

If the data-model is meant to be interchanged, does that mean that the "private-use-ness" might be a relevant detail to the consumer? I mean, maybe it's FunctionRef but if it weren't, it wouldn't be UnsupportedAnnotation necessarily either? Or am I barking up the wrong tree? 🌳

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If an implementation does define and support a private-use annotation, I think there are two options for its data model representation:

  1. It's mapped to some representation within the base data model, e.g. as some func value or maybe as an operand of some sort. But then why need the new syntax in the first place?
  2. The annotation is doing something truly different, and it doesn't fit within the base data model. In this case, the implementation extends the base, e.g. adding a new field in the Expression interfaces.

If going the first route, then interchange is of course easier, but that syntax will also have some representation without private use. If going the second route, interchange will require both ends to agree about the private-use.

spec/syntax.md Outdated Show resolved Hide resolved
spec/syntax.md Outdated Show resolved Hide resolved
spec/syntax.md Outdated Show resolved Hide resolved
spec/syntax.md Outdated Show resolved Hide resolved
eemeli and others added 2 commits December 2, 2023 23:43
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@eemeli
Copy link
Collaborator Author

eemeli commented Dec 2, 2023

Dropped the matcher reserved-statement, as discussed in #548 (comment).

@aphillips aphillips merged commit b60d250 into main Dec 4, 2023
@aphillips aphillips deleted the dot-keywords-more branch December 4, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting specification syntax Issues related with MF Syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants