-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
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 |
Yes, there is some kind of voodoo. Also sometimes adding to original big_float.cr |
|
If you don't 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 @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. |
@asterite Both "big_float.cr" and sample spec have |
@akzhan require "./big/lib_gmp"
require "./big/big_int"
require "./big/big_float"
require "./big/big_rational" You can see that |
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. |
* 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.
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 |
When forward declaration is needed for correct method resolution, shouldn't it be right to demand forward declaration? |
@funny-falcon That's exactly the change I'm proposing. |
This comes from the old days where you had: def foo(x : Some)
end If |
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 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 |
By the way, it is not truly possible to forward declare the If we want true forward declarations there should be an annotation for them. |
Steps to reproduce:
Add to
BigFloat
new constructor that receivedBigRational
and raises anyway:and create any spec to expect that should be raised.
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
The text was updated successfully, but these errors were encountered: