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

Do not use variable rule within declaration #440

Closed
wants to merge 1 commit into from

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Jul 25, 2023

This applies the suggestion I originally made in #412 (comment), i.e. changes the declaration rule to be:

declaration = let s "$" name [s] "=" [s] expression

This makes it a bit clearer that a variable is always a reference to something, rather than doing double duty as also the target of a let statement.

The declaration text is correspondingly modified and updated to properly refer to relevant definitions. I've intentionally kept the language very close to its current shape in order to not inveigle this in the larger ongoing discussions about variable shadowing and namespacing.

Ping @catamorphism, whom I apparently can't add as a reviewer.

@eemeli eemeli requested review from aphillips and stasm July 25, 2023 18:52
@aphillips
Copy link
Member

I don't agree with this, at least not yet, because variable is not doing "double-duty" currently. The object of a let is, in fact, a variable. The production variable is also used to allow variables into operand and it currently also covers external names, not just declarations. I think we should resolve our conversation about namespaces and mutability and shadowing and such rather than fooling with the ABNF. Those choices will then tell us how to make semantic and/or structural changes to the ABNF.


@catamorphism I will look into Tim's difficulties in participating in our group.

@catamorphism
Copy link
Collaborator

I like the change to make the distinction between variable declarations and variable references clearer. (I've been added to the message-format-wg org now, so you should be able to add me as a reviewer if you want.)

@eemeli eemeli requested a review from catamorphism July 26, 2023 07:10
@stasm
Copy link
Collaborator

stasm commented Jul 26, 2023

I continue to think that we're abusing slightly the ABNF when we attempt to represent data model concepts in it. In my mind, the ABNF describes the grammar, and doesn't need to map 1-1 to the data model. That said, I don't want to block such changes if others find them useful.

How about going for something even more explicit: separate variable name from variable reference, too?

variable-declaration = let s variable-name [s] "=" [s] expression
variable-reference = variable-name
variable-name = "$" name

@catamorphism
Copy link
Collaborator

I continue to think that we're abusing slightly the ABNF when we attempt to represent data model concepts in it. In my mind, the ABNF describes the grammar, and doesn't need to map 1-1 to the data model.

That's a fair point. Typically, a programming language has a concrete syntax and an abstract syntax. The ABNF represents the concrete syntax. The interfaces in the recently-added https://github.com/unicode-org/message-format-wg/blob/main/spec/data-model.md represent the abstract syntax. Differentiating variable declarations from references is usually done in the abstract syntax (and in this case, it already is). However, the spec is written in terms of the concrete syntax, and if it's going to remain so, I think it's worth structuring the concrete syntax in ways that make it easier to write a clear specification.

@aphillips
Copy link
Member

@stasm @catamorphism The purpose of the ABNF is to aid in the implementation. Interstitial constructs are often used because they help implementers understand the grammar or because they represent useful "intermediate states" in the processing.

I think that some blending of the abstract syntax is helpful, though. For example, in the rewrite of the syntax spec, it's useful to give names to the different types of expression, particularly since we intend to constrain the "selector" ones in specific ways and since "placeholder" is a useful term inside patterns. Whether those reach into the ABNF, though, is another question. I would much rather see:

; expressions inside a pattern are called "placeholders"
pattern = "{" * (text / expression) "}"

than:

placeholder = expression
pattern = "{" *(text / placeholder) "}"

At the same time, I oppose making this change because it removes meaning for the users of our ABNF and our spec. I'll make an analogy here. Have you read URI (RFC3986)? The terms scheme, authority, path, query, and fragment are extremely useful in understanding how URLs are structured and work. But none of them are actually required in order to create a parser-friendly grammar. They exist to allow users to conceptually grok the different parts of a URL and allow the prose to meaningfully interact with the grammar.

The term variable in this PR makes the grammar very easy to understand and also provides a useful way to refer to variables when talking about declarations. The bare processing doesn't require the concept of a "variable", but neither does it require the concept of an "expression" either. Why not just reductio ad absurdum the rule 😉

; I could go further and break up "annotation" eh?
declaration = "let" s "$" name [s] "=" [s] "{" [s] (( literal / "$" name ) [s annotation] / annotation) [s] "}"

@aphillips
Copy link
Member

This was discussed in the 2023-08-07 call with some opposition to merging. Please update to address conflicts produced by merging #441 and we'll discuss at next call.

@eemeli
Copy link
Collaborator Author

eemeli commented Aug 28, 2023

Rebased to resolve differences.

@aphillips
Copy link
Member

The syntax changes for input and local appear to have made this obsolete.

@eemeli
Copy link
Collaborator Author

eemeli commented Oct 29, 2023

That's a fair point. Reviewing this with respect to our current syntax with its input-declaration and local-declaration, where the former includes a variable-declaration which itself includes a variable, the changes proposed here would not really clarify anything.

So I'm withdrawing this PR to have one fewer thing to quibble over.

@eemeli eemeli closed this Oct 29, 2023
@eemeli eemeli deleted the declare-name branch October 29, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants