Skip to content

Enforce hash key types at compile time #8893

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.

Sign up for GitHub

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

Draft
jgaskins wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

Enforce hash key types at compile time #8893

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2d5b06c
Enforce hash key types at compile time
jgaskins Mar 10, 2020
af171e8
Ensure self type doesn't get lost
jgaskins Mar 10, 2020
2a37670
Fix hash specs
jgaskins Mar 10, 2020
7acd11a
Allow unions the other direction
jgaskins Mar 11, 2020
6184650
Add unions to Hash#== specs
jgaskins Mar 11, 2020
8639817
Fix YAML specs
jgaskins Mar 11, 2020
599b1f9
Fix compiler spec
jgaskins Mar 11, 2020
1ab3ad4
Add Set substraction specs back in
jgaskins Mar 11, 2020
ede59aa
Remove need for explicitly specifying set type
jgaskins Mar 11, 2020
51f66fb
Remove one more type specifier in Set spec
jgaskins Mar 11, 2020
a02b625
Remove unnecessary type check and cast
jgaskins Apr 8, 2020
f365d32
Clarify error message on NamedTuple.from
jgaskins Apr 8, 2020
8b6c2a4
Remove redundant nil check
jgaskins Apr 9, 2020
f2f15da
Merge branch 'master' into enforce-hash-key-types
jgaskins Apr 9, 2020
66d581b
Merge branch 'master' into enforce-hash-key-types
straight-shoota May 5, 2022
4f7680e
Adapt `Hash#subset_of?`
straight-shoota May 5, 2022
4bf7485
Refactor `NamedTuple.from`
straight-shoota May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion spec/compiler/crystal/tools/init_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ private def run_init_project(skeleton_type, name, author, email, github_name, di
).run
end

private def yaml(value)
YAML::Any.new(value)
Comment thread
straight-shoota marked this conversation as resolved.
end

module Crystal
describe Init::InitProject do
it "has proper contents" do
Expand Down Expand Up @@ -146,7 +150,11 @@ module Crystal

with_file "example_app/shard.yml" do |shard_yml|
parsed = YAML.parse(shard_yml)
parsed["targets"].should eq({"example_app" => {"main" => "src/example_app.cr"}})
parsed["targets"].should eq({
yaml("example_app") => {
yaml("main") => "src/example_app.cr",
},
Comment on lines +179 to +181

@asterite asterite Apr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not very intuitive. What prevents us from not modifying equality at all in this PR?

@asterite asterite Apr 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, I'd like this change to be gone but still have it compiling and working.

})
end

with_file "example/.travis.yml" do |travis|
Expand Down
4 changes: 4 additions & 0 deletions spec/std/hash_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ describe "Hash" do
a.should_not eq(c)
c.should_not eq(a)
d.should_not eq(a)

# Checking unions
Hash(String | Int32, Nil){32 => nil}.should eq Hash(Int32, Nil){32 => nil}
Hash(String, Nil){"hi" => nil}.should eq Hash(String | Int32, Nil){"hi" => nil}
end

it do
Expand Down
6 changes: 3 additions & 3 deletions spec/std/set_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ describe "Set" do
end

it "does -" do
set1 = Set{1, 2, 3, 4, 5}
set1 = Set(Int32 | Char){1, 2, 3, 4, 5}

@bcardiff bcardiff Mar 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bugs me how strict you need to be in this examples. I imagine that set1 - set2 no longer compiles when set1 : Set(Int32) and set2 : Enumerable(Int32 | Char). To me it falls into the same behaviour we want on equality: although we can infer it will always be false due to typing missmatch, we want to make it a valid that will return false instead of not compile.

It could happen that this set for example is generated from templates, macros and the explicit typing will be hard to inject. Hence, the type will be the infered from the elements, leading to Set(Int32).

@jgaskins jgaskins Mar 11, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is a a pretty irritating tradeoff on paper, but let’s think about the overall impact of this consequence. How often is it that you have a homogeneous set that you subtract a heterogeneous set from?

Honestly, I can’t think of a single time I’ve ever used a heterogeneous set in application code, let alone in this particular scenario where I’m subtracting one from a homogenous set. To be clear, you don’t need to specify types for all sets. Just ones where the type inference doesn’t get you what you need.

I do think that if you’re going to compare values, it’s probably a good idea to know that the types you’re comparing against can be different and the type specifier helps ensure that not only do you recognize that now, but you’ll recognize that when you change the code later. My perspective is that it’s not the same as adding two sets of different types where the result is a union of all the types in both sets. If there’s a bug there it’ll likely be caught elsewhere in the app because of the new type of the resulting set. For example:

numbers = Set{1, 2, 3}
another_set = Set{“1”, “2”}

result = numbers - another_set # => Set(Int32){1, 2, 3}
result = numbers + another_set # => Set(Int32 | String){1, 2, 3, “1”, “2”}

I don’t think the - version makes any sense at all and if I’m doing that, it’s probably a bug. When adding them, it changes the type and is more likely to catch that bug. With this PR, the subtraction would be a bug.

IMO, the type checker is there to help you. It’s why a lot of people still use static languages. Something that becomes a compile-time error that could be a runtime error is often a good thing. Being able to tell the compiler you know what you’re doing by simply adding a type annotation is also good.

All that said, I found a way to let me remove the explicit type specifier. I don’t think I agree with it, but it keeps the same behavior in this scenario.

@asterite asterite Mar 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess we should disallow - between sets of different types? That is, you can only do A - B if A's element types are a superset of B.

That would need to apply to all collection types.

The problem is always equality and inclusion, specially regarding ==. Because 1 == 1.0, and because == can be generally defined between any two types, when you have Set{1, 2, 3} then subtracting Set{1.0} could be valid...

Interestingly, in Ruby:

Set[1, 2, 3] - Set[1.0, 2.0, 3.0] # => #<Set: {1, 2, 3}>

Also:

irb(main):004:0> h = {1 => 2}
=> {1=>2}
irb(main):005:0> h[1.0]
=> nil

That's because Hash uses eql? and not == for comparing key values.

But then also in Ruby:

{a: 1} == {a: 1.0} # => true

but that's because it uses == for comparing elements, and 1 == 1.0 # => true.

So maybe we can disallow h[1.0] for Hash(Int32, V). However, if you have Hash(Int32 | Float64) and you have 1 as a key and you ask for 1.0 it will work and return the value even though with Hash(Int32, V) it wouldn't work... because we don't make the distinction of == and eql? like Ruby.

This is a hard problem... :-)

We'll need to decide if we want to introduce eql? to types, which might make sense in the context of union types and the way Ruby works.

Maybe we can also disallow == between any two types. For example comparing Int32 vs String wouldn't compile, which might make sense. But we can allow comparison between any two reference types, that being false by default.

Sorry for the wall of text... my point is that this PR can't be thought in isolation will all of these other things. And changing that is a huge breaking change which I'm not sure we can/should do right before 1.0.

@jgaskins jgaskins Mar 11, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess we should disallow - between sets of different types? That is, you can only do A - B if A's element types are a superset of B.

In an ideal world, I think that makes sense.

That would need to apply to all collection types.

Yeah, that's the catch. 🙂 It's why, even though I disagree with @bcardiff I still made it work without the explicit type annotation.

Your focus on consistency is important and I'm glad you called it out. Consistency can often be more important than anyone's perspective of what's "correct" because it allows you to guess at how something works. That's a super power for a programming language. Having to consult documentation all the time gets old quick.

I definitely want to have the conversation about whether Array(Int32)#-(Array(String)) should compile, but that particular change is too far outside the scope of this PR and I apologize for hammering on it. All I wanted was for the compiler to tell me when I changed my hash key type that I'd missed some of my key checks. 😂

if you have Hash(Int32 | Float64) and you have 1 as a key and you ask for 1.0 it will work and return the value even though with Hash(Int32, V) it wouldn't work... because we don't make the distinction of == and eql? like Ruby.

I've been using Ruby for 16 years and, honestly, I never understood why Ruby used eql? for that. But this definitely sounds like it might be why.

@paulcsmith paulcsmith Mar 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm loving the discussion here. I also use Crystal in large part due to the type-safety and would prefer the A must be a superset of B. I've been bitten by things like this when developing the internals of Lucky where I'm working with data from outside the system (cookies, params, sql).

I personally would love a more strict equality with an "escape hatch" of some kind. Maybe that's with eql? or a different method. The key is that you have that escape hatch. So strict by default to catch bugs, with an escape hatch for if you want to get into the kinky stuff.

You could even do something similar to what we do in Lucky where there is a fallback method that generates a nice little error telling you why it happened and what you can do isntead:

Can't compare/set ekys/etc Objects of different types.

Make sure the objects are the type you expect, or use `some_other_method` to allow setting/getting/comparing objects of different types.

This way you get the safety, but you have the escape hatch along with a nice error. I think this will really make the lagnuage super powerful and easy for beginners.

@HertzDevil HertzDevil Aug 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require something like #6997 (comment):

struct Set(T)
  def -(other : Enumerable(U)) forall U <= T
    # ...
  end

  @[Deprecated("Ensure *other*'s element type is `T` or a subtype of it")]
  def -(other : Enumerable)
    dup
  end
end

The story is more complex if we want to allow unions of Set instances as arguments (mainly due to #8973).

set2 = [2, 4, 'a']
set3 = set1 - set2
set3.should eq(Set{1, 3, 5})
Expand Down Expand Up @@ -242,7 +242,7 @@ describe "Set" do
end

it "does subtract" do
set1 = Set{1, 2, 3, 4, 5}
set1 = Set(Int32 | Char){1, 2, 3, 4, 5}
set2 = Set{2, 4, 'a'}
set1.subtract set2
set1.should eq(Set{1, 3, 5})
Expand All @@ -263,7 +263,7 @@ describe "Set" do
end

it "does subtract" do
set1 = Set{1, 2, 3, 4, 5}
set1 = Set(Int32 | Char){1, 2, 3, 4, 5}
set2 = [2, 4, 'a']
set1.subtract set2
set1.should eq(Set{1, 3, 5})
Expand Down
4 changes: 2 additions & 2 deletions spec/std/yaml/any_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ describe YAML::Any do
end

it "gets hash" do
YAML.parse("foo: bar").as_h.should eq({"foo" => "bar"})
YAML.parse("foo: bar").as_h?.should eq({"foo" => "bar"})
YAML.parse("foo: bar").as_h.should eq({YAML::Any.new("foo") => "bar"})
YAML.parse("foo: bar").as_h?.should eq({YAML::Any.new("foo") => "bar"})
YAML.parse("foo: bar")["foo"].as_h?.should be_nil
end

Expand Down
12 changes: 8 additions & 4 deletions spec/std/yaml/schema/core_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ private def it_parses_scalar_from_pull(string, file = __FILE__, line = __LINE__,
end
end

private def yaml(value) : YAML::Any
YAML::Any.new(value)
end

describe YAML::Schema::Core do
# nil
it_parses_scalar "~", nil
Expand Down Expand Up @@ -163,20 +167,20 @@ describe YAML::Schema::Core do
it_parses_scalar "N", "N"

# !!map
it_parses "!!map {1: 2}", {1 => 2}
it_parses "!!map {1: 2}", {yaml(1i64) => 2}
it_raises_on_parse "!!map 1", "Expected MAPPING_START"

# !!omap
it_parses "!!omap {1: 2}", {1 => 2}
it_parses "!!omap {1: 2}", {yaml(1i64) => 2}
it_raises_on_parse "!!omap 1", "Expected MAPPING_START"

# !!pairs
it_parses "!!pairs [{1: 2}, {3: 4}]", [{1 => 2}, {3 => 4}]
it_parses "!!pairs [{1: 2}, {3: 4}]", [{yaml(1i64) => 2}, {yaml(3i64) => 4}]
it_raises_on_parse "!!pairs 1", "Expected SEQUENCE_START"
it_raises_on_parse "!!pairs [{1: 2, 3: 4}]", "Expected MAPPING_END"

# !!set
it_parses "!!set { 1, 2, 3 }", Set{1, 2, 3}
it_parses "!!set { 1, 2, 3 }", Set{yaml(1i64), yaml(2i64), yaml(3i64)}
it_raises_on_parse "!!set 1", "Expected MAPPING_START"

# !!seq
Expand Down
19 changes: 15 additions & 4 deletions spec/std/yaml/schema/fail_safe_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,31 @@ private def it_raises_on_parse_all(string, message, file = __FILE__, line = __LI
end
end

private def yaml(value)
YAML::Any.new(value)
end

describe YAML::Schema::FailSafe do
# parse
it_parses "123", "123"
it_parses %(
context:
replace_me: "Yes please!"
), {"context" => {"replace_me" => "Yes please!"}}
), {yaml("context") => {yaml("replace_me") => "Yes please!"}}
it_parses %(
first:
document:

second:
document:
), {"first" => {"document" => ""}, "second" => {"document" => ""}}
), {
yaml("first") => {
yaml("document") => "",
},
yaml("second") => {
yaml("document") => "",
},
}
it_raises_on_parse %(
this: "gives"
an: "error"
Expand All @@ -54,14 +65,14 @@ describe YAML::Schema::FailSafe do
it_parses_all %(
context:
replace_me: "Yes please!"
), [{"context" => {"replace_me" => "Yes please!"}}]
), [{yaml("context") => {yaml("replace_me") => "Yes please!"}}]
it_parses_all %(
foo:
bar: 123

bar:
foo: 321
), [{"foo" => {"bar" => "123"}, "bar" => {"foo" => "321"}}]
), [{yaml("foo") => {yaml("bar") => "123"}, yaml("bar") => {yaml("foo") => "321"}}]
it_raises_on_parse_all %(
this: "raises"
an: "yaml"
Expand Down
25 changes: 19 additions & 6 deletions spec/std/yaml/yaml_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
require "spec"
require "yaml"

private def yaml(value)
YAML::Any.new(value)
end

describe "YAML" do
describe "parser" do
it { YAML.parse("foo").should eq("foo") }
it { YAML.parse("- foo\n- bar").should eq(["foo", "bar"]) }
it { YAML.parse_all("---\nfoo\n---\nbar\n").should eq(["foo", "bar"]) }
it { YAML.parse("foo: bar").should eq({"foo" => "bar"}) }
it { YAML.parse("foo: bar").should eq({yaml("foo") => "bar"}) }
it { YAML.parse("--- []\n").should eq([] of YAML::Any) }
it { YAML.parse("---\n...").should eq nil }

Expand Down Expand Up @@ -56,7 +60,7 @@ describe "YAML" do
bar:
<<: *x
))
doc["bar"].should eq({"bar" => 1, "baz" => 2})
doc["bar"].should eq({yaml("bar") => 1, yaml("baz") => 2})
end

it "merges other mapping with array of alias" do
Expand All @@ -68,7 +72,7 @@ describe "YAML" do
bar:
<<: [*x, *y]
))
doc["bar"].should eq({"bar" => 1, "baz" => 2})
doc["bar"].should eq({yaml("bar") => 1, yaml("baz") => 2})
end

it "doesn't merge explicit string key <<" do
Expand All @@ -78,7 +82,16 @@ describe "YAML" do
bar:
!!str '<<': *foo
))
doc.should eq({"foo" => {"hello" => "world"}, "bar" => {"<<" => {"hello" => "world"}}})
doc.should eq({
yaml("foo") => {
yaml("hello") => "world",
},
yaml("bar") => {
yaml("<<") => {
yaml("hello") => "world",
},
},
})
end

it "doesn't merge empty mapping" do
Expand All @@ -87,7 +100,7 @@ describe "YAML" do
bar:
<<: *foo
))
doc["bar"].should eq({"<<" => nil})
doc["bar"].should eq({yaml("<<") => nil})
end

it "doesn't merge arrays" do
Expand All @@ -97,7 +110,7 @@ describe "YAML" do
bar:
<<: *foo
))
doc["bar"].should eq({"<<" => [1]})
doc["bar"].should eq({yaml("<<") => [1]})
end

it "has correct line/number info (#2585)" do
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/crystal/program.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,11 +576,11 @@ module Crystal
file_module?(filename).try &.lookup_matches(signature)
end

def file_module?(filename)
def file_module?(filename : String)
file_modules[filename]?
end

def file_module(filename)
def file_module(filename : String)
file_modules[filename] ||= FileModule.new(self, self, filename)
end

Expand Down
8 changes: 4 additions & 4 deletions src/compiler/crystal/semantic/type_declaration_processor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ struct Crystal::TypeDeclarationProcessor
ancestor = owner.ancestors.first?
ancestor = uninstantiate(ancestor)
if ancestor
ancestor_non_nilable = @non_nilable_instance_vars[ancestor]?
ancestor_non_nilable = @non_nilable_instance_vars[ancestor]? if ancestor.is_a?(Type)
Comment thread
jgaskins marked this conversation as resolved.
Outdated
end

# If the ancestor has non-nilable instance vars, check that all initialize either call
Expand Down Expand Up @@ -538,7 +538,7 @@ struct Crystal::TypeDeclarationProcessor

owner.ancestors.any? do |ancestor|
ancestor = uninstantiate(ancestor)
@instance_vars_outside[ancestor]?.try &.includes?(name)
@instance_vars_outside[ancestor]?.try &.includes?(name) if ancestor.is_a? Type
end
end

Expand All @@ -553,7 +553,7 @@ struct Crystal::TypeDeclarationProcessor
end

private def compute_non_nilable_outside_single(owner, non_nilable_outisde)
if vars = @instance_vars_outside[owner]?
if owner.is_a?(Type) && (vars = @instance_vars_outside[owner]?)
non_nilable_outisde ||= [] of String
vars.each do |name|
non_nilable_outisde << name unless non_nilable_outisde.includes?(name)
Expand All @@ -570,7 +570,7 @@ struct Crystal::TypeDeclarationProcessor

owner.ancestors.each do |ancestor|
ancestor = uninstantiate(ancestor)
infos = @initialize_infos[ancestor]?
infos = @initialize_infos[ancestor]? if ancestor.is_a?(Type)
return infos if infos && !infos.empty?
end

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/tools/playground/server.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ module Crystal::Playground

agent_ws = PathWebSocketHandler.new "/agent" do |ws, context|
match_data = context.request.path.not_nil!.match(/\/(\d+)\/(\d+)$/).not_nil!
session_key = match_data[1]?.try(&.to_i)
session_key = match_data[1]?.try(&.to_i) || 0

@asterite asterite Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

@jgaskins jgaskins Mar 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session_key can't be nil because of this line. Because it can't store nil and it's using [] instead of []? below, this line seems like it can't return nil even without this PR or it would raise a KeyError below.

tag = match_data[2]?.try(&.to_i)
@logger.info "#{context.request.path} WebSocket connected (session=#{session_key}, tag=#{tag})"

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/tools/typed_def_processor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module Crystal::TypedDefProcessor

private def process_result(result : Compiler::Result)
process_type result.program
if file_module = result.program.file_module? target_location.original_filename
if (filename = target_location.original_filename) && (file_module = result.program.file_module?(filename))
process_type file_module
end
end
Expand Down
Loading