Revert to the use of StringMacroName/CmdMacroName kinds #583
Conversation
Trying out `macro_name_str` / `macro_name_cmd` in JuliaLowering it turns out to be inconvenient to work with identifiers which change their meaning due to being nested inside another construct. This change revert to the previous use of StringMacroName / CmdMacroName identifier kinds. macro_name is left as-is because it faithfully represents the position of the `@`. This isn't a complete reversion to the previous JuliaSyntax behavior. Previously, SyntaxNode would contain the symbol `@x_str` for the string macro `x` in `x"hi"` despite having kind set to `SringMacroName`. However, appending the `_str` is best seen as a symbolic lowering (/name mangling) step which isn't reflected in the source code and shouldn't be the business of the parser or parser-related tools. Thus, in the code here we defer this name mangling to the `Expr` conversion step instead, and introduce `lower_identifier_name()` as a standard way to do this conversion.
test/expr.jl
Outdated
| @@ -554,7 +554,6 @@ | |||
|
|
|||
| # var"" | |||
| @test parsestmt("@var\"#\" a") == Expr(:macrocall, Symbol("@#"), LineNumberNode(1), :a) | |||
| @test parsestmt("@var\"\\\"\" a") == Expr(:macrocall, Symbol("@\""), LineNumberNode(1), :a) | |||
There was a problem hiding this comment.
Should be kept. This is JuliaLang/julia#58885
There was a problem hiding this comment.
Good catch, thanks. My mistake in doing the partial-revert I'll do a closer self-review of the test changes.
test/parser.jl
Outdated
| # · and · normalize to ⋅ | ||
| @test parse_to_sexpr_str(JuliaSyntax.parse_eq, "a \u00B7 b") == "(call-i a \u22C5 b)" | ||
| @test parse_to_sexpr_str(JuliaSyntax.parse_eq, "a \u0387 b") == "(call-i a \u22C5 b)" | ||
| # − ('\u2212') normalizes to - ('\u002d') | ||
| @test parse_to_sexpr_str(JuliaSyntax.parse_expr, "a \u2212 b") == "(call-i a - b)" |
There was a problem hiding this comment.
Yeah, no idea what happened here, yikes
|
Fine by me in general, though it precludes enabling |
|
I feel it's sufficient that
Do you have thoughts on this? |
|
I do like |
|
I tried the I think I'm happy with this now. |
Trying out
macro_name_str/macro_name_cmdin JuliaLowering it turns out to be inconvenient to work with identifiers which change their meaning due to being nested inside another construct.This change revert to the previous use of StringMacroName / CmdMacroName identifier kinds. macro_name is left as-is because it faithfully represents the position of the
@.This isn't a complete reversion to the previous JuliaSyntax behavior. Previously, SyntaxNode would contain the symbol
@x_strfor the string macroxinx"hi"despite having kind set toSringMacroName. However, appending the_stris best seen as a symbolic lowering (/name mangling) step which isn't reflected in the source code and shouldn't be the business of the parser or parser-related tools. Thus, in the code here we defer this name mangling to theExprconversion step instead and introducelower_identifier_name()as a way to do this conversion for cases whereis_identifierreturns true.Given the subtle change of semantics (in terms of the SyntaxNode
.valfield) we could also take this opportunity to useMacroNameStrandMacroNameCmdif those seem like better names?I considered keeping
macro_name_strandmacro_name_cmdas wrappers, but that's actually more difficult implementation-wise if we only want to wrap an identifier (Eg, when dealing withA.B.x"hi"- it's awkward to avoid wrapping the whole path withmacro_name_str. Additionally, less nesting is simpler for consumers of the AST structure.Fix #582