Add variable resolution #305
Conversation
| If a local variable and an externally defined one use the same name, | ||
| the local variable takes precedence. | ||
|
|
||
| It is an error for a local variable definition to |
There was a problem hiding this comment.
The language is a bit awkward; I suggest:
Any reference to a local variable before it is defined is an error.
There was a problem hiding this comment.
@macchiati Good point; replaced with your suggestion.
|
Perhaps we should discuss and try to resolve #248 at the plenary meeting today. It would make it easier to make progress on the formatting spec. |
59cbfd7 to
8630218
Compare
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Translators are able to define additional local variables for a message.
- 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.
There was a problem hiding this comment.
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
Builds on #304, which should be merged before this.
Closes #297
This PR does not attempt to provide exhaustive contents for variable resolution, but at least a start that should correspond to the consensus reached (so far) in issue #297. It also clarifies the precedence of local vs. external variables, on which we've had some discussion, but I'm not able to find a corresponding issue.
CC @macchiati, who I wasn't able to add directly as a reviewer.