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

Wrong overload called instead of BigFloat.initialize(BigRational) #4897

Open
akzhan opened this issue Aug 29, 2017 · 13 comments · May be fixed by #11840
Open

Wrong overload called instead of BigFloat.initialize(BigRational) #4897

akzhan opened this issue Aug 29, 2017 · 13 comments · May be fixed by #11840
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic

Comments

@akzhan
Copy link
Contributor

akzhan commented Aug 29, 2017

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: akzhan@7ea0a77

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

@konovod
Copy link
Contributor

konovod commented Aug 29, 2017

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
Copy link
Contributor Author

akzhan commented Aug 29, 2017

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
Copy link
Contributor

RX14 commented Aug 29, 2017

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
Copy link
Member

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
Copy link
Contributor Author

akzhan commented Aug 29, 2017

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

@asterite
Copy link
Member

@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
Copy link
Contributor

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.

RX14 pushed a commit that referenced this issue Sep 1, 2017
* Additions to Big arithmetics.
Follows #4560.

* Follow @RX14 review (single line blocks, extract mantissa bits constants).

* Int cast is implicit here.

* Format src/big/big_rational.cr

* BigRational is not float, and frexp should be not used on it.

* frexp should not be used on BigInt.

* rework BigFloat./

* rename bsi, bfsi, bsf, bfsf etc. in spec.

* rework def BigFloat.<=>(other : Int)

* Simplify a bit, thanks to @RX14

* just remove commen. looks like single Number constructor looks OK in every case.

* more verbose naming for used variables.

* just explanation of workaround, refs #4897

* For now overloads work (i don't know why).

* dedup

* Forward declarations of Big Arithmentics types.
@asterite
Copy link
Member

asterite commented Sep 7, 2017

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
Copy link
Contributor

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
Copy link
Member

asterite commented Sep 7, 2017

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

@asterite
Copy link
Member

asterite commented Sep 7, 2017

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
Copy link
Contributor

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).

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Apr 15, 2021
@HertzDevil
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants