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

feat(engine): class private field #602

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yossydev
Copy link
Contributor

/// ### [6.2.5.4 IsPrivateReference ( V )](https://tc39.es/ecma262/#sec-isprivatereference)
///
/// The abstract operation IsPrivateReference takes argument V (a Reference
/// Record) and returns a Boolean.
pub(crate) fn is_private_reference(_: &Reference) -> bool {
// 1. If V.[[ReferencedName]] is a Private Name, return true; otherwise return false.
// matches!(reference.referenced_name, PropertyKey::PrivateName)
false
}

↑ I have not implemented this yet, but I feel I have a basic class private field implementation.

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Hey, thank you for this! The basic implementation of the compiler part seems roughly correct on the tin, but unfortunately it is entirely missing the "private" part of the "private fields" :)

You can try this out with code like this:

class Foo {
  #a = 3;
}
const a = new Foo();
print(a.a);

This should print undefined' but will instead print 3`, because you basically drop the private part out entirely and make private fields just be normal fields.

The main thing needed to implement actual private keys will be to add a fourth variant to our PropertyKey, PropertyKey::Private (or maybe PropertyKey::PrivateIdentifier). Once that is added, then we could conceptually start talking about private fields. Unfortunately, adding that variant will bring out a few problems, that all stem from the same basic issue:

According to the specification, there is no way to represent a "private key" as a JavaScript Value.

What this means is that when our bytecode compiler creates a PropertyKey::Private, it cannot add it into the constants Vec as that is a Vec<Value>. Furthermore, we couldn't actually even add the private property keys into objects because the "keys" of objects are currently just &[Value] slices.

Now, there are ways to conquer this issue. The first possibility is to make private key representable as JS Values. V8 has chosen this path: in V8, private keys are simply unique Symbols that are marked on the heap to never appear in eg. Object.getOwnPropertyDescriptors.

We could do the same, though I personally don't think that's the best option available to us. My thinking is that we'll instead represent private keys as basically unique i56 numbers. The ability to access a private key is lexically scoped, and so the correct key number can be resolved at compile time. We do need the "name" of the private key somewhere, but it should probably be enough to keep it in the declarative environment of the class.

The great thing with this sort of a solution would be that private keys can be entirely ignored by GC: The only thing that private keys need "globally" would be an i56 counter that is incremented every time a private key gets created.

The downside here is that now private keys need their own instructions in the bytecode, OR we need constants to store a superset of Value&PrivateKey instead of just Value. Another downside is that we need to refactor object property storage to not use &[Value] for keys.

But I like those odds.

@yossydev
Copy link
Contributor Author

@aapoalas

Ahhh…
I only got # to compile, and I definitely didn’t check the most important part… (the core of it, really…)

The main thing needed to implement actual private keys will be to add a fourth variant to our PropertyKey, PropertyKey::Private (or maybe PropertyKey::PrivateIdentifier).

I see, so this is where it should be added. And yeah, there’s even a TODO there 👀

https://github.com/trynova/nova/blob/main/nova_vm/src/ecmascript/types/language/object/property_key.rs#L36

Thanks also for sharing the implementation direction for private keys!
I still need to spend more time reading and understanding the codebase as a whole, so if you’d prefer, feel free to close this PR!

I’ve realized private fields are trickier than I thought…

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