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

Reimplement optional chain and nullish coalesce #1694

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

andreabergia
Copy link
Contributor

@andreabergia andreabergia commented Oct 11, 2024

This reimplements #1591, #1593, and #1624 to fix the problems highlighted in #1688. It also replaces #1684.
Basically it implements both ?? and ?. operators.

The structure in the IR is simplified; there is just a node property to mark that the property access is optional.

The bytecode generated (for both the compiled JVM class, and our interpreter) for a?.b is something of this kind:

compute 'a'
if isNullOrUndefined {
  put undefined on the stack
} else {
  put 'b'
  call ScriptRuntime.getElem
}

I have added two new conditional jump instructions in the interpreter, IF_NULL_UNDEF and IF_NOT_NULL_UNDEF, to implement the comparison and the if with a small number of instructions. There is also a new method OptRuntime.isNullOrUndefined for compiled mode, to avoid repeating three if in a row in each generated class (and, anyway, the JVM might do the inlining itself if it looks useful).

The approach is inspired by what other JS engines do; for example, v8-debug will print this for a?.b:

    0 S> 0x3d1200040048 @    0 : 21 00 00          LdaGlobal [0], [0]
         0x3d120004004b @    3 : c9                Star1
         0x3d120004004c @    4 : a4 08             JumpIfUndefinedOrNull [8] (0x3d1200040054 @ 12)
    1 E> 0x3d120004004e @    6 : 2f f8 01 02       GetNamedProperty r1, [1], [2]
         0x3d1200040052 @   10 : 8f 03             Jump [3] (0x3d1200040055 @ 13)
         0x3d1200040054 @   12 : 0e                LdaUndefined
         0x3d1200040055 @   13 : ca                Star0
    4 S> 0x3d1200040056 @   14 : af                Return

One non-obvious detail is that if you have a?.b.c, the access to c is implied to be optional. This is necessary to implement the short circuiting, as required by the spec. The current implementation is not super optimized; it will basically do:

compute 'a'
if isNullOrUndefined {
  put undefined on the stack
} else {
  put 'b'
  call ScriptRuntime.getElem
}
if isNullOrUndefined {
  put undefined on the stack
} else {
  put 'c'
  call ScriptRuntime.getElem
}

Obviously the second if should be collapsed with the first, but I found this really complex to implement, so I did not. We get a bit worse performances than other JS engines, but hopefully this pattern is not too common to be a significant problem.

For normal property access a.b there is no change in bytecode or runtime instructions.

Note that this PR is only partial - function calls such as a.b?.(), which were already present in #1593, are implemented in #1702

@rbri
Copy link
Collaborator

rbri commented Oct 16, 2024

@andreabergia :-D

 Execution failed for task ':tests:spotlessJavaCheck'.
  The following files had format violations:
  src/test/java/org/mozilla/javascript/tests/OptionalChainingOperatorTest.java

Made special ref work with optional chaining

Made optional chaining work with expressions, `a?.[b]`

Removed useless `++maxStack`

Add new token in `AstNode`

Fixed bug in interpreter code generation - was jumping a bit too far
`a?.b.c` is `undefined` if `a?.b` is undefined, _not_ a reference error
 because `c` does not exist! This is called "short circuiting".

 The implementation is not particularly efficient - we have more than
 one check "it's undefined? then jump to the end". Ideally we'd generate
 only one such jump; however, that is quite complicated with our code
 structure and hopefully this pattern is not super common to be a real
 problem right now.
@gbrail
Copy link
Collaborator

gbrail commented Oct 19, 2024

This looks good to me -- any more opinions? Otherwise I'd like to merge it.

@rbri
Copy link
Collaborator

rbri commented Oct 19, 2024

+1 for merge

and i can't wait to see the other great stuff from @andreabergia merged too...

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.

3 participants