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

Editorial: Fixed return types of algorithms returning ~empty~ #2924

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

jhnaldo
Copy link
Contributor

@jhnaldo jhnaldo commented Sep 29, 2022

We found the return type bug in the syntax-directed operation: 15.7.12 Runtime Semantics: EvaluateClassStaticBlockBody via ESMeta type analyzer.

Currently, the type of CatchClauseEvaluation is defined as follows:

Runtime Semantics: EvaluateClassStaticBlockBody (
  _functionObject_: a function object,
): either a normal completion containing an ECMAScript language value or an abrupt completion

However, I think its return type should contain a normal completion containing ~empty~.

In step 2 for, it returns the result of Evaluation of |ClassStaticBlockStatementList|. Because of the implicit definition of operations for chain productions (ClassStaticBlockStatementList -> StatementList -> StatementListItem -> Statement -> BlockStatement -> Block), the Evaluation of |Block| might be invoked, and it returns a normal completion containing ~empty~. Thus, CatchClauseEvaluation algorithm could return a normal completion containing ~empty~.

Therefore, I suggest extending its return type as follows:

         Runtime Semantics: EvaluateClassStaticBlockBody (
           _functionObject_: a function object,
-        ): either a normal completion containing an ECMAScript language value or an abrupt completion
+        ): either a normal completion containing either an ECMAScript language value or ~empty~, or an abrupt completion

In addition, we found similar bugs in the following algorithms:

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM.

@bakkot bakkot added the editor call to be discussed in the next editor call label Oct 9, 2022
@bakkot bakkot removed the editor call to be discussed in the next editor call label Oct 19, 2022
@michaelficarra
Copy link
Member

I've rebased this on #2966.

@bakkot
Copy link
Contributor

bakkot commented Jan 11, 2023

I agree this is broken but I think there's a better way to fix it. I'll try to write it up.

@bakkot bakkot force-pushed the fix-empty-return-type branch 3 times, most recently from f917449 to 8cff0e0 Compare September 2, 2024 00:37
@bakkot
Copy link
Contributor

bakkot commented Sep 2, 2024

I've pushed a commit which handles the changes to EvaluateBody/OrdinaryCallEvaluateBody/EvaluateFunctionBody/EvaluateClassStaticBlockBody by instead having EvaluateBody only ever return return/throw completions. The other branches of EvaluateBody already have this property; it was just the ordinary function case and the class static block case which needed to be updated to do this. Then OrdinaryCallEvaluateBody no longer has to deal with normal completions at all: evaluating a function body is guaranteed to give you a return completion or throw completion.

In essence this means that the "implicit return" case (i.e., walking past the end of a function body) is now handled in EvaluateFunctionBody instead of higher up the call chain in OrdinaryCallEvaluateBody, which I consider nicer because it means fewer things need to deal with propagating a normal completion which will be end up being equivalent to a return completion holding undefined.

The fact that EvaluateBody gives specifically return/throw completions, and not break/continue completions, follows from the syntax rules preventing break/continue statements crossing function boundaries. Unfortunately, though understandably, esmeta can't recognize this invariant, so it still ends up needing a few things in esmeta-ignore.

spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm. This is great.

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Nov 20, 2024
ljharb pushed a commit to jhnaldo/ecma262 that referenced this pull request Nov 21, 2024
)

Co-authored-by: jhnaldo <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
Co-authored-by: Kevin Gibbons <[email protected]>
ljharb pushed a commit to jhnaldo/ecma262 that referenced this pull request Nov 21, 2024
@ljharb ljharb changed the title Fixed return types of algorithms returning ~empty~ Editorial: Fixed return types of algorithms returning ~empty~ Nov 21, 2024
ljharb pushed a commit to jhnaldo/ecma262 that referenced this pull request Nov 21, 2024
)

Co-authored-by: jhnaldo <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
Co-authored-by: Kevin Gibbons <[email protected]>
ljharb pushed a commit to jhnaldo/ecma262 that referenced this pull request Nov 21, 2024
)

Co-authored-by: jhnaldo <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
Co-authored-by: Kevin Gibbons <[email protected]>
@ljharb ljharb merged commit 35af909 into tc39:main Nov 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants