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

Compiler fails on NamedTuple keys containing dashes #14784

Closed
fvirdia opened this issue Jul 5, 2024 · 8 comments · Fixed by #14785
Closed

Compiler fails on NamedTuple keys containing dashes #14784

fvirdia opened this issue Jul 5, 2024 · 8 comments · Fixed by #14785
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:stdlib:collection

Comments

@fvirdia
Copy link

fvirdia commented Jul 5, 2024

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
Copy link
Member

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
Copy link
Author

fvirdia commented Jul 5, 2024

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
Copy link
Author

fvirdia commented Jul 5, 2024

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 Blacksmoke16 added the kind:regression Something that used to correctly work but no longer works label Jul 5, 2024
@Blacksmoke16
Copy link
Member

Prob caused by #12011.

@Blacksmoke16
Copy link
Member

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
Copy link
Author

fvirdia commented Jul 5, 2024

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
Copy link
Member

Blacksmoke16 commented Jul 5, 2024

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
Copy link
Author

fvirdia commented Jul 5, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:stdlib:collection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants