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

Remove no longer used half of RTTI #2555

Merged
merged 3 commits into from
Nov 10, 2022
Merged

Remove no longer used half of RTTI #2555

merged 3 commits into from
Nov 10, 2022

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Nov 10, 2022

The compiler no longer depends on embedded RTTI, in particular base class ids (see #2548, where instanceof helpers are now compiler-generated). There are still two uses of RTTI outside of the compiler itself, though: One is to determine pointer-free objects during GC, and the other is the loader, that both rely on type flags. The pointer-free use case is important enough to at least keep the flags, so this PR slashes RTTI in half, removing the base class id alongside the now unused programmatic __instanceof helper.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@dcodeIO dcodeIO marked this pull request as ready for review November 10, 2022 19:45
@dcodeIO dcodeIO merged commit a565d73 into main Nov 10, 2022
@jtenner
Copy link
Contributor

jtenner commented Nov 12, 2022

This is going to break ASON. Why is __instanceof being removed? There doesn't seem like a logical reason for this to occur.

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 12, 2022

It's a change resulting from #2548, where remaining open issues with interfaces have been tackled as well that required a more sophisticated approach to implement instanceof to also support interfaces. It's now compiler generated. In turn, the old mechanism to perform these checks became unused, and as such redundant. Can you explain the use case a little? Perhaps there are ways to support it otherwise.

@jtenner
Copy link
Contributor

jtenner commented Nov 12, 2022

abstract class BaseClass {}

class MyClass extends BaseClass {}

ASON.deserialize<BaseClass>(buffer); // no longer can validate a serialized MyClass

This breaks the entirety of the as-lunatic package and I would have to ask end users of ASON to do this:

let result = ASON.deserialize<BaseClass>(); // oh no it's an Array
assert(result instanceof BaseClass); // not ergonomic and backwards uncompatible

also support interfaces

Okay. I understand. Sorry for not understanding. I just feel like I have no workarounds for this.

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 12, 2022

Can ASON.deserialize<T> perhaps do:

function deserialize<T>(...) {
  let ret = ...;
  if (ret instanceof T) ... // AS allows this
}

@jtenner
Copy link
Contributor

jtenner commented Nov 12, 2022

The assert would panic if it isn't an instanceof T... let me try that.

@dcodeIO
Copy link
Member Author

dcodeIO commented Nov 12, 2022

I guess one question there is what is used as the type of ret so this doesn't become precomputed, hmm.

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

Successfully merging this pull request may close these issues.

2 participants