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

Defer overload ordering until all types are fully defined #11840

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Feb 18, 2022

Closes #11692. The new semantics are only available if -Dpreview_overload_order is defined. This conflicts with the flag used for #10711, so suggestions are welcome. I was thinking about an opt-out -Dno_defer_overload_order flag, but I am not too confortable about that. This phase is done as part of the "type declarations" semantic stage; building an empty file and the standard library specs took about +0.08s and +0.2s respectively.

Fixes #4897. Suppose the forward declarations in src/big.cr are removed. Before:

require "big"

BigFloat.new(1, 2, 3) # Error: wrong number of arguments for 'BigFloat.new' (given 3, expected 0..2)

# Overloads are:
#  - BigFloat.new(num : Float, precision : Int)
#  - BigFloat.new(str : String)
#  - BigFloat.new(num : BigInt)
#  - BigFloat.new(num : BigFloat)
#  - BigFloat.new(num : Int8 | Int16 | Int32)
#  - BigFloat.new(num : UInt8 | UInt16 | UInt32)
#  - BigFloat.new(num : Int64)
#  - BigFloat.new(num : UInt64)
#  - BigFloat.new(num : Number)
#  - BigFloat.new(mpf : LibGMP::MPF)
#  - BigFloat.new(num : BigRational)
#  - BigFloat.new()
#  - BigFloat.new(&)

After: (the BigRational overload is at the correct location)

# Overloads are:
#  - BigFloat.new(num : Float, precision : Int)
#  - BigFloat.new(str : String)
#  - BigFloat.new(num : BigInt)
#  - BigFloat.new(num : BigRational)
#  - BigFloat.new(num : BigFloat)
#  - BigFloat.new(num : Int8 | Int16 | Int32)
#  - BigFloat.new(num : UInt8 | UInt16 | UInt32)
#  - BigFloat.new(num : Int64)
#  - BigFloat.new(num : UInt64)
#  - BigFloat.new(num : Number)
#  - BigFloat.new(mpf : LibGMP::MPF)
#  - BigFloat.new()
#  - BigFloat.new(&)

Fixes #7579:

class Base; end

def foo(a : Derive); 1; end
def foo(a : Base); true; end

class Derive < Base; end

# `true` before this patch
foo(Derive.new) # => 1

Fixes part of #10518:

puts (0..0).as(Enumerable(Int32)) # => 0..0

Fixes #11678:

class Foo1; end
class Foo2 < Foo1; end
class Foo3 < Foo1; end

class Foo1
  alias Bar = Foo2::Bar | Foo3::Bar
end

class Foo2 < Foo1
  class Bar; end
  def foo(other : Foo1::Bar); end
  def foo(other : String); end
end

class Foo3 < Foo1
  class Bar; end
end

# infinite recursive definition before this patch
Foo1::Bar # => (Foo2::Bar | Foo3::Bar)

For this example, A and B will be printed regardless of the declaration order of Foo#foo's overloads.

Does not affect #8771.

Again, all these examples work only if -Dpreview_overload_order is specified during compilation.

@HertzDevil HertzDevil added breaking-change topic:compiler:semantic kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:feature labels Feb 18, 2022
@HertzDevil HertzDevil marked this pull request as draft February 18, 2022 12:07
@HertzDevil HertzDevil marked this pull request as ready for review February 18, 2022 13:30
@mattrberry
Copy link
Contributor

I'm on mobile and don't have much time to look closer. Does this have any implications on #10986 ?

@straight-shoota
Copy link
Member

straight-shoota commented Feb 22, 2022

The new semantics are only available if -Dpreview_overload_order is defined. This conflicts with the flag used for #10711, so suggestions are welcome. I was thinking about an opt-out -Dno_defer_overload_order flag, but I am not too confortable about that.

Would it be unreasonable to have both changes use the same flag?

@HertzDevil
Copy link
Contributor Author

Does this have any implications on #10986 ?

For it to happen the compiler must traverse the list of overloads up to three times per method call (for each visibility), or the lists must be maintained separately. This is not done here.

Would it be unreasonable to have both changes use the same flag?

In principle the two features are completely independent from each other; if they truly are, sharing the same flag shouldn't complicate troubleshooting, but I think the granularity here wouldn't hurt.

@@ -904,28 +904,34 @@ module Crystal
getter hooks : Array(Hook)?
getter(parents) { [] of Type }

def add_def(a_def)
def add_def(a_def : Def, *, ordered = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name ordered seems a bit unclear to me. As a participle it suggests that some ordering is already applied. Instead, it expresses a request to perform a sort.
Maybe a better name would be order_overloads?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, another thought is that maybe they should even be two entirely different methods. Both variants' behaviour is significantly different. Without overload ordering, the return type is Nil, not Def?.
So maybe having add_def and add_def_with_overload_ordering would be a more suitable API?

@@ -904,28 +904,34 @@ module Crystal
getter hooks : Array(Hook)?
getter(parents) { [] of Type }

def add_def(a_def)
def add_def(a_def : Def, *, ordered = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, IMO it would be better to consider the new behaviour (i.e. what you get with -Dpreview_overload_ordering) as the new default. Thus the default value should be false.

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