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

asterite · 2022-04-11T18:26:57Z

Fixes #11673
Fixes #11994

I'll let CI run the specs because there's an XML spec that on my machine always segfaults and I still don't know why 🤷

src/compiler/crystal/semantic/type_declaration_processor.cr

beta-ziliani · 2022-04-11T19:32:51Z

errr... should we, like, discuss this before? 😅

IIUC here you're proposing that ivars with compatible types are OK to merge, and those with incompatible types aren't. What's confusing in the example:

          module M
            @a = 'a'
          end
 
          class A
            @a = 1
          end
 
          class B < A
            include M
          end

is that the error says that @a should be of type Int32, and not Int32 | Char. This is misguided, because it's not that there is a hard restriction for @a. If we just puts it, for instance, this might work fine. But if we're going to restrict it, then the error should be something along the lines that the @a was defined with another type before.

(but this might just be nitpicking while doing reverse engineering of your proposal)

asterite · 2022-04-11T19:57:50Z

@beta-ziliani Makes sense.

With this PR I'm making it work when it should work (compatible types.) When it doesn't work, the error isn't the ideal, but that's how it was before this issue was approached (even before #11768)

The error message can definitely be improved! But I feel that that can happen in 1.5, or maybe as a patch to 1.4.1. This PR is about not introducing a breaking change in 1.4, while also doing a correct fix for the problem :-)

asterite · 2022-04-11T20:06:42Z

I'll push a commit to improve the error message, it's pretty easy :-)

asterite · 2022-04-11T20:17:12Z

Done! There are still a couple of specs that could have an improved error message, but that's related to guessed types and I'd rather not touch those for a minor release.

straight-shoota · 2022-04-12T07:54:45Z

src/compiler/crystal/semantic/type_declaration_processor.cr

+      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})"


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.

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?

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).

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.

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?

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

straight-shoota · 2022-04-12T07:58:22Z

src/compiler/crystal/semantic/type_declaration_processor.cr

@@ -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)


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.

beta-ziliani · 2022-04-12T13:26:48Z

After thinking more about it, I'm now convinced that this behavior is the most reasonable one given how other parts of the language work, so here you have my stamp 👌 in case you need it 😄 I'm not in conditions to review it rn, but let's make it happen 🚀

asterite · 2022-04-12T14:23:58Z

Yeah, sorry about not discussing it first. Sometimes I have ideas about how to solve things so I try them and if they kind of work locally I send a PR to see if full CI passes too. In this case I tried two different approaches, the first one didn't work but this second one did. Given that this is mainly a bug fix, I didn't feel the need to open a discussion about what the fix should be (like, what should compile or not compile)

straight-shoota · 2022-04-12T15:31:37Z

I think this is totally fine. A discussion was already started and you have a clear understanding of the problem. Opening a draft PR helps to communicate the intended changes very clearly and thus helps the discussion.

beta-ziliani

Just comments 🚀

beta-ziliani · 2022-04-13T18:25:04Z

src/compiler/crystal/semantic/type_declaration_processor.cr

+      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})"


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

beta-ziliani · 2022-04-13T18:41:38Z

src/compiler/crystal/semantic/type_declaration_processor.cr

+    remove_duplicate_instance_vars_declarations(@program, duplicates_checked)
+  end
+
+  private def remove_duplicate_instance_vars_declarations(type : Type, duplicates_checked : Set(Type))


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

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 :-)

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

beta-ziliani · 2022-04-20T14:18:15Z

I added it to the milestone. We can improve the error messages later.

jgaskins · 2022-04-24T03:35:30Z

there's an XML spec that on my machine always segfaults and I still don't know why 🤷

@asterite I had segfaults with XML in Crystal, and it turns out they were in libxml2 provided by macOS. If that's what you're seeing (IIRC you're on a Mac, too), I posted the solution in this issue.

asterite added 2 commits April 11, 2022 15:26

Compiler: remove duplicate instance vars once we know them all

e7681d5

No need to hold all types in an instance variable

e954e76

jkthorne reviewed Apr 11, 2022

View reviewed changes

src/compiler/crystal/semantic/type_declaration_processor.cr Outdated Show resolved Hide resolved

asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Apr 11, 2022

Improve error message on duplicate ivar with different type

637ffc2

asterite marked this pull request as ready for review April 11, 2022 21:30

straight-shoota reviewed Apr 12, 2022

View reviewed changes

beta-ziliani approved these changes Apr 13, 2022

View reviewed changes

beta-ziliani added this to the 1.4.1 milestone Apr 20, 2022

straight-shoota approved these changes Apr 20, 2022

View reviewed changes

straight-shoota merged commit 86cc1bb into master Apr 21, 2022

straight-shoota deleted the bug/duplicate-instance-var branch April 21, 2022 10:07

Blacksmoke16 added the kind:regression Something that used to correctly work but no longer works label May 14, 2022

Blacksmoke16 mentioned this pull request May 14, 2022

fix Doesn't work in Crystal 1.4.0 amberframework/granite#464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

asterite commented Apr 11, 2022 •

edited

Loading

beta-ziliani commented Apr 11, 2022

asterite commented Apr 11, 2022

asterite commented Apr 11, 2022

asterite commented Apr 11, 2022

straight-shoota Apr 12, 2022

asterite Apr 12, 2022

straight-shoota Apr 12, 2022

beta-ziliani Apr 12, 2022

asterite Apr 12, 2022

beta-ziliani Apr 13, 2022

straight-shoota Apr 12, 2022 •

edited

Loading

beta-ziliani commented Apr 12, 2022

asterite commented Apr 12, 2022

straight-shoota commented Apr 12, 2022

beta-ziliani left a comment

beta-ziliani Apr 13, 2022

beta-ziliani Apr 13, 2022

asterite Apr 13, 2022

beta-ziliani Apr 14, 2022

beta-ziliani commented Apr 20, 2022

jgaskins commented Apr 24, 2022

	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)

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

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

Conversation

asterite commented Apr 11, 2022 • edited Loading

beta-ziliani commented Apr 11, 2022

asterite commented Apr 11, 2022

asterite commented Apr 11, 2022

asterite commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

beta-ziliani commented Apr 12, 2022

asterite commented Apr 12, 2022

straight-shoota commented Apr 12, 2022

beta-ziliani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beta-ziliani commented Apr 20, 2022

jgaskins commented Apr 24, 2022

asterite commented Apr 11, 2022 •

edited

Loading

straight-shoota Apr 12, 2022 •

edited

Loading