crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.35k stars 1.62k forks source link

Ambiguity between sigils and macro fresh variables #14782

Open HertzDevil opened 2 months ago

HertzDevil commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

but require (or recommend) [...]

You mean in docs, or as a warning?

straight-shoota commented 2 months ago

Incrementally reinforcing: documentation, formatter, warning, syntax error

oprypin commented 2 months ago

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 https://github.com/crystal-lang/crystal/issues/14782#issuecomment-2210651635 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 commented 2 months ago

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.