Editorial: Correct GetExportedNames return type

[ghost]

The GetExportedNames method of Source Text Module Record claims it returns a List of either Strings or null. I find no way for it to ever include null in the result. Module Record apparently can be subclassed, and I'm unsure how to check all possible subclasses. I'm giving the argument here and hopefully someone familiar with modules can evaluate.

Called this Editorial because it changes no behavior, it just corrects the description of the actual behavior. But maybe it needs to be Normative because it does change a signature.

---

There's only a single use of GetExportedNames in the spec, which assumes a String-only result. GetModuleNamespace calls it an iterates the result, passing each name to ResolveExport. That method accepts only a String for the name.

The ResolveExport concrete method of a Source Text Module Record module takes argument exportName (a String) ..

GetExportedNames produces a list of names from ExportEntry.[[ExportName]]. The only situation that puts null in [[ExportName]] is a star export.

export * from './source.mjs'

All star exports end up in Module.[[StarExportEntries]].

A List of ExportEntry records derived from the code of this module that correspond to export * declarations that occur within the module

The branch of GetExportedNames that handles star exports (line 8 of the algorithm) never reads the export name. It pulls the referenced module and iterates its exported names recursively.

---

I tried to look through history to figure out when the return type was established, but it's difficult to find when a change to a single section happened. Possibly the method return type was just transferred over from the ExportEntry.[[ExportName]] type, which does allow both (to handle the star export case).

a String or null

This patch changes the GetExportedEntries return type from "List of either Strings or null" to "List of Strings", to accurately describe what it returns.

[jmdyck]

I tried to look through history to figure out when the return type was established, but it's difficult to find when a change to a single section happened.

git log -S 'a List of either Strings or *null*' finds that GetExportedNames got its return type in PR #2547.

[bakkot]

I assume that, as you say, we gave it that type because the returned list is built from (among other things) the [[ExportName]] slot of ExportEntry Records, which is typed as String or null. You have to read pretty carefully to determine those can't actually be null in this algorithm.

Specifically, [[ExportName]] can't be null here is because null is only used for the [[ExportName]] of an export * from "mod" declaration, and those only go in the [[StarExportEntries]] slot of a Source Text Module Record, not into the [[LocalExportEntries]] or [[IndirectExportEntries]] slots, which are what get used in this algorithm.

To make that clearer for future readers, would you add Assert: _e_.[[ExportName]] is not *null* before each of the two Append e.[[ExportName]] to exportedNames lines?

[ghost]

Thanks for reviewing. Just pushed that change.

[ljharb]

@bojavou would you please register as a contributor?

[ghost]

I'm not willing to compromise my privacy like that. Feel free to revert this if necessary.

[ljharb]

@bojavou not all the fields are required; what would the compromise be?

The additional issue is that user.email=134245276+bojavou@users.noreply.github.com doesn't seem like an email address that's linked to your github account.

If we revert this, we'd also have to revert #3126 and #3125.

[bakkot]

@ljharb The form does require a name and a phone number, which I at least would consider compromising my privacy if I were not using a public identity. If someone doesn't want to fill out the form, that's certainly their prerogative.

[ljharb]

Indeed, but we shouldn't be requiring a phone number, I suspect (and i believe people have previously been told it's ok to put in a fake one)

[ghost]

The required fields are personally identifying information. I've been sticking sticking to Editorial patches because Normative requires that form. But maybe this one was too close to the line. If it's better, I can stick to issues instead of patches.

Sorry about the email issue. Copypasted that wrong. I noticed it after the last one and fixed it for this patch. Is it possible you have the last one cached somewhere ?

[ljharb]

All contributions require that form, it's not just normative ones.

The previously merged PRs are indelibly linked to that incorrect email, unfortunately.

[bakkot]

Unfortunately we don't actually currently draw a distinction between editorial and normative changes, as far as I know. I would like to be able to mark contributions as not requiring the IPR form, but thus far that's not something we've been able to get approval for. Issues are fine though.

[ghost]

Oh, you're right. It doesn't mention Normative.

The Contributing file says it's required for significant patches:

.. you are required to make these rights available by signing a Contributor License Agreement (CLA) for non-trivial contributions.

If you wish to submit a proposal or make a significant PR, and you are not a representative of a TC39 member, please register as a TC39 RFTG contributor.

I was thinking of little corrections and typo fixes as trivial, and so not requiring the form. I'll just stick to issues from now on to avoid problems.

[michaelficarra]

@bojavou It is understandable that you came to that conclusion. The wording there is a bit misleading, as we have not come to an agreement about what constitutes "significant". For now, that includes all patches, so we will update the wording to make it more clear. Sorry about the confusion.

[doehyunbaek]

@bakkot It seems that changes suggested here are reverted. Can I do a follow-up PR containing changes in this PR and one additional modifications?

I want to add assertion of Assert: _e_.[[ExportName]] is not *null* before step 1.a of InitializeEnvironment AO, as ResolveExport method takes String not String | null as parameter.

[doehyunbaek]

Also if you don't mind, I would also like to add the following assertions:

• For the GetExportedNames, before step 8.a, Assert: _e_. [[ModuleRequest]] is not *null*.
• For the ResolveExport, before step 6.a.i and 9.a, Assert: _e_. [[ModuleRequest]] is not *null*.

Which seems to hold by way of construction of starExportEntries and indirectExportEntries in ParseModule.

[michaelficarra]

@doehyunbaek yes, please open a PR