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

Type-matching with case doesn't downcast with >= 3 types #8864

Closed
jgaskins opened this issue Feb 28, 2020 · 8 comments · Fixed by #10147
Closed

Type-matching with case doesn't downcast with >= 3 types #8864

jgaskins opened this issue Feb 28, 2020 · 8 comments · Fixed by #10147

Comments

@jgaskins
Copy link
Contributor

Given the following code:

def foo : String | Int32 | Int64 | Nil
  case rand
  when 0...1/4
    42
  when 1/4...1/2
    42_i64
  when 1/2..3/4
    nil
  else
    "foo"
  end
end

case value = foo
when String, Int32, Int64
  typeof(value) # => (Int32 | Int64 | String | Nil)
else
  typeof(value) # => (Int32 | String | Nil)
end

In the first branch of the top-level case statement (checking the return value of foo), the compile-time type of the return value is all 4 return types despite being cast down to 3 types. The else clause also seems to be wrong. In that branch, the type can only be Nil, but the compiler thinks it can also be Int32 or String.

It works as expected if, instead of 3 or more types in the when clause, I use 1 or 2, and whether Nil is one of those types doesn't seem to have an effect. For example:

case value = foo
when String, Int32
  typeof(value) # => (Int32 | String)
else
  typeof(value) # => (Int64 | Nil)
end
@straight-shoota
Copy link
Member

Also happens when the condition is a union type:

value = 1.as(String | Int32 | Int64 | Nil)
case value
when Int32 | String
  typeof(value) # => (Int32 | Int64 | String | Nil)
end

I suppose conditions consisting of one or two types are transformed into .is_a? calls while unions and conditions with more than two types are passed through === which can't reflect the proper type restriction.

@asterite
Copy link
Member

asterite commented Mar 2, 2020

when Int32 | String is not the same as when Int32, String. The former is invoking | on Int32, then calling === on the resulting union type, which doesn't restrict types.

It's confusing so maybe we should make that work. But commas is the way to use it.

@asterite
Copy link
Member

asterite commented Mar 2, 2020

Also, for the original snippet, that case is rewritten to:

value = 1.as(String | Int32 | Int64 | Nil)
if value.is_a?(Int32) || value.is_a?(Int64) || value.is_a?(String)
  # ...
end

is_a? combined with || will restrict two types, but not more because it's a half baked feature. I wanted to implement it is_a? with || but it has bugs.

I think we should remove that because in the general case there's no way to make it work. So you can't do when X, Y, you have to write separate cases and use a common function, or just duplicate code, if you want duplicate logic. Sorry...

@straight-shoota
Copy link
Member

What about when X | Y?

@asterite
Copy link
Member

asterite commented Mar 2, 2020

I don't know.

@Sija
Copy link
Contributor

Sija commented Mar 3, 2020

I think we should remove that because in the general case there's no way to make it work.

@asterite Would you care to explain why not?

@HertzDevil
Copy link
Contributor

What about when X | Y?

That is not equivalent to when X, Y because a disjunction of is_a? predicates can distinguish types within an otherwise virtual hierarchy:

class Foo; end

class BarA < Foo; end
class BarB < Foo; end
class BarC < Foo; end

x = BarA.new || BarB.new || BarC.new
x.is_a?(BarB) || x.is_a?(BarC) # false
x.is_a?(BarB | BarC)           # true
x.is_a?(Union(BarB, BarC))     # true
x.is_a?(Foo)                   # true

On the other hand:

x = BarA.new
x.is_a?(BarB) || x.is_a?(BarC) # false
x.is_a?(BarB | BarC)           # false
x.is_a?(Union(BarB, BarC))     # true
x.is_a?(Foo)                   # true

@asterite
Copy link
Member

I think some results are unexpected and I consider them bugs.

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

Successfully merging a pull request may close this issue.

5 participants