1650188 - [META] Private Fields Support for Devtools

Status: RESOLVED FIXED

(DevTools :: Console, task)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Firefox Devtools Issues

Right now the Private Fields JS language features is under development under the private-fields bug tree.

I did some testing, and as I had suspected, there will need to be some cooperation between DevTools and the JS engine to make private fields work well. I'll need some guidance help provide the right interfaces.

Attached is a screenshot showing at least two different issues I found while running the build I created on try atop the not-yet-landed patches in Bug 1642476.

1. The first issue is that we don't show private fields in devtools. This is not unexpected: It is intended that from a language-specification perspective that private fields are not observable via any reflection system; this means that we'll have to provide a side-entry to devtools to bypass this.
2. The act of adding a private field to an object is not idempotent; it appears that while I was experimenting with the peculiar nature of private fields, the "show the result of the computation" feature stamped window for me, then when I hit enter I got an exception, as you can't add private fields twice.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Chrome Devtools.png

The Chrome Devtools screenshot is attached to contrast with what currently exists for Firefox.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Thanks Matthew for filing this bug.
For the second error, I think we need to exclude instant evaluation when the class has private fields.
We might want to sync up with Logan to know what our options are

Comment 3

[Logan Smyth [:loganfsmyth]]

The second issue is, I would guess, because https://hg.mozilla.org/mozilla-central/diff/937d4dcd886732318b8722ee66052463a6c4bf19/js/src/debugger/Script.cpp put JSOp::InitPrivateElem in the section for sideeffect-free bytecodes. I assume that was following the lead because the other Init* bytecodes are in there, but at least for this one, this means you can initialize an element on an object that already existed. AFAIK the assumption for the other Init* bytecodes is that if you're initializing a property for the first time, the object that the property is attached to is also newly-allocated. That assumption does not hold up in for private fields in this case, so we'll need to move that to be an effectful bytecode.

Do you happen to know if any of the other Init* bytecodes will have similar issues with class fields?

Comment 4

[Logan Smyth [:loganfsmyth]]

For the first point, I could imagine a debuggerObject.getOwnPrivateNames() to go along with getOwnPropertyNames and getOwnPropertySymbols, but I'm not sure exactly what we'd want to return. I see that internally the names are symbols, I'm assuming we'd want the debugger to treat that as an implementation detail, right? So we'd need a new Debugger.PrivateName object to wrap the symbol, and then we can use the normal debuggerObject.getProperty() to get the value itself.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Logan Smyth [:loganfsmyth] from comment #3)

The second issue is, I would guess, because https://hg.mozilla.org/mozilla-central/diff/937d4dcd886732318b8722ee66052463a6c4bf19/js/src/debugger/Script.cpp put JSOp::InitPrivateElem in the section for sideeffect-free bytecodes. I assume that was following the lead because the other Init* bytecodes are in there, but at least for this one, this means you can initialize an element on an object that already existed. AFAIK the assumption for the other Init* bytecodes is that if you're initializing a property for the first time, the object that the property is attached to is also newly-allocated. That assumption does not hold up in for private fields in this case, so we'll need to move that to be an effectful bytecode.

Do you happen to know if any of the other Init* bytecodes will have similar issues with class fields?

Ah -- that's a mea-culpa then. I'll prep a patch to fix that and open a separate bug. Regarding your Q about fields: We're definitely in a bit of trouble; I'll open a new bug and cc you on it. I'm not sure what the right fix is there; it may be we cannot classify any of the initelem bytecodes as effecting.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Logan Smyth [:loganfsmyth] from comment #4)

For the first point, I could imagine a debuggerObject.getOwnPrivateNames() to go along with getOwnPropertyNames and getOwnPropertySymbols, but I'm not sure exactly what we'd want to return. I see that internally the names are symbols, I'm assuming we'd want the debugger to treat that as an implementation detail, right? So we'd need a new Debugger.PrivateName object to wrap the symbol, and then we can use the normal debuggerObject.getProperty() to get the value itself.

Yeah, we definitely don't want to expose the name as a symbol.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

let's turn this bug into a meta

Comment 8

[Zac Svoboda :zacnomore]

I am hesitant to close this as a meta bug but this appears to be fully implemented. For example class a { #x = 10; } evaluates correctly
Tagging triage owner to close

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Zac Svoboda :zacnomore from comment #8)

I am hesitant to close this as a meta bug but this appears to be fully implemented. For example class a { #x = 10; } evaluates correctly
Tagging triage owner to close

Good call, all the blockers are resolved, we can close the meta