Skip to content

Add variable resolution #305

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Add variable resolution #305

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions spec/formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,14 @@ when formatting a message for display in a user interface, or for some later pro

The document is part of the MessageFormat 2.0 specification,
the successor to ICU MessageFormat, henceforth called ICU MessageFormat 1.0.

## Variable Resolution

To resolve the value of a Variable,
its Name is used to identify either a local variable,
or a variable defined elsewhere.
If a local variable and an externally defined one use the same name,
the local variable takes precedence.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies that name shadowing is possible. My current thinking is that it should be OK, and I see use-cases for it. However, I don't recall an explicit discussion on the subject. It might also make sense to start with a stricter design and disallow shadowing at first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rather think that local variables must be allowed to shadow externally provided named arguments.

For instance, I could well imagine translation systems with the following attributes:

  1. Translators are able to define additional local variables for a message.
  2. A developer may pass the same basket of arguments to multiple messages, not all of which use each of them.

If shadowing were not allowed, then a combination of the above could result in a runtime error which would be difficult to discover ahead of time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this part of the change (shadowing) is a bit of an overreach.
It is unrelated to the hoisting problem, and it was not commonly agreed.

In general I don't think it is a good idea to allow translators to override a developer decision.
So "Translators are able to define additional local variables for a message." is a cons in my book, not a pro.

And "A developer may pass the same basket of arguments to multiple messages, not all of which use each of them." does not look like something related to shadowing.

There is a reason many linters warn about shadowing.

"If shadowing were not allowed, then a combination of the above could result in a runtime error which would be difficult to discover ahead of time."

Not necessarily.
One can say that translators can't define variables (and that is a good thing).
It is an error, but the decision between runtime error and simply ignore the conflicting local variable is implementation dependent.
We should not use that as an argument for what the standard should do.

But TLDR:

  • I don't see valid use-cases for it. In fact the proposal we had used different sigils for local variables (macros at the time) and arguments.
  • I don't recall an explicit discussion on the subject.
  • It might also make sense to start with a stricter design and disallow shadowing at first.

Filed #310


It is an error for a local variable definition to
Copy link
Member

Choose a reason for hiding this comment

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

The language is a bit awkward; I suggest:

Any reference to a local variable before it is defined is an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@macchiati Good point; replaced with your suggestion.

refer to a local variable that's defined after it in the message.