Require variable names to be globally unique #402

catamorphism · 2023-06-24T00:24:13Z

aphillips

I oppose banning redefinitions so don't want to merge this at all. I have a couple of technical comments about that.

Most comments below, however, are with my chair hat on and pertain to formatting and normative language.

aphillips · 2023-06-25T18:42:23Z

spec/formatting.md


-It is an error for a local variable definition to
-refer to a local variable that's defined after it in the message.
+It is an error for the right-hand side of a local variable declaration to


the right and left hand sides have names in the ABNF. You should use those. In this case it should be something like:

Suggested change

It is an error for the right-hand side of a local variable declaration to

The `expression` part of a `declaration` MUST NOT refer to a `variable` that is declared after it in the `message`.

(Technically we could just say "an expression MUST NOT refer to a variable that has not been assigned in a previous declaration or passed in...")

+1 to Addison's phrasing.

I made an attempt to rephrase this.

This would be much easier to specify if, in the grammar, declaration and operand were changed:

declaration = let s variable-definition [s] "=" [s] expression operand = literal / variable-reference option = name [s] "=" [s] operand variable-definition = variable variable-reference = variable

If this change were made, then I'd be able to write something like:

"The variable of any variable-reference must be preceded by a full declaration whose variable-definition has the same variable."

and could avoid referring to the enclosing context of "the operand".

aphillips · 2023-06-25T18:43:03Z

spec/formatting.md

+It is an error for the right-hand side of a local variable declaration to
+refer to a local variable that's declared after it in the message.
+
+Variable names are required to be globally unique. That is,


Use normative language.

Suggested change

Variable names are required to be globally unique. That is,

Each `variable` MUST be unique within a message. It is a (what type?) error if a `declaration` uses a name assigned by a previous `declaration`.

in the spirit of the generic comment I left on this PR, I suggest we use "constants" everywhere in this PR (and const keyword). (of course not for the passed-in ones)

@mihnita Note that I'm using production names from the ABNF. If we decided to make the variables constant then we should change the name of the production at the same time.

Rewrote this; for now I'll defer the const vs. let question and try to focus on making the language precise.

aphillips · 2023-06-25T18:45:01Z

spec/formatting.md

+for any local variable declaration of the form `let $v = e`:
+* It is an error if `v` is the left-hand side of a local variable declaration
+that appears before `let $v = e` in the message.
+* It is also an error if `v` is an externally defined variable.


We discussed this briefly in the recent call (talking about changing the sigil for external values). I think this is bad because it is easy for code to suddenly produce an error when the input data (which is dynamic) collides with a declaration.

Nitpick: let $v = e => let $v = {$e} (everywhere)

@aphillips I agree that it would be preferable to change the sigil for externally-defined variables. If that change lands before this PR does (i.e. if #403 is resolved), then I'll update this PR to reflect that.

@mihnita I think my rewrites that I just pushed eliminate metavariables anyway, but if I end up re-adding any I'll keep that in mind.

spec/formatting.md

aphillips · 2023-06-25T18:51:22Z

spec/formatting.md

+
+When there are multiple _declarations_ for the same _variable_,
+the fallback string is formatted based on the _expression_ of
+the first declaration of that _variable_.


what about if var were externally defined? And why is this a good fallback string?

When there are multiple declarations for the same variable,

That would be an error.

If we disallow redefinition

const $_foo = {|horse|} const $_foo = {|dog|} // ERROR

And if we also force names starting with _ on local consts, and disallow names starting with _ in passed variables, then multiple declarations are errors.

what about if var were externally defined?

Rewrote to include that case as well.

And why is this a good fallback string?

See https://github.com/unicode-org/message-format-wg/blob/main/spec/formatting.md?plain=1#L208

I wanted to be as consistent as possible with the existing spec, which says that fallback values should be formatted based on the RHS of a declaration. I thought the most consistent thing if there are multiple declarations is to pick one of them. Do you have another suggestion?

When there are multiple declarations for the same variable,

That would be an error.

@mihnita I'm not sure what you mean -- doesn't there have to be a fallback string to replace any expression that resolves to an error?

mihnita · 2023-06-26T17:58:55Z

I think it would be good to use const instead of let or var, as that would make it a lot more obvious that they should not be overridden, without reading the spec.
I was proposed by Addison last Monday, and I think it is a good idea.

I also had a meeting with Stas last week, and he would really like to see something that mitigates the shadowing between these "local variables" (consts :-) and the "externally defined variables" (edv :-) or "arguments"

I proposed another sigil, but he would rather avoid that, and proposed either $$, or _ in the name.
So local consts would be

const $_foo = {$foo :number ...}
const $foo = {...} // ERROR, local variable name does not start with _
const $_foo = {...} // ERROR, overrides a "externally defined variables" / argument

One of the added benefits (in addition to shadowing) is that we can do better static checking.
If the message uses any _ named placeholder, is must also have a const in the same message.
That is better than using the same convention for locals / externally defined.

I am fine with _. I can even live with $$, although not a fan :-)

stasm · 2023-06-26T18:03:33Z

A small correction:

I proposed another sigil, but he would rather avoid that, and proposed either $$, or _ in the name.

I suggested the HTML approach of requiring a dash - inside the name, e.g. $my-foo. I considered $$ as a prefix, but don't have a strong opinion about it.

I don't think this is the right place to discuss this, however. I'll file a new issue.

mihnita · 2023-06-26T18:05:41Z

spec/formatting.md

+    > let $var = {|horse|}
+    > {The value is {$var}.}
+    > ```
+
  - **Unknown Function errors** occur when an _expression_ includes


Nitpick: would be good to have a code example with a function here

Do you mean a function in the declaration of $var, or a function in the expression that uses $var, or both?

spec/formatting.md

mihnita · 2023-06-26T18:12:22Z

I suggested the HTML approach of requiring a dash - inside the name, e.g. $my-foo. I considered $$ as a prefix, but don't have a strong opinion about it.

Thank you for the correction.

(Personally I am not a fan of - in variable names.
But the misrepresentation of what you said was not intentional.
Sorry)

eemeli

I don't think disallowing value re-definitions is a good idea.

For an alternative approach, could we consider changing the let keyword to be set, and for references to declarations to be changed to definitions or assignments? This might make it easier to consider them as assignments rather than initializations.

I do not think using const would be a good idea, esp. as many such "constants" would be variable depending on the formatting call arguments.

See unicode-org#310

Co-authored-by: Addison Phillips <addisonI18N@gmail.com>

…dd an example of that

catamorphism · 2023-07-05T03:36:27Z

I think it would be good to use const instead of let or var, as that would make it a lot more obvious that they should not be overridden, without reading the spec. I was proposed by Addison last Monday, and I think it is a good idea.

I would be fine with that change, but perhaps if we get to the point where everyone is OK with this PR as-is, then renaming let to const could be a separate PR afterward?

catamorphism · 2023-07-05T03:48:55Z

I don't think disallowing value re-definitions is a good idea.

For an alternative approach, could we consider changing the let keyword to be set, and for references to declarations to be changed to definitions or assignments? This might make it easier to consider them as assignments rather than initializations.

I filed a new issue for this: #413

eemeli · 2023-09-02T10:23:57Z

As discussed during the last MFWG call, changes to how we handle variable names probably needs a design document.

catamorphism · 2023-11-01T19:27:05Z

I think this pull request is made obsolete by 5065a51 .

catamorphism force-pushed the unique-variables branch 3 times, most recently from bc6be29 to 77f2e6c Compare June 24, 2023 00:34

aphillips requested changes Jun 25, 2023

View reviewed changes

mihnita reviewed Jun 26, 2023

View reviewed changes

spec/formatting.md Outdated Show resolved Hide resolved

stasm mentioned this pull request Jun 26, 2023

Allow to statically analyze messages to detect undefined local variables #403

Closed

eemeli requested changes Jul 3, 2023

View reviewed changes

Require variable names to be globally unique

8c2aecb

See unicode-org#310

catamorphism force-pushed the unique-variables branch from 77f2e6c to 8c2aecb Compare July 5, 2023 02:34

Clarify definition of use-before-definition error

05ba489

catamorphism force-pushed the unique-variables branch from 0e19f21 to 05ba489 Compare July 5, 2023 03:04

Rewrite the 'globally unique' property to use normative language

8c8acad

catamorphism force-pushed the unique-variables branch from 0f55fa6 to 8c8acad Compare July 5, 2023 03:12

catamorphism and others added 3 commits July 4, 2023 20:17

Avoid normative language in examples

dd3674d

Apply suggestions from code review

aebde67

Co-authored-by: Addison Phillips <addisonI18N@gmail.com>

Clarify fallback string when an external variable is redefined, and a…

828c748

…dd an example of that

catamorphism mentioned this pull request Jul 5, 2023

Should variables be mutable? #413

Closed

catamorphism mentioned this pull request Jul 5, 2023

Replace (literal / variable) with operand in definition of option #412

Closed

cdaringe approved these changes Jul 20, 2023

View reviewed changes

catamorphism closed this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require variable names to be globally unique #402

Require variable names to be globally unique #402

catamorphism commented Jun 24, 2023

aphillips left a comment

aphillips Jun 25, 2023

mihnita Jun 26, 2023

catamorphism Jul 5, 2023 •

edited

Loading

aphillips Jun 25, 2023

mihnita Jun 26, 2023

aphillips Jun 30, 2023

catamorphism Jul 5, 2023

aphillips Jun 25, 2023

mihnita Jun 26, 2023

catamorphism Jul 5, 2023

aphillips Jun 25, 2023

mihnita Jun 26, 2023 •

edited

Loading

catamorphism Jul 5, 2023

catamorphism Jul 5, 2023

mihnita commented Jun 26, 2023

stasm commented Jun 26, 2023 •

edited

Loading

mihnita Jun 26, 2023

catamorphism Jul 5, 2023

mihnita commented Jun 26, 2023

eemeli left a comment •

edited

Loading

catamorphism commented Jul 5, 2023

catamorphism commented Jul 5, 2023

eemeli commented Sep 2, 2023

catamorphism commented Nov 1, 2023

	It is an error for the right-hand side of a local variable declaration to
	The `expression` part of a `declaration` MUST NOT refer to a `variable` that is declared after it in the `message`.

	Variable names are required to be globally unique. That is,
	Each `variable` MUST be unique within a message. It is a (what type?) error if a `declaration` uses a name assigned by a previous `declaration`.

Require variable names to be globally unique #402

Require variable names to be globally unique #402

Conversation

catamorphism commented Jun 24, 2023

aphillips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catamorphism Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihnita Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihnita commented Jun 26, 2023

stasm commented Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihnita commented Jun 26, 2023

eemeli left a comment • edited Loading

Choose a reason for hiding this comment

catamorphism commented Jul 5, 2023

catamorphism commented Jul 5, 2023

eemeli commented Sep 2, 2023

catamorphism commented Nov 1, 2023

catamorphism Jul 5, 2023 •

edited

Loading

mihnita Jun 26, 2023 •

edited

Loading

stasm commented Jun 26, 2023 •

edited

Loading

eemeli left a comment •

edited

Loading