1683786 - Confusing error message trying to call a private field whose value is not a function

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Orendorff [:jorendorff]]

class C {
    #f = 7;
    constructor() {
        this.#f();   // TypeError: this[#f] is not a function
    }
}

The error message is weird, and I suspect it's the expression decompiler's fault.

It should say "this.#f is not a function".

Comment 1

[Alex Vincent [:WeirdAl]]

Just because this caught my eye, I dove in as an amateur to figure out at least where the fix might go.

Two possibilities:
(1) ExpressionDecompiler::decompilePC, under JSOp::GetElem, SpiderMonkey writes the square brackets, regardless of whether it's a private field or not. pc[-2] is definitely this and pc[-1] is definitely "#f".
(2) The fact that the opcode is GetElem instead of GetProp as suggested earlier in the decompilePC function says that maybe the original parsing of the class didn't assign the right opcode. This led me to ElemOpEmitter::emitGet.

Comment 2

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

Nice detective work! I think you're on the right path with special casing the GetElem handling in the ExpressionDecompiler. The opcode is actually the correct one based on the design of Private Fields as we have them implemented at the moment.

Comment 3

[Alex Vincent [:WeirdAl]]

Unfortunately, this appears to be as far as I can take it. I don't know the bytecode API's enough to figure out how to do that special case.

Comment 4

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

It does seem like it's not a trivial thing to fix; I don't know the expression decompiler well enough to make an educated guess at the correct fix here.

Arai, would this be something you had a better guess at? Or does the fact that private fields is using GetElem really mean that we'd actually need new bytecode to fix this properly so that the decompiler has that to go on?

Comment 5

[Tooru Fujisawa [:arai]]

If there's a special sequence around GetElem for private field, (like, CheckPrivateField + Pop + GetElem that I see in the current bytecode),
it would be possible to detect that structure in BytecodeParser::parse, and record that information into Bytecode instance of GetElem (by adding new flag field),
and use the flag when decompiling GetElem, so that it uses . instead of [ and ].
same for SetElem (that may have different sequence, like CheckPrivateField + Pop + Pick + StrictSetElem in the current bytecode).

another way would be adding out-param to ExpressionDecompiler::decompilePC, about whether the decompiled code was private name atom,
and use that information to switch between ., or [ and ] (it will require rewriting already written data inside sprinter, and maybe somewhat overkill)

then, if adding separate opcode is beneficial also outside of decompiler (like, adding optimized-path, etc), I think adding separate opcode would be the clearest solution.