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: let Proc(T) pass a restriction of Proc(Nil) #8969

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

asterite
Copy link
Member

Fixes #8964

@asterite asterite changed the title Compilre: let Proc(T) pass a restriction of Proc(Nil) Compiler: let Proc(T) pass a restriction of Proc(Nil) Mar 28, 2020
# If checking the return type, any type matches Nil
if i == other.type_vars.size - 1 && nil_type?(other_type_var, context)
# Also, all other types matched, so the matching type is this proc type
# except that it has a Nil return type
Copy link
Member

Choose a reason for hiding this comment

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

I thought the proc were honoring subtyping of arrow types.

It is safe for the following example to compile.

class Foo
end
 
class Bar < Foo
end
 
def foo(x : Proc(Bar, Nil))
  x
end
 
a = 1
 
proc = foo(->(b : Foo) { a = 2 })
r = proc.call(Bar.new)

It is subject for a later change, but is the subtying of arrow type in procs that we are missing it seems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we need to make subtyping work, but it won't work all of the time: only for class references. For structs it's not possible because the representation is different. Also for unions, I think. So for now I didn't implement it until we think it well. But matching against Nil is more common and easier to check.

@asterite
Copy link
Member Author

I won't merge any more PRs because I don't know whether they should be in 0.34.0 or later.

@bcardiff
Copy link
Member

I think this is safe to fit into 0.34 since it's handling more cases without loosing or changing others. On my end, feel free to merge it if you want.

@asterite asterite added this to the 0.34.0 milestone Mar 29, 2020
@asterite asterite merged commit c28a11e into master Mar 29, 2020
@asterite asterite deleted the bug/pass-proc-t-to-proc-nil branch March 29, 2020 20:34
@paulcsmith
Copy link
Contributor

Thanks @asterite!

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

Successfully merging this pull request may close these issues.

Proc(Nil) in a method type restriction does not work like it does elsewhere
4 participants