Various spec fixes #37

nicolo-ribaudo · 2023-05-09T17:40:15Z

This PR fixes various errors or ambiguities in the spec. My original goal was to write what I think is the best layering (the first two commits), but while doing so I wound some problems with the current spec text. I will open a separate PR with the first commits.

guybedford

Thanks for the fixes, these all look correct to me.

guybedford · 2023-05-09T17:48:14Z

spec.emu

                1. <ins>Assert: _found_ is *false*.</ins>
                1. <ins>Set _found_ to *true*.</ins>
+                1. <ins>If _mr2_.[[Phase]] is ~source~ and _mr_.[[Phase]] is ~evaluation~, set _mr2_.[[Phase]] to ~evaluation~.</ins>


Actually, I'm not sure about this - as it changes the execution ordering when there is both a source and an evaluation import I think, where the source import order takes precedence?

For example, with this change:

import source 'b'; import 'a'; import 'b';

will execute b before a instead of a before b? Haven't read it through thoroughly again but let me know if that is correct?

This would execute first b and then a, I remember discussing about this ~1 month ago and concluding that it was good 😅

(I left a comment about this line in my self-review below: #37 (comment))

In my last PR I was exactly thinking about this problem when I redesigned the calls, and it seemed to solve the issue for me.

Given that we can solve it and this change isn't a big thing editorially - I would like to keep the correct normative behaviour.

nicolo-ribaudo · 2023-05-09T17:40:36Z

spec.emu

            _moduleCompletion_: either a normal completion containing a Module Record or a throw completion,
-            <ins>_phase_: ~source~ or ~evaluation.</ins>


ContinueDynamicImport is called with phase as the second parameter.

nicolo-ribaudo · 2023-05-09T17:40:51Z

spec.emu

@@ -88,7 +88,7 @@ contributors: Luca Casonato, Guy Bedford, Nicolò Ribaudo
            1. Return ~unused~.
          1. Let _module_ be _moduleCompletion_.[[Value]].
          1. <ins>If _phase_ is ~source~, then</ins>
-            1. <ins>Let _moduleSourceObject_ be _module_.GetModuleSource().</ins>
+            1. <ins>Let _moduleSourceObject_ be Completion(_module_.GetModuleSource()).</ins>


Completion records are always explicitly marked in the spec.

nicolo-ribaudo · 2023-05-09T17:42:33Z

spec.emu

                1. <ins>Assert: _found_ is *false*.</ins>
                1. <ins>Set _found_ to *true*.</ins>
+                1. <ins>If _mr2_.[[Phase]] is ~source~ and _mr_.[[Phase]] is ~evaluation~, set _mr2_.[[Phase]] to ~evaluation~.</ins>


Now that we use GetSourceObject() instead of an external slot, we can again deduplicate module requests and only ask to the host "I will need the module up to this phase". The host doesn't need the separate ModuleRequests to know when to create the source object anymore.

^{I still think that this was never necessary since it was never observable, but it's even less necessary now :P}

nicolo-ribaudo · 2023-05-09T17:42:55Z

spec.emu

@@ -292,6 +293,8 @@ contributors: Luca Casonato, Guy Bedford, Nicolò Ribaudo
            </tbody>
          </table>
        </emu-table>
+
+        <emu-note type="editor">In general, this proposal replaces places where module specifiers are passed around with ModuleRequest Records. For example, several syntax-directed operations, such as ModuleRequests produce Lists of ModuleRequest Records rather than Lists of Strings which are interpreted as module specifiers. Some algorithms like ImportEntries and ImportEntriesForModule pass around ModuleRequest Records rather than Strings, in a way which doesn't require any particular textual change. Additionally, record fields in Cyclic Module Records and Source Text Module Records which contained Lists of Strings are replaced by Lists of ModuleRequest Records, as indicated above.</emu-note>


I think this was deleted in the other PR by accident?

nicolo-ribaudo · 2023-05-09T17:43:12Z

spec.emu

@@ -417,15 +420,12 @@ contributors: Luca Casonato, Guy Bedford, Nicolò Ribaudo
            </tr>
            <tr>
              <td>
-                <ins>GetModuleSource(): either a normal completion containing an Object or a throw completion</ins>
+                <ins>GetModuleSource()</ins>


We cannot have return types here in tables, I moved it to the description.

nicolo-ribaudo · 2023-05-09T17:44:00Z

spec.emu

-                </emu-note>
+                <p><ins>It returns either a normal completion completion for the ModuleSource Object corresponding to this source Module Record's source phase (<emu-xref href="#sec-module-source-objects"></emu-xref>), or a throw completion.</ins></p>
+                <ins>When called multiple times on the same Module Record, if GetModuleSource() returns a normal completion it must always return a normal completion containing the same object.</ins>
+                <ins>The returned object should be a subclass of %AbstractModuleSource%, and it must have an internal slot [[ModuleSourceRecord]].</ins>


There are not just notes, but normative requirements. I used "should" for the subclassing bit because as already discussed it's not really enforceable.

nicolo-ribaudo · 2023-05-09T17:44:52Z

spec.emu

                1. Append _module_ to _state_.[[Visited]].
                1. Let _requestedModulesCount_ be the number of elements in _module_.[[RequestedModules]].
                1. Set _state_.[[PendingModulesCount]] to _state_.[[PendingModulesCount]] + _requestedModulesCount_.
                1. <del>For each String _required_ of _module_.[[RequestedModules]], do</del>
                1. <ins>For each ModuleRequest Record _required_ of _module_.[[RequestedModules]], do</ins>
                  1. If _module_.[[LoadedModules]] contains a Record whose [[Specifier]] is _required_<ins>.[[Specifier]]</ins>, then
                    1. Let _record_ be that Record.
-                    1. <ins>If _required_.[[Phase]] is not ~source~, then</ins>
-                      1. Perform InnerModuleLoading(_state_, _record_.[[Module]]<ins>, ~evaluation~</ins>).
+                    1. Perform InnerModuleLoading(_state_, _record_.[[Module]], <ins>_required_.[[Phase]]</ins>).


We must always call InnerModuleLoading, for the steps about PendingModulesCount. The loop to recursively load the dependencies has a phase guard anyway, so it will be skipped for source imports.

nicolo-ribaudo · 2023-05-09T17:45:20Z

spec.emu

                <ins>_phase_: ~source~ or ~evaluation~,</ins>
+                _moduleCompletion_: either a normal completion containing a Module Record or a throw completion,


ContinueModuleLoading was called passing the phase as the second parameter.

nicolo-ribaudo · 2023-05-09T17:49:43Z

spec.emu

@@ -919,27 +917,24 @@ contributors: Luca Casonato, Guy Bedford, Nicolò Ribaudo
            The host environment must perform FinishLoadingImportedModule(_referrer_, <del>_specifier_</del><ins>_moduleRequest_</ins>, _payload_, _result_), where _result_ is either a normal completion containing the loaded Module Record or a throw completion, either synchronously or asynchronously.
          </li>
          <li>
-            <p>If this operation is called multiple times with <del>the same</del><ins>two</ins> (_referrer_, <del>_specifier_</del><ins>_moduleRequest_</ins>) pair<ins>s</ins> such that:</p>


I rephrased a bit this section about the requirements of HostLoadImportedModule to be more similar about the current spec rather than the import attributes proposal, since this proposal doesn't change the requirements for module caching.

Additionally:

"the phase should not affect the identity or state of the resultant Module Record" is now a must, stating that the returned completion record must not be affected by the phase.

I moved the sentence about preloading into a note, first explaining what preloading is and then saying that implementations should take into account the module phase. This is just a note because preloading is not defined anywhere, and it already happens through mechanism outside of ECMA-262 or HTML (which today I learned this makes it implementation-defined and not host-defined).

cc @lucacasonato #36 (comment)

nicolo-ribaudo · 2023-05-09T17:52:07Z

spec.emu

@@ -1036,7 +1031,7 @@ contributors: Luca Casonato, Guy Bedford, Nicolò Ribaudo
      </emu-alg>
      <emu-grammar>ImportDeclaration : `import` ImportClause FromClause `;`</emu-grammar>
      <emu-alg>
-        1. Let _module_ be <del>the sole element of ModuleRequests</del><ins>SV</ins> of |FromClause|.
+        1. Let _module_ be the sole element of ModuleRequests of |FromClause|.


I think you forgot to revert this while reverting the changes to ImportEntries that I initially suggested copying from the spec I wrote for defer eval (I like overloading [[ImportName]] to also be ~source~/~defer~, as you did, more :) ).

guybedford

Changing my approval until we work through the normative execution ordering case.

Everything else is 100% great, and I am in full agreement on.

nicolo-ribaudo · 2023-05-09T18:21:01Z

Normative change reverted.

For other readers:

Consider the following code:

import source _ from "A";
import "B";
import "A";

The loading order is A and then B. What should be the evaluation order?
Before #36 it was A and then B, #36 changed it to B and then A, and in this PR I was originally reverting it back to A and then B because the change wasn't mentioned so I didn't realize it was intentional.

guybedford · 2023-05-09T18:37:19Z

Thanks for working these through!

nicolo-ribaudo added 4 commits May 9, 2023 19:20

Partial revert of pr 36, only passing the specifier to the host

b2f4985

Add note about preloading

6ac77a5

Explicitly pass the phase to the host hook

3fd689b

Use the Module Record to rebuild the loading continuation state

90661ee

guybedford approved these changes May 9, 2023

View reviewed changes

guybedford reviewed May 9, 2023

View reviewed changes

nicolo-ribaudo commented May 9, 2023

View reviewed changes

guybedford requested changes May 9, 2023

View reviewed changes

Revert normative change

8c2c9f8

guybedford approved these changes May 9, 2023

View reviewed changes

guybedford merged commit 99b13f3 into tc39:main May 9, 2023

nicolo-ribaudo deleted the layering-changes-3 branch May 9, 2023 18:37

nicolo-ribaudo mentioned this pull request May 10, 2023

Stage 3 reviews #31

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various spec fixes #37

Various spec fixes #37

nicolo-ribaudo commented May 9, 2023

guybedford left a comment

guybedford May 9, 2023

nicolo-ribaudo May 9, 2023 •

edited

Loading

guybedford May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

nicolo-ribaudo May 9, 2023

guybedford left a comment

nicolo-ribaudo commented May 9, 2023

guybedford commented May 9, 2023

		_moduleCompletion_: either a normal completion containing a Module Record or a throw completion,
		<ins>_phase_: ~source~ or ~evaluation.</ins>

		<ins>_phase_: ~source~ or ~evaluation~,</ins>
		_moduleCompletion_: either a normal completion containing a Module Record or a throw completion,

Various spec fixes #37

Various spec fixes #37

Conversation

nicolo-ribaudo commented May 9, 2023

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented May 9, 2023

guybedford commented May 9, 2023

nicolo-ribaudo May 9, 2023 •

edited

Loading