Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler: remove duplicate instance vars once we know them all #11995

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Compiler: remove duplicate instance vars once we know them all #11995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7681d5
Compiler: remove duplicate instance vars once we know them all
asterite Apr 11, 2022
e954e76
No need to hold all types in an instance variable
asterite Apr 11, 2022
637ffc2
Improve error message on duplicate ivar with different type
asterite Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 72 additions & 6 deletions spec/compiler/semantic/instance_var_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
50 changes: 37 additions & 13 deletions src/compiler/crystal/semantic/type_declaration_processor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -272,20 +274,14 @@ 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)
Copy link
Member

@straight-shoota straight-shoota Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very happy about the duplication of owner here. Type names can be quite a mouthful, so it would be nice to reduce their usage to the minimum.

Maybe we can rephrase this error message to be more concise?

Suggested change
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)
raise TypeException.new("instance variable '#{name}' is already declared as #{supervar.type} in #{supervar.owner} (trying to re-declare it as #{type_decl.type} in #{owner})", type_decl.location)

A similar rephrasing could be applied to the other message.

end

# Reject annotations to existing instance var
type_decl.annotations.try &.each do |_, ann|
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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, wouldn't it make sense just to use defaults and avoid the other definition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using defaults makes sense. It's not like I'd like to call this with one of the arguments and not the other. But... if you feel like it, feel free to adapt this PR to your taste :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is fine, I was just curious. And I just saw a resolved previous comment about this same thing, sorry for the nagging!

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})"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe adding the reference to re-declaring in type is confusing. type is the including class. But from a user's perspective, that's not where you find the variable. It's actually declared in a module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the conflict is that the variable was redeclared in the class because if the module inclusion. In any case are you fine with continuing this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is a gread solution. It restores backwards compatibility and fixes the original issue.

We were discussing the benefits of more strict behaviour, but that would be something for later (if at all).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick comment: I think that If we want a stricter behavior, that should be discussed now: by enabling this behavior (which wasn't really working before) we are making it part of the language. Removing it later is a major breaking change. This said, I think we agree we want this, and not having this might be a discussion worth having only for 2.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why I think this is the correct thing to do: instance variables only live on classes, because you can't instantiate modules. If an instance variable is "declared" in a module that's later included in the middle of a type hierarchy, and the type matches, I don't see why it would be a problem. It was a problem before only because a bug existed around this. But if the bug didn't exist, I can't think of another semantic for this. Why would it not compile?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the 'ol saying: with great power comes great responsibility

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|
Expand Down