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

Don't generate unique type IDs for virtual metaclasses #11188

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Sep 8, 2021

Type IDs for virtual metaclasses can be generated through at least four ways:

class A; end
class B < A; end

x = typeof(B.new || A.new) # storing the value of a `typeof` whose argument has a virtual type

(B || A || 1).is_a?(A.class) # calling `is_a?` with a virtual metaclass argument

(B || A).name # performing virtual dispatch on metaclasses
              # (because identifying `self`'s type reuses part of `is_a?`'s code)

class C < Exception; end # defining a non-leaf subclass of `Exception`
class D < C; end         # (because the rescue mechanism reuses part of `is_a?`'s code)

Those IDs are unnecessary; because virtual types are not directly exposed in Crystal code, the typeof should behave identically whether its argument has a virtual type or not:

x = typeof(B.new || A.new) # => A
# should be equivalent to:
x = typeof(A.new) # => A

That is, x's binary representation should be A.crystal_type_id in the first case, not (B | A).crystal_type_id. The variable x is still allowed to have type A+.class instead of A.class, since the method lookup process is slightly different between the two, but the binary representation of virtual metaclass variables should be identical to non-virtual ones: a single type ID referring to the non-virtual dynamic type of that variable. This is similar to how non-metaclass objects never use virtual type IDs.

is_a?(T.class) (more specifically the LLVM function ~match<T+.class>) unconditionally checks for this virtual metaclass's type ID, which explains why there is a when Foo+.crystal_type_id branch in #11134. With the above change we could assert that this branch is redundant; the default implementation will suffice, since calling each_concrete_type on a virtual metaclass will yield its non-virtual counterpart.

A blank source file generates unique IDs for Exception+.class, File::Error+.class, and IO::Error+.class. This PR makes rescue-expressions for those exception types marginally faster.

@asterite
Copy link
Member

asterite commented Sep 8, 2021

Oh, yes! I noticed this too while working on the interpreter. I didn't know what to do about it, but I think it makes sense to also remove virtual types from user-facing code. That is, don't let them have a different type ID (or, don't expose it), like you did here.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @HertzDevil 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants