-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Better linking for abstract methods #3745
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
base: main
Are you sure you want to change the base?
Conversation
|
The rendered spec for this PR is available as a single page at https://tc39.es/ecma262/pr/3745 and as multiple pages at https://tc39.es/ecma262/pr/3745/multipage . |
spec.html
Outdated
| </dl> | ||
|
|
||
| <emu-alg> | ||
| 1. NOTE: This method does not use _resolveSet_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. NOTE: This method does not use _resolveSet_. | |
| 1. NOTE: This implementation of the ResolveExport method does not use _resolveSet_. |
Maybe this phrasing so it's clear we don't mean to say that all implementations do not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (As "This implementation of ResolveExport does not use resolveSet".)
spec.html
Outdated
| <td>Evaluate ( ): a Promise</td> | ||
| <td> | ||
| <p>Returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully. If the promise is rejected, hosts are expected to handle the promise rejection and rethrow the evaluation error.</p> | ||
| <p>It returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully. If the promise is rejected, hosts are expected to handle the promise rejection and rethrow the evaluation error.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <p>It returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully. If the promise is rejected, hosts are expected to handle the promise rejection and rethrow the evaluation error.</p> | |
| <p>It returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully. If the promise is rejected, hosts are expected to handle the promise rejection and rethrow the evaluation error. Unless this module is a Cyclic Module Record, the returned promise must be already settled.</p> |
While we are fixing up these descriptions... This is needed for step 1 of InnerModuleEvaluation, and has been true since when we made this method return a promise (i.e. when we added TLA).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I'm comfortable calling this editorial because as you say this is already enforced by an assert in EvaluateModuleSync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that EvaluateModuleSync is relatively new, but the assertion was already there in the previous text (https://tc39.es/ecma262/2022/#sec-innermoduleevaluation)
71294d8 to
27fe792
Compare
|
per discussion offline, will do that with a |
27fe792 to
b32eb03
Compare
|
Rebased to three commits which should all land without squashing. |
|
Opened #3747 to address all weird column widths at once. |


This updates the small number of tables of abstract methods to use the new syntax introduced in tc39/ecmarkup#666. That PR is included here.
I have not yet gone through and updated method invocations where the base is a statically knowable type to point to the concrete rather than abstract definition for the method, but that can be done in a followup.
This PR also moves GetThisBinding to the base Environment Record type, so that there is a canonical place which invocations can link to. For Environment Record subclasses on which the method is never invoked, the method is added to their sections but with no implementation and instead prose which says "The GetThisBinding concrete method of a Whatever Environment Record is never used within this specification", like we were already doing for CreateImmutableBinding for Object Environment Records. In the subclasses which did have an implementation of GetThisBinding I moved it up to match what I felt to be the natural ordering within the table.
Some of the module machinery had concrete methods which did not define an optional parameter which they didn't use. The new lint rules forbid that; instead it is included along with a new first step of the algorithm like
1. NOTE: This method does not use _resolveSet_.