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

Fix bug when Hash only has one nil value #5170

Closed
wants to merge 1 commit into from

Conversation

werner
Copy link
Contributor

@werner werner commented Oct 23, 2017

When a Hash only has one value and this is nil, compact throws a compile error

@oprypin
Copy link
Member

oprypin commented Oct 23, 2017

This completely breaks the behavior of a standard library method to work around a corner case bug in the compiler.
There is no actual "fix" here, and it's not about "only one nil value", it's about only one nil type.

p ({"a" => nil} of String => Int32?).compact -- works
p ({} of String => Nil).compact -- does not work

Missing hash key: "value"
repos/crystal/src/hash.cr:0:9 in 'fetch'
repos/crystal/src/hash.cr:63:5 in '[]'
repos/crystal/src/compiler/crystal/codegen/codegen.cr:1406:17 in 'visit'
repos/crystal/src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
repos/crystal/src/compiler/crystal/codegen/codegen.cr:2027:7 in 'accept'
repos/crystal/src/compiler/crystal/codegen/codegen.cr:578:9 in 'visit'
repos/crystal/src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
...

@werner werner closed this Oct 23, 2017
@werner
Copy link
Contributor Author

werner commented Oct 23, 2017

@oprypin I just tested it does work, you're wrong

@Fryguy
Copy link
Contributor

Fryguy commented Oct 24, 2017

I can confirm what @oprypin sees on both 0.23.1 and HEAD.

While I agree that it's a bug for a specific type of Hash, the signature of compact shouldn't change for the rest of Hashes.

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

Successfully merging this pull request may close these issues.

None yet

3 participants