diff --git a/spec/compiler/semantic/instance_var_spec.cr b/spec/compiler/semantic/instance_var_spec.cr index 0183dc35460d..19ee3a163c28 100644 --- a/spec/compiler/semantic/instance_var_spec.cr +++ b/spec/compiler/semantic/instance_var_spec.cr @@ -5534,7 +5534,7 @@ describe "Semantic: instance var" do describe "instance variable inherited from multiple parents" do context "with compatible type" do it "module and class, with declarations" do - assert_error <<-CR, "instance variable '@a' of B is already defined in A" + result = assert_type(%( module M @a : Int32 = 1 end @@ -5546,11 +5546,71 @@ describe "Semantic: instance var" do class B < A include M end - CR + + B.new.@a + )) { int32 } + + program = result.program + program.types["A"].instance_vars.size.should eq(1) + program.types["B"].instance_vars.size.should eq(0) + end + + it "module and class, with declarations (2)" do + result = assert_type(%( + module M + @a = 1 + end + + class A + include M + end + + class B < A + @a = 1 + end + + B.new.@a + )) { int32 } + + program = result.program + program.types["A"].instance_vars.size.should eq(1) + program.types["B"].instance_vars.size.should eq(0) + end + + it "module and class, with declarations (3)" do + result = assert_type(%( + module M + @a = 1 + end + + class A + include M + end + + class B < A + @a = 1 + end + + class C + @a = 1 + end + + class D < C + include M + end + + {B.new.@a, D.new.@a} + )) { tuple_of [int32, int32] } + + program = result.program + program.types["A"].instance_vars.size.should eq(1) + program.types["B"].instance_vars.size.should eq(0) + program.types["C"].instance_vars.size.should eq(1) + program.types["D"].instance_vars.size.should eq(0) end it "module and class, with definitions" do - assert_error <<-CR, "instance variable '@a' of B is already defined in A" + result = assert_type(%( module M @a = 1 end @@ -5562,7 +5622,13 @@ describe "Semantic: instance var" do class B < A include M end - CR + + B.new.@a + )) { int32 } + + program = result.program + program.types["A"].instance_vars.size.should eq(1) + program.types["B"].instance_vars.size.should eq(0) end it "accepts module and module, with definitions" do @@ -5602,7 +5668,7 @@ describe "Semantic: instance var" do context "with incompatible type" do it "module and class, with definitions" do - assert_error <<-CR, "instance variable '@a' of B is already defined in A" + assert_error <<-CR, "instance variable '@a' of A, with B < A, is already declared as Int32 (trying to re-declare it in B as Char)" module M @a = 'a' end @@ -5618,7 +5684,7 @@ describe "Semantic: instance var" do end it "module and class, with declarations" do - assert_error <<-CR, "instance variable '@a' of B is already defined in A" + assert_error <<-CR, "instance variable '@a' of A, with B < A, is already declared as Int32 (trying to re-declare it in B as Char)" module M @a : Char = 'a' end diff --git a/src/compiler/crystal/semantic/type_declaration_processor.cr b/src/compiler/crystal/semantic/type_declaration_processor.cr index 83944f6ef72b..1fd1cf5aa36b 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -155,6 +155,8 @@ struct Crystal::TypeDeclarationProcessor process_instance_vars_declarations + remove_duplicate_instance_vars_declarations + # Check that instance vars that weren't initialized in an initialize, # but a superclass does initialize then, are nilable, and if not # give an error @@ -272,7 +274,7 @@ struct Crystal::TypeDeclarationProcessor if supervar && supervar.owner != owner # Redeclaring a variable with the same type is OK unless supervar.type.same?(type_decl.type) - raise TypeException.new("instance variable '#{name}' of #{supervar.owner}, with #{owner} < #{supervar.owner}, is already declared as #{supervar.type} (trying to re-declare as #{type_decl.type})", type_decl.location) + raise TypeException.new("instance variable '#{name}' of #{supervar.owner}, with #{owner} < #{supervar.owner}, is already declared as #{supervar.type} (trying to re-declare it in #{owner} as #{type_decl.type})", type_decl.location) end # Reject annotations to existing instance var @@ -280,12 +282,6 @@ struct Crystal::TypeDeclarationProcessor ann.raise "can't annotate #{name} in #{owner} because it was first defined in #{supervar.owner}" end else - owner.all_subclasses.each do |subclass| - if subclass.is_a?(InstanceVarContainer) && (subclass_var = subclass.instance_vars[name]?) - raise TypeException.new("instance variable '#{name}' of #{subclass_var.owner} is already defined in #{owner}", subclass_var.location || type_decl.location) - end - end - declare_meta_type_var(owner.instance_vars, owner, name, type_decl, instance_var: true, check_nilable: !owner.module?) remove_error owner, name @@ -391,12 +387,6 @@ struct Crystal::TypeDeclarationProcessor supervar = owner.lookup_instance_var?(name) return if supervar - owner.all_subclasses.each do |subclass| - if subclass.is_a?(InstanceVarContainer) && (subclass_var = subclass.instance_vars[name]?) - raise TypeException.new("instance variable '#{name}' of #{subclass_var.owner} is already defined in #{owner}", subclass_var.location || type_info.location) - end - end - case owner when NonGenericClassType type = type_info.type @@ -631,6 +621,40 @@ struct Crystal::TypeDeclarationProcessor nil end + private def remove_duplicate_instance_vars_declarations + # All the types that we checked for duplicate variables + duplicates_checked = Set(Type).new + remove_duplicate_instance_vars_declarations(@program, duplicates_checked) + end + + private def remove_duplicate_instance_vars_declarations(type : Type, duplicates_checked : Set(Type)) + return unless duplicates_checked.add?(type) + + # If a class has an instance variable that already exists in a superclass, remove it. + # Ideally we should process instance variables in a top-down fashion, but it's tricky + # with modules and multiple-inheritance. Removing duplicates at the end is maybe + # a bit more expensive, but it's simpler. + if type.is_a?(InstanceVarContainer) && type.class? && !type.instance_vars.empty? + type.instance_vars.reject! do |name, ivar| + supervar = type.superclass.try &.lookup_instance_var?(name) + if supervar && supervar.type != ivar.type + message = "instance variable '#{name}' of #{supervar.owner}, with #{type} < #{supervar.owner}, is already declared as #{supervar.type} (trying to re-declare it in #{type} as #{ivar.type})" + location = ivar.location || type.locations.try &.first + if location + raise TypeException.new(message) + else + raise TypeException.new(message) + end + end + supervar + end + end + + type.types?.try &.each_value do |nested_type| + remove_duplicate_instance_vars_declarations(nested_type, duplicates_checked) + end + end + private def check_nilable_instance_vars @nilable_instance_vars.each do |owner, vars| vars.each do |name, info|