From 529858700a797553490bb9edce6eee3be5ab086a Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Wed, 14 Apr 2021 03:20:38 +0800 Subject: [PATCH] Consider free vars in parameters of abstract def implementations before existing types (#10503) --- spec/compiler/semantic/abstract_def_spec.cr | 19 +++++++ .../crystal/semantic/abstract_def_checker.cr | 51 ++++++++++++------- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/spec/compiler/semantic/abstract_def_spec.cr b/spec/compiler/semantic/abstract_def_spec.cr index fa503c07d9ba..c95a6e5aa664 100644 --- a/spec/compiler/semantic/abstract_def_spec.cr +++ b/spec/compiler/semantic/abstract_def_spec.cr @@ -1014,4 +1014,23 @@ describe "Semantic: abstract def" do end ), "abstract `def Foo#foo(*, foo : Int32)` must be implemented by Bar" end + + it "doesn't error if free var in arg restriction shadows another type (#10153)" do + assert_no_errors %( + module Foo + abstract def foo(x : Int32, y : Array(Int32)) + end + + class Bar + include Foo + + def foo(x : Quux, y : Array(Quux)) forall Quux + x + end + end + + class Quux + end + ) + end end diff --git a/src/compiler/crystal/semantic/abstract_def_checker.cr b/src/compiler/crystal/semantic/abstract_def_checker.cr index 8bc6a14b50b9..03d858eae947 100644 --- a/src/compiler/crystal/semantic/abstract_def_checker.cr +++ b/src/compiler/crystal/semantic/abstract_def_checker.cr @@ -46,7 +46,8 @@ class Crystal::AbstractDefChecker defs_with_metadata.each do |def_with_metadata| a_def = def_with_metadata.def if a_def.abstract? - check_implemented_in_subtypes(type, a_def) + free_vars = free_var_nodes(a_def) + check_implemented_in_subtypes(type, a_def, free_vars) end end end @@ -55,11 +56,11 @@ class Crystal::AbstractDefChecker check_types(type) end - def check_implemented_in_subtypes(type, method) - check_implemented_in_subtypes(type, type, method) + def check_implemented_in_subtypes(type, method, free_vars) + check_implemented_in_subtypes(type, type, method, free_vars) end - def check_implemented_in_subtypes(base, type, method) + def check_implemented_in_subtypes(base, type, method, free_vars) subtypes = case type when NonGenericModuleType type.raw_including_types @@ -70,13 +71,13 @@ class Crystal::AbstractDefChecker end subtypes.try &.each do |subtype| - next if implements_with_ancestors?(subtype, method, base) + next if implements_with_ancestors?(subtype, method, base, free_vars) # Union doesn't need a hash, dup, to_s, etc., methods because it's special next if subtype == @program.union if subtype.abstract? || subtype.module? - check_implemented_in_subtypes(base, subtype, method) + check_implemented_in_subtypes(base, subtype, method, free_vars) else msg = "abstract `def #{Call.def_full_name(base, method)}` must be implemented by #{subtype}" if location = subtype.locations.try &.first? @@ -88,22 +89,23 @@ class Crystal::AbstractDefChecker end end - def implements_with_ancestors?(type : Type, method : Def, base : Type) - return true if implements?(type, type, method, base) + def implements_with_ancestors?(type : Type, method : Def, base : Type, free_vars) + return true if implements?(type, type, method, base, free_vars) type.ancestors.any? do |ancestor| - implements?(type, ancestor, method, base) + implements?(type, ancestor, method, base, free_vars) end end # Returns `true` if `ancestor_type` implements `method` of `base` when computing # that information to check whether `target_type` implements `method` of `base`. - def implements?(target_type : Type, ancestor_type : Type, method : Def, base : Type) + def implements?(target_type : Type, ancestor_type : Type, method : Def, base : Type, method_free_vars) ancestor_type.defs.try &.each_value do |defs_with_metadata| defs_with_metadata.each do |def_with_metadata| a_def = def_with_metadata.def + def_free_vars = free_var_nodes(a_def) - if implements?(target_type, ancestor_type, a_def, base, method) + if implements?(target_type, ancestor_type, a_def, def_free_vars, base, method, method_free_vars) check_return_type(target_type, ancestor_type, a_def, base, method) return true end @@ -115,7 +117,7 @@ class Crystal::AbstractDefChecker # Returns `true` if the method `t1#m1` implements `t2#m2` when computing # that to check whether `target_type` implements `t2#m2` # (`t1` is an ancestor of `target_type`). - def implements?(target_type : Type, t1 : Type, m1 : Def, t2 : Type, m2 : Def) + def implements?(target_type : Type, t1 : Type, m1 : Def, free_vars1, t2 : Type, m2 : Def, free_vars2) return false if m1.abstract? return false unless m1.name == m2.name return false unless m1.yields == m2.yields @@ -169,7 +171,7 @@ class Crystal::AbstractDefChecker end if a1 && a2 - return false unless check_arg(t1, a1, t2, a2) + return false unless check_arg(t1, a1, free_vars1, t2, a2, free_vars2) end # Move next, unless we're on the splat already or at the end of the arguments @@ -195,7 +197,7 @@ class Crystal::AbstractDefChecker # Check double splat if m2_double_splat = m2.double_splat if m1_double_splat = m1.double_splat - return false unless check_arg(t1, m1_double_splat, t2, m2_double_splat) + return false unless check_arg(t1, m1_double_splat, free_vars1, t2, m2_double_splat, free_vars2) else return false end @@ -206,7 +208,7 @@ class Crystal::AbstractDefChecker m2_kargs.each do |i| a2 = m2.args[i] if a1 = kargs.delete(a2.name) || m1.double_splat - return false unless check_arg(t1, a1, t2, a2) + return false unless check_arg(t1, a1, free_vars1, t2, a2, free_vars2) else return false end @@ -217,7 +219,7 @@ class Crystal::AbstractDefChecker kargs.each_value do |a1| return false unless a1.default_value if m2_double_splat = m2.double_splat - return false unless check_arg(t1, a1, t2, m2_double_splat) + return false unless check_arg(t1, a1, free_vars1, t2, m2_double_splat, free_vars2) end end @@ -236,7 +238,7 @@ class Crystal::AbstractDefChecker end end - def check_arg(t1 : Type, a1 : Arg, t2 : Type, a2 : Arg) + def check_arg(t1 : Type, a1 : Arg, free_vars1, t2 : Type, a2 : Arg, free_vars2) if a2.default_value return false unless a1.default_value == a2.default_value end @@ -247,8 +249,8 @@ class Crystal::AbstractDefChecker if r2 && r1 && r1 != r2 # Check if a1.restriction is contravariant with a2.restriction begin - rt1 = t1.lookup_type(r1) - rt2 = t2.lookup_type(r2) + rt1 = t1.lookup_type(r1, free_vars: free_vars1) + rt2 = t2.lookup_type(r2, free_vars: free_vars2) return false unless rt2.implements?(rt1) rescue Crystal::TypeException # Ignore if we can't find a type (assume the method is implemented) @@ -331,6 +333,17 @@ class Crystal::AbstractDefChecker node.raise(message, nil) end + # Fictitious nodes for a given def's free vars, used to override type lookup + # so that they are never shadowed by existing types. Type lookup will find + # these Paths and raise because a Path isn't a type. (`#check_arg` relies on + # ignoring undefined types for free vars to work, this should be improved in + # the future.) + private def free_var_nodes(a_def : Def) + a_def.free_vars.try &.to_h do |var| + {var, Path.new(var).as(TypeVar)} + end + end + class ReplacePathWithTypeVar < Visitor def initialize(@base_type : GenericType, @generic_type : GenericInstanceType) end