Wrong overload called instead of BigFloat.initialize(BigRational)

[akzhan]

Steps to reproduce:

Add to BigFloat new constructor that received BigRational and raises anyway:

struct BigFloat

  def initialize(rat : BigRational)
    raise "Rat"
    LibGMP.mpf_init(out @mpf)
    # LibGMP.mpf_set_q(self, num)
  end

and create any spec to expect that should be raised.

require "spec"
require "big"

describe "BigFloat" do
  it "initialize(BigRational)" do
    expect_raises do
      BigFloat.new( BigRational.new(1, 1) )
    end
  end
end

And this spec will failed.

Commit diff: https://github.com/akzhan/crystal/commit/7ea0a7777ff8d156ec236545c4dc03f48b6d1685

Code can be pulled from: https://github.com/akzhan/crystal/tree/wrong-overload-called-intead-of-initialize-bigrational

[konovod]

It is interesting that it doesn't fails in a single snippet:

require "spec"
require "big"

struct BigFloat
 def initialize(rat : BigRational)
    raise "Rat"
    LibGMP.mpf_init(out @mpf)
    # LibGMP.mpf_set_q(self, num)
  end
end

describe "BigFloat" do
  it "initialize(BigRational)" do
    expect_raises do
      BigFloat.new( BigRational.new(1, 1) )
    end
  end
end

https://carc.in/#/r/2m5w so triggering a bug depends of declaration order of overloads?

[akzhan]

Yes, there is some kind of voodoo.

Also sometimes adding to original big_float.cr require "big/big_rational"helps, but it's frustrating me.

[RX14]

BigFloat has a initialize(Number) overload which would accept initialize(BigRational) but is less-specific. It's a bug that the less-specific overload is taken.

[asterite]

If you don't require "big/big_rational" then when the compiler finds def initialize(rat : BigRational) it has no idea what BigRational is and so puts it at the end of the overload list.

This works this way so you don't have to use forward declarations. But of course it breaks in other subtle ways... so I think it's better to remove this anti-feature and always try to type parameters when methods are defined. Yet, some forward declarations will be needed, but it's more explicit.

This isn't trivial to implement because of things like def foo(x : Array(T)) forall T: what type is Array(T)? In the compiler there's a TypeParameter fictitious type, but the code surrounding it is messy and hard to understand.

@RX14 If you, or someone else, want something to try to tackle inside the compiler, this would be a good first step :-)

Like this, there are tons of other small issues in the compiler and the language that should, little by little, be improved and fixed.

[akzhan]

@asterite Both "big_float.cr" and sample spec have require "big", that requires all files in big folder.

[asterite]

@akzhan big.cr is:

require "./big/lib_gmp"
require "./big/big_int"
require "./big/big_float"
require "./big/big_rational"

You can see that big_float is required before big_rational

[funny-falcon]

it has no idea what BigRational is and so puts it at the end of the overload list. so I think it's better to remove this anti-feature and always try to type parameters when methods are defined.

As alternative, overload list could have a flag "has undeclared type". When method invocation resolved, and this flag is set, then overload list must be rebuilt.

[asterite]

So, I decided the compiler will try to solve method restrictions as soon as it finds them, and give an error if it can't find a type. That means in some cases you'll need forward declarations, but that shouldn't be that common.

The main issue is that restrictions are used to determine overloads and redefinitions, so we can't delay this resolution because methods might be queried via macros. I think this is the safest choice. In the future we could try to improve this, but I don't think a few forward declarations hurt, specially when it's just about writing:

class ForwardDeclaration; end

[funny-falcon]

When forward declaration is needed for correct method resolution, shouldn't it be right to demand forward declaration? It is pity that such kind of errors is silently ignored.

[asterite]

@funny-falcon That's exactly the change I'm proposing.

[asterite]

This comes from the old days where you had:

def foo(x : Some)
end

If Some didn't exist, it would become a free variable, taking the type of the given call argument. We later changed it to only happen when things like T or T1 were used. Eventually we removed all of that and we went with forall. But the logic in the compiler to lazily solve this is still there.

[HertzDevil]

Included (extended) modules also need forward declarations, apart from inherited types (this is precisely what happened in #10518). Consider:

module A; end
module B; end

class AImpl; include A; end
class BImpl; include B; end

class Foo
  def foo(x : A)
    puts "A"
  end

  def foo(x : B)
    puts "B"
  end
end

{% if Foo.methods.map(&.args.first.restriction.id) == %w(A B).map(&.id) %}
  module B; include A; end
{% else %}
  module A; include B; end
{% end %}

Foo.new.foo(AImpl.new) # => A
Foo.new.foo(BImpl.new) # => A

Since BImpl < B < A, the second call should print B, not A. On the other hand, if the orders of the two foo overloads are swapped, both calls would print B instead.

I personally think we should simply defer overload ordering to a separate compiler phase after all top-level declarations have been processed. Top-level macros like above would still see all the defs, but in the exact same order they were defined, i.e. it won't be possible to observe the overload ordering at the top level (macros inside defs would observe the ordering; contrast with TypeNode#instance_variables).

[HertzDevil]

By the way, it is not truly possible to forward declare the Big types alone, because they include Comparable which requires a definition for #<=>; so either none of the Big types are defined, or all of them are. On the other hand, a type's "forward declaration" counts as a definition, so if the Big types have no abstract defs then a user might inadvertently find them as empty types. For Big this is probably fine, but for something as large as the entire standard library, forward declarations should not imply definitions.

If we want true forward declarations there should be an annotation for them.