Skip to content

Commit

Permalink
Skip abstract def warnings after first def with matching parameter na…
Browse files Browse the repository at this point in the history
…mes (#12167)

Fixes #12150.
  • Loading branch information
HertzDevil authored Jul 5, 2022
1 parent 6bd5709 commit 47549a6
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 14 deletions.
109 changes: 96 additions & 13 deletions spec/compiler/semantic/warnings_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -621,19 +621,6 @@ describe "Semantic: warnings" do
end

it "informs warnings once per matching overload (2)" do
assert_warning <<-CR, "warning in line 7\nWarning: positional parameter 'y' corresponds to parameter 'x' of the overridden method"
abstract class Foo
abstract def foo(x : Int32)
end
class Bar < Foo
def foo(x : Int32 | Char); end
def foo(y : Int32 | String); end
end
CR
end

it "informs warnings once per matching overload (3)" do
warnings_result(<<-CR).size.should eq(2)
abstract class Foo
abstract def foo(x : Int32)
Expand All @@ -645,5 +632,101 @@ describe "Semantic: warnings" do
end
CR
end

describe "stops warning after implementation with matching parameters is found (#12150)" do
it "exact match" do
warnings_result(<<-CR).should be_empty
abstract class Foo
abstract def foo(x : Int32)
end
class Bar < Foo
def foo(x : Int32); end
def foo(y : Int32 | String); end
end
CR
end

it "contravariant restrictions" do
warnings_result(<<-CR).should be_empty
abstract class Foo
abstract def foo(x : Int32, y : Int32)
end
class Bar < Foo
def foo(x : Int32 | Char, y : Int); end
def foo(y : Int32 | String, z : Int32); end
end
CR
end

it "different single splats" do
warnings_result(<<-CR).should be_empty
abstract class Foo
abstract def foo(x : Int32, *y)
end
class Bar < Foo
def foo(x : Int32, *z); end
def foo(y : Int32 | String, *z); end
end
CR
end

it "reordered named parameters" do
warnings_result(<<-CR).should be_empty
abstract class Foo
abstract def foo(x : Int32, *, y : Int32, z : Int32)
end
class Bar < Foo
def foo(x : Int32, *, z : Int32, y : Int32); end
def foo(w : Int, *, y : Int32, z : Int32); end
end
CR
end
end

describe "continues warning if implementation with matching parameters is not found (#12150)" do
it "not a full implementation" do
assert_warning <<-CR, "warning in line 8\nWarning: positional parameter 'y' corresponds to parameter 'x' of the overridden method"
abstract class Foo
abstract def foo(x : Int32 | String)
end
class Bar < Foo
def foo(x : Int32); end
def foo(x : String); end
def foo(y : Int32 | String); end
end
CR
end

it "single splat" do
assert_warning <<-CR, "warning in line 7\nWarning: positional parameter 'y' corresponds to parameter 'x' of the overridden method"
abstract class Foo
abstract def foo(x : Int32)
end
class Bar < Foo
def foo(x : Int32, *y); end
def foo(y : Int32 | String); end
end
CR
end

it "double splat" do
assert_warning <<-CR, "warning in line 7\nWarning: positional parameter 'z' corresponds to parameter 'x' of the overridden method"
abstract class Foo
abstract def foo(x : Int32, *, y)
end
class Bar < Foo
def foo(x : Int32, **opts); end
def foo(z : Int32, *, y); end
end
CR
end
end
end
end
46 changes: 45 additions & 1 deletion src/compiler/crystal/semantic/abstract_def_checker.cr
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class Crystal::AbstractDefChecker
# that information to check whether `target_type` implements `method` of `base`.
def implements?(target_type : Type, ancestor_type : Type, method : Def, base : Type, method_free_vars)
implemented = false
found_param_match = false
ancestor_type.defs.try &.each_value do |defs_with_metadata|
defs_with_metadata.each do |def_with_metadata|
a_def = def_with_metadata.def
Expand All @@ -122,7 +123,11 @@ class Crystal::AbstractDefChecker
check_return_type(target_type, ancestor_type, a_def, base, method)
implemented = true
end
check_positional_param_names(target_type, ancestor_type, a_def, base, method)

unless found_param_match
check_positional_param_names(target_type, ancestor_type, a_def, base, method)
found_param_match = true if same_parameters?(a_def, method)
end
end
end
end
Expand Down Expand Up @@ -276,6 +281,45 @@ class Crystal::AbstractDefChecker
true
end

def same_parameters?(m1 : Def, m2 : Def)
return false unless m1.args.size == m2.args.size

splat_index = m1.splat_index
return false unless splat_index == m2.splat_index

named_args1 = nil
named_args2 = nil

m1.args.each_with_index do |arg1, i|
arg2 = m2.args[i]

if splat_index
if i > splat_index
# named parameters may be in any order
(named_args1 ||= [] of String) << arg1.external_name
(named_args2 ||= [] of String) << arg2.external_name
next
elsif i == splat_index
# single splat name may be different; bare splats must agree
return false unless (arg1.external_name != "") == (arg2.external_name != "")
next
end
end

# positional parameter name must agree
return false unless arg1.external_name == arg2.external_name
end

if named_args1 && named_args2
named_args1.sort!
named_args2.sort!
return false unless named_args1 == named_args2
end

# double splat name may be different
m1.double_splat.nil? == m2.double_splat.nil?
end

# Checks that the return type of `type#method` matches that of `base_type#base_method`
# when computing that information for `target_type` (`type` is an ancestor of `target_type`).
def check_return_type(target_type : Type, type : Type, method : Def, base_type : Type, base_method : Def)
Expand Down

0 comments on commit 47549a6

Please sign in to comment.