From 47549a6312c2765f204a8273e138a7ca56896f97 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Tue, 5 Jul 2022 20:54:36 +0800 Subject: [PATCH] Skip abstract def warnings after first def with matching parameter names (#12167) Fixes #12150. --- spec/compiler/semantic/warnings_spec.cr | 109 +++++++++++++++--- .../crystal/semantic/abstract_def_checker.cr | 46 +++++++- 2 files changed, 141 insertions(+), 14 deletions(-) diff --git a/spec/compiler/semantic/warnings_spec.cr b/spec/compiler/semantic/warnings_spec.cr index c84cb9891f02..61497b95d573 100644 --- a/spec/compiler/semantic/warnings_spec.cr +++ b/spec/compiler/semantic/warnings_spec.cr @@ -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) @@ -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 diff --git a/src/compiler/crystal/semantic/abstract_def_checker.cr b/src/compiler/crystal/semantic/abstract_def_checker.cr index 049a9eb6cb2f..09705a8f9599 100644 --- a/src/compiler/crystal/semantic/abstract_def_checker.cr +++ b/src/compiler/crystal/semantic/abstract_def_checker.cr @@ -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 @@ -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 @@ -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)