crystal-lang / crystal

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

Compiler fails on NamedTuple keys containing dashes #14784

Closed fvirdia closed 2 months ago

fvirdia commented 3 months ago

Bug Report

Trying to use a string literal as NamedTuple key fails if the literal contains a dash -.

Compiler info:

Crystal 1.12.2 [04998c0c7] (2024-05-31)

LLVM: 15.0.7
Default target: x86_64-unknown-linux-gnu

Example code

# quoting https://crystal-lang.org/reference/1.12/syntax_and_semantics/literals/named_tuple.html
# A named tuple key can also be a string literal:
# {"this is a key": 1}

# this works
x = NamedTuple("a_b": String).new("a_b": "")
puts x

# this fails
y = NamedTuple("a-b": String).new("a-b": "")
puts y

Compiler output:

$ crystal build bug.cr 
Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'element_type'

Code in macro 'macro_126783410818560'

 7 | "a-b": options[:"a-b"].as(typeof(element_type(a-b))),
                                      ^
Called macro defined in /usr/share/crystal/src/named_tuple.cr:715:11

 715 | private macro element_type(key)

Which expanded to:

 > 1 | x = uninitialized self
 > 2 | x[:"a - b"]
        ^
Error: missing key 'a - b' for named tuple NamedTuple("a-b": String)
Blacksmoke16 commented 3 months ago

I think this is scoped to NamedTuple.new itself. I don't think I've ever seen someone using the constructor over the literal, which works just fine: {"a-b": ""}. We should still fix it of course, but I'd be curious to know what the use case for even having NamedTuple.new is over just using the literal...

fvirdia commented 3 months ago

Playing a bit with the "try it online" thingy, it seems this was working as of 1.4.1, and stopped working with 1.5.0.

fvirdia commented 3 months ago

I'd be curious to know what the use case for even having NamedTuple.new is over just using the literal...

inexperience with the language

Blacksmoke16 commented 3 months ago

Prob caused by https://github.com/crystal-lang/crystal/pull/12011.

Blacksmoke16 commented 3 months ago

I'd be curious to know what the use case for even having NamedTuple.new is over just using the literal...

inexperience with the language

No worries, more so meant from a language perspective. I submitted a PR to fix this, but for now can just use the literal syntax if you're needing one with a hyphen.

fvirdia commented 3 months ago

That was quick!

I went back and looked at why I used the constructor, I think I was unsure whether I would be getting Hash(String, String). I wanted to make sure I knew the type I was instantiating. Again, at the time of stumbling across the bug (actually a week ago, it was on my todo list) I had been using crystal for about a day.

Is the literal guaranteed to always result in a NamedTuple?

Blacksmoke16 commented 3 months ago

Is the literal guaranteed to always result in a NamedTuple?

Yes, an "object" using {} with : between key and value is a NamedTuple. E.g. {foo: "bar"}. However if you use => as the separator, you get a hash. E.g. {"foo" => "bar"}.

fvirdia commented 3 months ago

Thanks!