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
160 changes: 160 additions & 0 deletions spec/compiler/semantic/def_overload_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -1672,6 +1672,166 @@ describe "Semantic: def overload" do
foo(1)
CRYSTAL
end

describe "without `-Dpreview_overload_order`" do
it "orders overloads as soon as they are defined" do
assert_type(<<-CR) { bool }
class Foo
end

def foo(a : Bar)
1
end

def foo(a : Foo)
true
end

class Bar < Foo
end

foo(Bar.new)
CR
end

it "overload ordering is observable from top-level macros (1)" do
assert_type(<<-CR) { int32 }
class Foo
end

class Bar < Foo
def foo(a : Foo)
true
end

def foo(a : Bar)
1
end
end

{{ Bar.methods[0].body }}
CR
end

it "overload ordering is observable from top-level macros (2)" do
assert_type(<<-CR) { int32 }
module Foo
def foo
true
end

def foo
1
end
end

{{ Foo.methods[0].body }}
CR
end

it "overload ordering is observable from macro defs" do
assert_type(<<-CR) { int32 }
class Foo
end

class Bar < Foo
def foo(a : Foo)
true
end

def foo(a : Bar)
1
end
end

def foo
{{ Bar.methods[0].body }}
end

foo
CR
end
end

describe "with `-Dpreview_overload_order`" do
it "orders overloads after all types are defined" do
assert_type(<<-CR, flags: "preview_overload_order") { int32 }
class Foo
end

def foo(a : Bar)
1
end

def foo(a : Foo)
true
end

class Bar < Foo
end

foo(Bar.new)
CR
end

it "overload ordering is not observable from top-level macros (1)" do
assert_type(<<-CR, flags: "preview_overload_order") { bool }
class Foo
end

class Bar < Foo
def foo(a : Foo)
true
end

def foo(a : Bar)
1
end
end

{{ Bar.methods[0].body }}
CR
end

it "overload ordering is not observable from top-level macros (2)" do
assert_type(<<-CR, flags: "preview_overload_order") { bool }
module Foo
def foo
true
end

def foo
1
end
end

{{ Foo.methods[0].body }}
CR
end

it "overload ordering is observable from macro defs" do
assert_type(<<-CR, flags: "preview_overload_order") { int32 }
class Foo
end

class Bar < Foo
def foo(a : Foo)
true
end

def foo(a : Bar)
1
end
end

def foo
{{ Bar.methods[0].body }}
end

foo
CR
end
end
end

private def each_union_variant(t1, t2)
Expand Down
38 changes: 38 additions & 0 deletions spec/compiler/semantic/method_missing_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,44 @@ describe "Semantic: method_missing" do
)) { int32 }
end

it "reorders expanded def immediately" do
assert_type(<<-CR) { int32 }
class Foo
macro method_missing(call)
def foo(y : Int32)
1
end
end

def foo(x : Int32 | String)
true
end
end

Foo.new.foo(y: 1)
Foo.new.foo(1)
CR
end

it "reorders expanded def immediately, even if overload ordering is deferred" do
assert_type(<<-CR, flags: "preview_overload_order") { int32 }
class Foo
macro method_missing(call)
def foo(y : Int32)
1
end
end

def foo(x : Int32 | String)
true
end
end

Foo.new.foo(y: 1)
Foo.new.foo(1)
CR
end

it "doesn't look up method_missing in with_yield_scope if call has a receiver (#12097)" do
assert_error(%(
class Foo
Expand Down
14 changes: 14 additions & 0 deletions spec/compiler/semantic/previous_def_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@ describe "Semantic: previous_def" do
)) { int32 }
end

it "types previous def even if overload ordering is deferred" do
assert_type(<<-CR, flags: "preview_overload_order") { int32 }
def foo
1
end

def foo
previous_def
end

foo
CR
end

it "types previous def in generic class" do
assert_type(%(
class Foo(T)
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/crystal/program.cr
Original file line number Diff line number Diff line change
Expand Up @@ -583,9 +583,9 @@ module Crystal
"main"
end

def add_def(node : Def)
def add_def(node : Def, *, ordered = true)
if file_module = check_private(node)
file_module.add_def node
file_module.add_def node, ordered: ordered
else
super
end
Expand Down
13 changes: 7 additions & 6 deletions src/compiler/crystal/semantic/new.cr
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ module Crystal

if check
type = type.as(ModuleType)
defer_overload_order = @program.has_flag?("preview_overload_order")

self_initialize_methods = type.lookup_defs_without_parents("initialize")
self_new_methods = type.metaclass.lookup_defs_without_parents("new")
Expand All @@ -54,11 +55,11 @@ module Crystal
if !has_new_or_initialize
# Add self.new
new_method = Def.argless_new(type)
type.metaclass.as(ModuleType).add_def(new_method)
type.metaclass.as(ModuleType).add_def(new_method, ordered: !defer_overload_order)

# Also add `initialize`, so `super` in a subclass
# inside an `initialize` will find this one
type.add_def Def.argless_initialize(type)
type.add_def Def.argless_initialize(type), ordered: !defer_overload_order
end

# Check to see if a type doesn't define `initialize`
Expand All @@ -84,8 +85,8 @@ module Crystal
if initialize_methods.empty?
# If the type has `self.new()`, don't override it
unless has_default_self_new
type.metaclass.as(ModuleType).add_def(Def.argless_new(type))
type.add_def(Def.argless_initialize(type))
type.metaclass.as(ModuleType).add_def(Def.argless_new(type), ordered: !defer_overload_order)
type.add_def(Def.argless_initialize(type), ordered: !defer_overload_order)
end
else
initialize_owner = nil
Expand All @@ -104,14 +105,14 @@ module Crystal
initialize_owner = initialize.owner

new_method = initialize.expand_new_from_initialize(type)
type.metaclass.as(ModuleType).add_def(new_method)
type.metaclass.as(ModuleType).add_def(new_method, ordered: !defer_overload_order)
end

# Copy non-generated `new` methods from parent to child
new_methods.each do |new_method|
next if new_method.new?

type.metaclass.as(ModuleType).add_def(new_method.clone)
type.metaclass.as(ModuleType).add_def(new_method.clone, ordered: !defer_overload_order)
end
end
else
Expand Down
83 changes: 83 additions & 0 deletions src/compiler/crystal/semantic/overload_order_processor.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# Reorders overloads so that more specific overloads come before less specific
# ones, according to `Crystal::DefWithMetadata#restriction_of?`. Also assigns
# the correct `previous_def`s for redefinitions.
#
# This processor is used only if `-Dpreview_overload_order` is specified;
# otherwise, every new def is ordered as soon as it is defined, which could
# cause problems because type definitions may not be complete yet:
#
# ```
# class Foo
# end
#
# module Bar
# end
#
# def foo(a : Bar)
# 1
# end
#
# # requests the definitions of `Foo` and `Bar` to determine the overload order;
# # at this point `Foo < Bar` does not hold, so the `Bar` overload is still
# # considered first
# def foo(a : Foo)
# 2
# end
#
# class Foo
# include Bar
# end
#
# foo(Foo.new) # => 1
# ```
#
# A consequence of deferring overload ordering is that top-level macros can no
# longer observe intermediate overload orders via `TypeNode#methods`.
#
# Defs added after this processor runs must still be ordered on definition (e.g.
# those from `method_missing`).
class Crystal::OverloadOrderingProcessor
def initialize(@program : Program)
@all_checked = Set(Type).new
end

def run
check_type(@program)
@program.file_modules.each_value do |file_module|
check_type(file_module)
end
end

def check_type(type)
return if @all_checked.includes?(type)
@all_checked << type

check_single(type)

type.types?.try &.each_value do |type|
check_type(type)
end
end

def check_single(type)
if type.is_a?(ModuleType)
reorder_overloads(type)
reorder_overloads(type.metaclass.as(ModuleType))
end
end

def reorder_overloads(type)
type.defs.try &.each do |method, overloads|
next unless overloads.size > 1

# simply re-add all overloads one by one
# TODO: if the overload order is confirmed to be transitive, we could sort
# the overloads in-place
unordered = overloads.dup
overloads.clear
unordered.each do |item|
type.add_def(item.def, ordered: true)
end
end
end
end
Loading