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

Ambiguity between sigils and macro fresh variables #14782

Open
HertzDevil opened this issue Jul 4, 2024 · 7 comments
Open

Ambiguity between sigils and macro fresh variables #14782

HertzDevil opened this issue Jul 4, 2024 · 7 comments

Comments

@HertzDevil
Copy link
Contributor

Crystal supports 6 sigils which modify the meaning of string literals. Curly braces may be used instead of the usual double quotation marks for those strings:

%i{1}
%q{2}
%Q{3}
%r{4}
%x{5}
%w{6}

The same syntax is used for fresh variables inside macros:

macro foo
  {% for i in 1..6 %}
    %b{i} = {{ i }}
  {% end %}
  {% for i in 1..6 %}
    p %b{i}
  {% end %}
end

foo

It is a syntax error here to use a fresh variable name that coincides with one of the sigils, because the sigil has precedence and assignment to a string literal is never allowed. This is problematic since adding any new sigil to the language, e.g. %b for #2886, immediately introduces a breaking change by making those macros no longer valid.

I believe the language should be more future-proof here, unless we can say for sure that new sigils must not be introduced.

@straight-shoota
Copy link
Member

I understand the fresh variable index syntax can be considered somewhat like syntax sugar.
%var{a} should be more or less equivalent to %var_{{a}} (doesn't compile, probably collides with the short syntax as well) or {{ "%var_#{a}".id }} (different macro scope nesting, though).

Maybe we could remove the syntax sugar entirely and use %var_{{a}} instead? Maybe make the underscore required after the static part of the variable name, that would entirely exclude confusions with literal sigils. This makes the fresh variables a bit less concise, but I think that's not a problem. They are a rarely-used low-level feature.

The fresh variable syntax also allows multiple index values (%var{a, b, c}). I'm not sure if that's used anywhere. It should work the same way with normal expansion, though (%var_{{a}}_{{b}}_{{c}}).

@HertzDevil
Copy link
Contributor Author

Simple interpolation can be problematic if the index is not something that is valid as part of an identifier (e.g. negative numbers, defs).

@straight-shoota
Copy link
Member

Another, simpler idea: We keep the fresh variable index syntax as is, but require (or recommend) to put a special character between the name and the opening curly brace. This character should never be used in the sigial of a percent literal. For example an underscore. So %b{a} becomes %b_{a}.
The nice thing is both syntax variants are valid and we can transition to the latter one, eventually removing the former.

@beta-ziliani
Copy link
Member

but require (or recommend) [...]

You mean in docs, or as a warning?

@straight-shoota
Copy link
Member

Incrementally reinforcing: documentation, formatter, warning, syntax error

@oprypin
Copy link
Member

oprypin commented Jul 5, 2024

Why not simply add a warning to never use single-letter "fresh variable" identifiers? It's never required to have a particular name for them, and it affects literally nothing.

I'm saying this especially in light of #14782 (comment)
It's so confusing to have to write %b_{x} when you could simply write %boo{x} with the exact same outcome and without adding any new syntax

@straight-shoota
Copy link
Member

Yeah, might be overthinking it.
Recommending longer names might be entirely sufficient. We could consider having single-letter names produce a warning (and eventually an error).

That's all assuming, we'll only ever use single letters as indicator for percent literals. We don't have a use case for multi-letters yet, and maybe there is none. So this might be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants