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.

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

Fix VirtualMetaclassType#implements? to ignore base_type #12632

Conversation

straight-shoota
Copy link
Member

This method override was plainly incorrect. Removing it defaults to the super implementation which is all that we want.

Fixes #12628

This method override was plainly incorrect. Removing it defaults
to the super implementation which is all that we want.
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic kind:regression Something that used to correctly work but no longer works labels Oct 19, 2022
@straight-shoota straight-shoota changed the title Add Changelog for 1.6.1 Fix VirtualMetaclassType#implements? to ignore base_type Oct 19, 2022
@HertzDevil
Copy link
Contributor

It should be something like base_type.metaclass.implements?(other). Don't know if the CI crash is something else entirely.

@straight-shoota
Copy link
Member Author

The failures point directly to one of the added specs, so it's definitely related.

Compiler specs succeeded on my machine, though 🤔

@straight-shoota
Copy link
Member Author

One of the specs was buggy. It returned a metaclass instance from Program.run which apparently crashes (and that's probably fine). Just changed the spec code to make it work.

@HertzDevil
Copy link
Contributor

Maybe even delegate the method to base_type.metaclass, since a similar delegate is present in Crystal::VirtualType.

@straight-shoota
Copy link
Member Author

straight-shoota commented Oct 20, 2022

Do you have an example that fails without super || base_type.metaclass.implements?(other) (or delegating to base_type.metaclass)?

@HertzDevil
Copy link
Contributor

HertzDevil commented Oct 20, 2022

Only base_type.metaclass.implements?(other) ensures T+.class is a subtype of T.class, which is consistent with how T+ is a subtype of T. Ideally both are false if we follow strict virtual type semantics, e.g. something like #10785, but unfortunately the compiler doesn't do that yet. So at the moment it is best to pursue consistency first. (The code class A; end; class B < A; end; x = A; pointerof(x).value = A || B will remain invalid.)

@straight-shoota straight-shoota added this to the 1.7.0 milestone Oct 28, 2022
@straight-shoota straight-shoota merged commit 1c33a0b into crystal-lang:master Oct 29, 2022
@straight-shoota straight-shoota deleted the fix/virtual-metaclass-implements branch October 29, 2022 18:30
@beta-ziliani beta-ziliani modified the milestones: 1.7.0, 1.6.2 Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:compiler:semantic topic:lang:type-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: is_a?(self) on a union of a struct class and its metaclass considers class as an instance of itself
3 participants