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: Model the bindings of a declarative Env Rec as a List of Records #2288

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Jan 24, 2021

Making this a draft PR because I'm not sure about module Env Recs.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 24, 2021

Some alternatives for DeclarativeBindingRecord:

(1)
Replace [[IsInitialized]]: Boolean, [[BoundValue]]: ECMAScript language value
with [[BoundValue]]: ~uninitialized~ or ECMAScript language value
[Later: I did this.]

(2)
To avoid the weirdness of the [[BoundValue]] field in indirect bindings...

(2a)
Instead of defining the sub-type ImportDeclarativeBindingRecord, allow the [[BoundValue]] field of a DeclarativeBindingRecord to be a record with just the [[TargetModuleRec]] and [[TargetName]] fields.

(2b)
Or you could have an abstract base type that is specialized into either having a [[BoundValue]] field or the [[TargetModuleRec]] and [[TargetName]] fields. (E.g. DeclarativeBindingRecord specializes to DirectDeclarativeBindingRecord and IndirectDeclarativeBindingRecord.) This is probably nicest from a theoretical standpoint, but 2a is basically equivalent without having to set up another type hierarchy.

[Later: I went with 2b, basically.]

@bakkot bakkot added the editor call to be discussed in the next editor call label Jan 27, 2021
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 81b6a51 to 894539c Compare January 28, 2021 22:32
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 28, 2021

(forced-pushed after rebase to master)

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 13, 2021

(force-pushed to resolve merge conflicts)

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from f958ce0 to c48c4f0 Compare July 18, 2021 14:16
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from c48c4f0 to 5841268 Compare August 25, 2021 02:28
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 5841268 to 097ae97 Compare September 14, 2021 16:10
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch 2 times, most recently from 237c68d to 03eeaf8 Compare November 12, 2021 01:54
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 03eeaf8 to 05b931b Compare January 11, 2022 03:10
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 05b931b to 47fddef Compare January 19, 2022 20:32
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 47fddef to 7d473b2 Compare January 27, 2022 02:23
@jmdyck jmdyck marked this pull request as ready for review January 27, 2022 02:26
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 27, 2022

Okay, this PR is ready for review.

Issue #2639 might affect SetMutableBinding, but probably not much.

@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 3bb7239 to ac458e8 Compare November 4, 2022 21:08
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from ac458e8 to e023ad5 Compare February 10, 2023 22:27
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from e023ad5 to 475dc26 Compare April 21, 2023 00:28
spec.html Outdated
</table>
</emu-table>

<p>A <dfn>DeclarativeBinding</dfn> is either a SimpleDeclarativeBinding or an ImportDeclarativeBinding. SimpleDeclarativeBindings can appear in any Declarative Environment Record, but ImportDeclarativeBindings can only appear in a Module Environment Record.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>A <dfn>DeclarativeBinding</dfn> is either a SimpleDeclarativeBinding or an ImportDeclarativeBinding. SimpleDeclarativeBindings can appear in any Declarative Environment Record, but ImportDeclarativeBindings can only appear in a Module Environment Record.</p>
<p>A <dfn variants="DeclarativeBindings">DeclarativeBinding</dfn> is either a SimpleDeclarativeBinding or an ImportDeclarativeBinding. SimpleDeclarativeBindings can appear in any Declarative Environment Record, but ImportDeclarativeBindings can only appear in a Module Environment Record.</p>

Same for SimpleDeclarativeBinding and ImportDeclarativeBinding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

spec.html Outdated
<th>Meaning</th>
</tr>
<tr>
<td>[[BoundName]]</td>
Copy link
Member

Choose a reason for hiding this comment

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

This field needs to be moved to the description of DeclarativeBinding since we refer to it on values that are known only to be a DeclarativeBinding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you saying it needs to be that way in order for the spec to be "correct" in some sense? Because it sounds like you're talking about statically typing the pseudocode, and you have in mind a specific rule for how that should work.

As a counter-example, consider ComposeWriteEventBytes, which has _W_.[[ByteIndex]], where _W_ is either a WriteSharedMemory event or a ReadModifyWriteSharedMemory event, each of which separately declares a [[ByteIndex]] field. There are several other such examples.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
1. <emu-not-ref>Record</emu-not-ref> that the binding for _N_ in _envRec_ has been initialized.
1. Assert: _envRec_.HasBinding(_N_) is *true*.
1. Let _binding_ be the DeclarativeBinding in _envRec_.[[Bindings]] whose [[BoundName]] field equals _N_.
1. Assert: _binding_ is a SimpleDeclarativeBinding.
Copy link
Member

@michaelficarra michaelficarra Aug 29, 2023

Choose a reason for hiding this comment

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

Can't we just say SimpleDeclarativeBinding in place of DeclarativeBinding in the above step then?

edit: Same below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think the spec does that.

That is, in a phrase like:

the [thing] in [some list] that satisfies [some constraint]

I don't think the [thing] phrase is ever restrictive. Rather, it's a descriptor that's true of every element of [some list]. All the restriction happens in [some constraint].

@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 475dc26 to 7664b2d Compare September 3, 2023 03:03
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Sep 3, 2023
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Sep 12, 2023
@jmdyck jmdyck force-pushed the declarative_Env_Rec branch from 7664b2d to ade059f Compare November 7, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants