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 inference weirdness #9184

Open
cromega opened this issue Apr 27, 2020 · 9 comments
Open

type inference weirdness #9184

cromega opened this issue Apr 27, 2020 · 9 comments

Comments

@cromega
Copy link

cromega commented Apr 27, 2020

Sorry for the very generic issue title, I've bumped into this rather confusing situation, and I have no idea what causes it so I couldn't really come up with a better one. Let the snippets do the talking:

abstract class Animal; end

class Dog < Animal; end

class Cat < Animal; end

class House
  @animals : Array(Animal)

  def initialize(*animals : Animal)
    @animals = animals.to_a
  end
end

p House.new(Cat.new, Cat.new)
$ crystal run animals.cr
Showing last frame. Use --error-trace for full trace.

In animals.cr:11:5

 11 | @animals = animals.to_a
      ^-------
Error: instance variable '@animals' of House must be Array(Animal), not Array(Cat)

If I change one of the cats into a Dog, it compiles.

Crystal 0.34.0 [4401e90f0] (2020-04-06)

LLVM: 8.0.0
Default target: x86_64-unknown-linux-gnu
(Running in WSL)
@cromega
Copy link
Author

cromega commented Apr 27, 2020

I read the entry about variance and covariance in the handbook. It says the type of the array needs to be set explicitly. However, in my case the type of the input splat is defined as Animal.

@jhass
Copy link
Member

jhass commented Apr 27, 2020

So what's happening is that you're effectively passing an Tuple(Cat, Cat). This passes the argument restriction of Tuple(*Animal), but performs no casting on it. Then Tuple#to_a does Array(Union(*T)).build, so Array(Cat).build effectively. This however is not assignable to Array(Animal). Yes the language is a bit inconsistent there, see #3803. Adding a Dog to the mix, constructs a Union(Cat, Dog) which the compiler simplifies to the parent type, Animal+, hence the observed effect of it working then.

Anyways, for now you can do @animals = Array(Animal).new(animals.size) {|i| animals[i] } to workaround this.

@cromega
Copy link
Author

cromega commented Apr 27, 2020

@jhass Thanks for the the explanation, it makes sense. At least in a "now I understand what's happening" way. Shall I keep the issue open?

@jhass
Copy link
Member

jhass commented Apr 27, 2020

I'm not sure, let's see what others think. It is indeed not very actionable and mostly covered by #3803 and all the related issues we already have open I think.

@straight-shoota
Copy link
Member

I don't think this can be changed in a fundamental way. Maybe when we get to revisit generics and improve covariance.
But we could make the workaround easier by adding an optional argument to #to_a, so you could call animals.to_a(Animal) instead of having to re-implement the method.

@cromega
Copy link
Author

cromega commented Apr 27, 2020

animals.to_a(Animal) would be great, it communicates the intent a lot better than the workaround.

Additionally, if the inconsistency can not be resolved easily, perhaps a warning could be printed if someone attempts something similar? That way at least I would see the same thing on the console regardless of what parameters I pass into the splat (instead of compilation error in one case and works fine in the other)

@RX14
Copy link
Contributor

RX14 commented Apr 27, 2020

Tuple#map returns a tuple, and Tuple#to_a doesn't accept a block for transforming it. I wonder if there's a space for #to_a taking a block, or whether animals.map(&.as(Animal)).to_a is fast enough (on the stack) that there doesn't need to be a new method.

@straight-shoota
Copy link
Member

I suppose #to_a(type) could actually be generally useful for any Enumerable, not just Tuple.

module Enumerable
  def to_a(type : U.class = T) forall U
    ary = [] of U
    each { |e| ary << e }
    ary
  end
end
 
class Foo
end
 
class Bar < Foo
end
 
class Baz < Foo
end
 
typeof([Bar.new, Bar.new].to_a(Foo)) # => Array(Foo)
typeof([Bar.new, Bar.new].to_a)      # => Array(Bar)

@HertzDevil
Copy link
Contributor

HertzDevil commented Jun 12, 2021

On master there is a different way to convert any Enumerable to an Array of appropriate type:

abstract class Animal; end

class Dog < Animal; end

class Cat < Animal; end

class House
  @animals : Array(Animal)

  def initialize(*animals : Animal)
    @animals = [*animals] of Animal
  end
end

House.new(Cat.new, Cat.new) # => #<House:... @animals=[#<Cat:...>, #<Cat:...>]>

Similarly Set(Animal){*animals} creates a Set. They all roughly expand to the same code as the to_a(type) above.

However, if the argument is additionally an Indexable (like in this case), the following is slightly faster:

Array(Animal).new(animals.size) do |i|
  animals.unsafe_fetch(i)
end

For some reason Crystal itself doesn't seem to use that outside Deque.new(array : Array(T)).

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

No branches or pull requests

5 participants