From e7681d5e0c2b5d96fd48887e240e724bf236ab94 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 11 Apr 2022 15:26:32 -0300 Subject: [PATCH 1/3] Compiler: remove duplicate instance vars once we know them all --- spec/compiler/semantic/instance_var_spec.cr | 78 +++++++++++++++++-- .../semantic/type_declaration_processor.cr | 39 +++++++--- 2 files changed, 99 insertions(+), 18 deletions(-) diff --git a/spec/compiler/semantic/instance_var_spec.cr b/spec/compiler/semantic/instance_var_spec.cr index 0183dc35460d..81f70c2d5c10 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 must be Int32, not (Char | Int32)" 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 must be Int32, not (Char | Int32)" 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..0deefc59f6f3 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -122,6 +122,9 @@ struct Crystal::TypeDeclarationProcessor # of instance vars in extended modules @has_no_extenders = Set(Type).new + # All the types that we checked for duplicate variables + @duplicates_checked = Set(Type).new + @type_decl_visitor = TypeDeclarationVisitor.new(@program, @explicit_instance_vars) @type_guess_visitor = TypeGuessVisitor.new(@program, @explicit_instance_vars, @@ -155,6 +158,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 @@ -280,12 +285,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 +390,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 +624,28 @@ struct Crystal::TypeDeclarationProcessor nil end + private def remove_duplicate_instance_vars_declarations + remove_duplicate_instance_vars_declarations(@program) + end + + private def remove_duplicate_instance_vars_declarations(type : 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| + type.superclass.try &.lookup_instance_var?(name) + end + end + + type.types?.try &.each_value do |nested_type| + remove_duplicate_instance_vars_declarations(nested_type) + end + end + private def check_nilable_instance_vars @nilable_instance_vars.each do |owner, vars| vars.each do |name, info| From e954e76f57c4ad4bc860453b459168526f93d8ec Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 11 Apr 2022 15:30:12 -0300 Subject: [PATCH 2/3] No need to hold all types in an instance variable --- .../crystal/semantic/type_declaration_processor.cr | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/compiler/crystal/semantic/type_declaration_processor.cr b/src/compiler/crystal/semantic/type_declaration_processor.cr index 0deefc59f6f3..67c685e8fc0f 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -122,9 +122,6 @@ struct Crystal::TypeDeclarationProcessor # of instance vars in extended modules @has_no_extenders = Set(Type).new - # All the types that we checked for duplicate variables - @duplicates_checked = Set(Type).new - @type_decl_visitor = TypeDeclarationVisitor.new(@program, @explicit_instance_vars) @type_guess_visitor = TypeGuessVisitor.new(@program, @explicit_instance_vars, @@ -625,11 +622,13 @@ struct Crystal::TypeDeclarationProcessor end private def remove_duplicate_instance_vars_declarations - remove_duplicate_instance_vars_declarations(@program) + # 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) - return unless @duplicates_checked.add?(type) + 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 @@ -642,7 +641,7 @@ struct Crystal::TypeDeclarationProcessor end type.types?.try &.each_value do |nested_type| - remove_duplicate_instance_vars_declarations(nested_type) + remove_duplicate_instance_vars_declarations(nested_type, duplicates_checked) end end From 637ffc298e2555ec80558509df60ad143fd0ffcb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 11 Apr 2022 17:10:11 -0300 Subject: [PATCH 3/3] Improve error message on duplicate ivar with different type --- spec/compiler/semantic/instance_var_spec.cr | 4 ++-- .../crystal/semantic/type_declaration_processor.cr | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/spec/compiler/semantic/instance_var_spec.cr b/spec/compiler/semantic/instance_var_spec.cr index 81f70c2d5c10..19ee3a163c28 100644 --- a/spec/compiler/semantic/instance_var_spec.cr +++ b/spec/compiler/semantic/instance_var_spec.cr @@ -5668,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 A must be Int32, not (Char | Int32)" + 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 @@ -5684,7 +5684,7 @@ describe "Semantic: instance var" do end it "module and class, with declarations" do - assert_error <<-CR, "instance variable '@a' of A must be Int32, not (Char | Int32)" + 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 67c685e8fc0f..1fd1cf5aa36b 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -274,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 @@ -636,7 +636,17 @@ struct Crystal::TypeDeclarationProcessor # 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| - type.superclass.try &.lookup_instance_var?(name) + 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