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

Rename "Environment Record" as "Scope Record", and recast Lexical Environment as a Record #1477

Closed
wants to merge 10 commits into from

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Mar 13, 2019

(Follow-up to issue #1473.)

In a comment in issue #1473, @allenwb said:

This distinction [between Lexical Environments and Environment Records] might have been clearer if I had used the term "Bindings Record" in place of "Environment Record".

I agree that a renaming would help, but I wasn't happy with "Bindings Record" for a couple of reasons. For one thing, "Bindings" is plural, which smells off to me. For another, if we ever fully specify declarative Environment/Scope Records, we'll probably define a [[Bindings]] field, and having "Bindings" on both the outside and the inside might be confusing.

I considered terms like "contour" and "nest", but then I noticed that, in the current spec, Environment Records are almost always defined as representing (or being associated with) a scope:

  • "An Environment Record records the identifier bindings that are created within the scope of its associated Lexical Environment."
  • "Each declarative Environment Record is associated with an ECMAScript program scope ..."
  • "A function Environment Record is a declarative Environment Record that is used to represent the top-level scope of a function ..."
  • "A global Environment Record is used to represent the outer most scope ..."
  • "A module Environment Record is a declarative Environment Record that is used to represent the outer scope of an ECMAScript Module."

So it seems to me that renaming "Environment Record" to "Scope Record" is a reasonably good fit.

This PR starts by renaming metavariables that currently involve the word "scope". (These all refer to Lexical Environments.)

I've split the PR into 10 commits, each of which has a set of fairly similar changes, for those who want to review it that way. Whether it stays that way on merging is up to the editors, although I'd suggest leaving at least the last commit ("Recast Lexical Environment as a Record") separate, since it's logically independent.

Effects on downstream dependencies:

  • HTML: nothing that I could see.
  • WebIDL: one reference to "object environment record", which I think is referring to the ES concept (although the "Terms defined by reference" section doesn't mention it).

spec.html Outdated
<p>A <dfn id="global-environment">global environment</dfn> is a Lexical Environment which does not have an outer environment. The global environment's outer environment reference is *null*. A global environment's EnvironmentRecord may be prepopulated with identifier bindings and includes an associated global object whose properties provide some of the global environment's identifier bindings. As ECMAScript code is executed, additional properties may be added to the global object and the initial properties may be modified.</p>
<p>A <dfn>Lexical Environment</dfn> is a specification type used to define the association of |Identifier|s to specific variables and functions based upon the lexical nesting structure of ECMAScript code. Usually a Lexical Environment is associated with some specific syntactic structure of ECMAScript code such as a |FunctionDeclaration|, a |BlockStatement|, or a |Catch| clause of a |TryStatement| and a new Lexical Environment is created each time such code is evaluated.</p>
<p>A Lexical Environment is represented as a Record with two fields: [[ScopeRecord]] and [[OuterEnv]]. The [[ScopeRecord]] field is a Scope Record, which records the identifier bindings that are created within the scope of its associated Lexical Environment. The [[OuterEnv]] field is either null, or a reference to an outer Lexical Environment. This reference is used to model the logical nesting of Lexical Environment values. The outer reference of a (inner) Lexical Environment is a reference to the Lexical Environment that logically surrounds the inner Lexical Environment. An outer Lexical Environment may, of course, have its own outer Lexical Environment. A Lexical Environment may serve as the outer environment for multiple inner Lexical Environments. For example, if a |FunctionDeclaration| contains two nested |FunctionDeclaration|s then the Lexical Environments of each of the nested functions will have as their outer Lexical Environment the Lexical Environment of the current evaluation of the surrounding function.</p>
<p>A <dfn id="global-environment">global environment</dfn> is a Lexical Environment which does not have an outer environment. The global environment's outer environment reference is *null*. A global environment's [[ScopeRecord]] may be prepopulated with identifier bindings and includes an associated global object whose properties provide some of the global environment's identifier bindings. As ECMAScript code is executed, additional properties may be added to the global object and the initial properties may be modified.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "outer environment reference" here be "[[OuterEnv]]"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At outer environment reference is *null* ? Yes, it should.

(Can you do that as a Github 'suggestion'? I want to see how they look from this side.)

Copy link
Contributor

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 id="global-environment">global environment</dfn> is a Lexical Environment which does not have an outer environment. The global environment's outer environment reference is *null*. A global environment's [[ScopeRecord]] may be prepopulated with identifier bindings and includes an associated global object whose properties provide some of the global environment's identifier bindings. As ECMAScript code is executed, additional properties may be added to the global object and the initial properties may be modified.</p>
<p>A <dfn id="global-environment">global environment</dfn> is a Lexical Environment which does not have an outer environment. The global environment's [[OuterEnv]] field is *null*. A global environment's [[ScopeRecord]] may be prepopulated with identifier bindings and includes an associated global object whose properties provide some of the global environment's identifier bindings. As ECMAScript code is executed, additional properties may be added to the global object and the initial properties may be modified.</p>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. (GitHub isn't letting me commit the suggestion from here, so I'll do it the normal way.)

@allenwb
Copy link
Member

allenwb commented Mar 13, 2019

So it seems to me that renaming "Environment Record" to "Scope Record" is a reasonably good fit.

The problem I see with that name is that the terms "environment" and "scope" are both informally used by various language implementors often for the same or very similar concepts. So having an "Environment Record" that contain (via reference or directly) a "Scope" record may be as confusing (to some readers) as what we already have.

The distinction that I think is important is the distinction between how the identifiers introduced within a specific scope/environment map(binds) to storage/values and how that specific scope/environment relates to other scopes/environments that have binding for some of the same identifiers.

I agree that the pluralness of "Binds Record" is awkward for some reason and I originally considered suggesting "Binding Record" but that isn't really right because we are talking about an abstraction that captures multiple bindings. I think some of the linguistic awkwardness comes form the word "record". Alternative names that avoid "record" seem to scan better without stumbling over singular/plural confusion: "binding map", "binding store", "binding table", etc.

It is also worth remembering that "Environment Records" are not just simple record abstractions. They are an object-like specification abstraction that implicitly dispatch a set of polymorphic specification operations depending upon the specific kind of Environment Record. So not having "record" in the name may also be good for that reason.

So, I suggest using the two terms "Environment" and "BindingTable".

An Environment has a [[Bindings]] fields that contains/references a BindingTable. But because of the polymorphic nature of Binding Tables I wouldn't expect any of them to ever have a [[Bindings] field. A Declarative Binding Table (if its specification was made more concrete, which I don't think is really necessary) might have a [[BindingMap]] field whose value is a map, an Object Binding Table would have a [[BindingObject]] field, a Global Binding Table could retain the the current [[ObjectRecord]] and [[DeclarativeRecord]] or slightly rename them (but, ugh more churn).

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Mar 18, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@littledan
Copy link
Member

I think it's nice if such refactorings/renamings help reduce complexity. I'd be in favor of the originally proposed reduction in indirection.

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jun 17, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jun 17, 2019

(force-pushed to resolve merge conflicts)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jul 19, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jul 19, 2019

(force-pushed to resolve merge conflicts again)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Sep 14, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 14, 2019

(force-pushed to resolve merge conflicts /3)

@syg
Copy link
Contributor

syg commented Sep 14, 2019

I'd rather not we rename Environment to Scope for precisely the reasons @allenwb stated. Scopes are used in at least SpiderMonkey and V8 to denote static, parse-time concepts.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 14, 2019

Scopes are used in at least SpiderMonkey and V8 to denote static, parse-time concepts.

In that case, what terms do they use to denote what the spec models as Environment Records? And, since I'm in the neighborhood, do they make a distinction between what the spec models as Environment Records and Lexical Environments? (Asking for a friend #1473.)

@syg
Copy link
Contributor

syg commented Sep 17, 2019

In that case, what terms do they use to denote what the spec models as Environment Records?

Just a quick caveat: the data structures that engines heap allocate to hold bindings during runtime for closures aren't exactly Environment Records, since the engines will only keep the bindings that are in-fact closed over. If a function's bindings can be all stack allocated, there will be no runtime component.

SpiderMonkey calls them environments or environment objects after the spec.

V8 calls them Contexts, though V8's notion of Context is broader, I think.

And, since I'm in the neighborhood, do they make a distinction between what the spec models as Environment Records and Lexical Environments? (Asking for a friend #1473.)

No, they don't and probably never will. Having 2 allocations would be very wasteful.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 17, 2019

Cool, thanks. So from that point of view, PR #1697 is probably preferable to this.

@syg
Copy link
Contributor

syg commented Sep 17, 2019

Yeah, #1697 seems fine to me.

@allenwb
Copy link
Member

allenwb commented Sep 18, 2019

I stand by my suggestion in #1477 (comment)

So, I suggest using the two terms "Environment" and "BindingTable".

And still contend that the conceptual merging done in #1697 is undesirable.

since I'm in the neighborhood, do they make a distinction between what the spec models as Environment Records and Lexical Environments?

It is irrelevant whether or not any implementation would actually use distinct data structure to represent the nesting of scopes and the scope-local bindings of identifiers. It is not the purpose of this kind of language specification to provide design suggestions for implementor (that's the job of the compiler design literature). The purpose is the specification is to clearly described the requirement semantics so that an implementor can verify that their preferred (and likely optimized) design will conform to the requirements of the specification.

It is very unlikely that an actual implementation would follow either the model has been used in the specification since ES5 or the model in #1697. There is a rich literature (and real world experience) regarding how to implement and optimize lexical scoping and closure capture. That's what a serious ES implementation would build upon and then check against the simple model used in the specification.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 18, 2019

It is not the purpose of this kind of language specification to provide design suggestions for implementor [...]. The purpose is the specification is to clearly described the requirement semantics so that an implementor can verify that their preferred (and likely optimized) design will conform to the requirements of the specification.

I agree. However, where correspondences exist between the spec and its implementations, and where there is some agreement among implementers how to name things, then I think it's helpful if the spec doesn't use conflicting names. So I respect @syg's comments about the use of the terms "scope" and "environment" in implementations.

Similarly, if implementations do have separate constructs/concepts for what the spec terms Lexical Environment and Environment Record, I'm interested in whether there's any consensus on how they're named.

(Of course, it's not just implementers who have terminology for these kinds of concepts/constructs. There's also the terminology used in the theory and teaching of programming languages. But around here, it's generally easier to get feedback from implementers than from theorists and educators.)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 14, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 14, 2019

(force-pushed to resolve merge conflicts /4)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 24, 2019
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 24, 2019

(force-pushed to resolve merge conflicts /5)

... when it's passed to NewFooEnvironment()
as the outer lexical environment.
... because they're all Lexical Environments.
@ljharb ljharb requested review from syg, michaelficarra, allenwb, bakkot and a team October 24, 2019 22:02
@michaelficarra
Copy link
Member

IMO, it'd be appropriate to tackle #1741 in this PR as well. @jmdyck do you have interest in doing that?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 25, 2019

I'm interested in tackling #1741, but I think:

1. Append _F_ to _declaredFunctionOrVarNames_.
1. When the |FunctionDeclaration| _f_ is evaluated, perform the following steps in place of the |FunctionDeclaration| Evaluation algorithm provided in <emu-xref href="#sec-function-definitions-runtime-semantics-evaluation"></emu-xref>:
1. Let _genv_ be the running execution context's VariableEnvironment.
1. Let _genvRec_ be _genv_'s EnvironmentRecord.
1. Let _gScope_ be _genv_.[[ScopeRecord]].
Copy link
Member

Choose a reason for hiding this comment

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

I know this is supposed to be a fairly mechanical renaming, but I don't really like how the names used in this one algorithm stand out. Can we make them slightly more descriptive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the names were kind of odd to begin with, and the mechanical renaming didn't improve things. I'd suggest:

  • _genv_ -> _evalVarEnv_
  • _benv_ -> _evalLexEnv_
  • _gScope_ -> _evalVarScope_
  • _bScope_ -> _evalLexScope_

(And similarly in the previous clause, for consistency.)

Copy link
Member

Choose a reason for hiding this comment

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

Go for it!

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.

You've missed an EnvironmentRecord reference in 8.1.1 globalThis (probably due to its introduction since opening this PR). Other than that, LGTM.

(the Lexical Environment component)
... and [[DeclarativeRecord]] as [[DeclarativeScope]].
(fields of the Global Scope Record)
(internal slot of ArgGetter/ArgSetter function)
(Mostly rename "_envRec_" to "_scope_", but various others.)
Includes suggestion from gibson042
(tc39#1477 (comment))
to change another occurrence of "outer environment reference"
to "[[OuterEnv]] field".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 25, 2019

You've missed an EnvironmentRecord reference in 8.1.1 globalThis (probably due to its introduction since opening this PR).

Yeah, that would have snuck in as a "non-conflicting" merge. Thanks for catching it. Fixed!

@syg
Copy link
Contributor

syg commented Oct 25, 2019

I am not in favor of this renaming. It's a lot of churn for no added clarity (and possibly confusion for people already used to the existing terminology).

To existing implementers used to reading the spec to implement scope/environment-related things, they're already tuned in to how 262 uses "Environment", including "Environment Record". To new implementers, the meaning of "Scope" in "Scope Record" doesn't exactly correspond to static notions of scoping.

@michaelficarra
Copy link
Member

@syg I think the renaming is a significant enough readability benefit for the new reader, and will not invalidate the familiarity that experienced users have acquired.

the meaning of "Scope" in "Scope Record" doesn't exactly correspond to static notions of scoping

How so?

@syg
Copy link
Contributor

syg commented Oct 25, 2019

I think the opposite about invalidating the familiarity.

As for why I think "Scope Record" is not a good name, it comes down to "scoping" meaning the visibility of bindings, which is a static property on the source text.

In something like function outer() { let x = 0; return function inner() { x++; } }, the scope of the x binding is the body of outer, regardless of how many times outer gets invoked or how many inner closures are made. Scoping doesn't say anything about "placement" of bindings: where they should live (on a LIFO short-lived thing like a stack or on a long-lived thing like an environment?), how they should be represented during evaluation. Environment Records are the spec mechanism of that placement: they're tables of bindings, get made anew per-closure, etc. One could argue that they are evaluation-time reifications of scope, but they're more, they reify both scope + placement, and I think Environment is a fine name for that.

@allenwb
Copy link
Member

allenwb commented Oct 26, 2019

@syg

As for why I think "Scope Record" is not a good name, it comes down to "scoping" meaning the visibility of bindings, which is a static property on the source text.

I totally agree with Shu. "Scope Record" is a terrible name and this usage is contrary to the common use in programming language design/implementation literature. The data structures bind "names" to entities are generally known as "environments". Environment Record is a fine name for a concrete realization of that concept.

Consider, for example, what is currently called a "Global Environment Record". It encapsulates and a Declarative Environment Record and an Object Environment Record. Those encapsulated records correspond to different binding mechanism not to different parts of the source code. Yet renaming them to as "x Scope Records" would strongly suggest exactly the latter.

@allenwb
Copy link
Member

allenwb commented Oct 26, 2019

Environment Record is a fine name for a concrete realization of that concept.

But "Bindings Record" would be even a better name.

@syg
Copy link
Contributor

syg commented Oct 31, 2019

@jmdyck We discussed this in the editor call and decided to keep the current Environment name as is, for the same reasons @allenwb and myself have expressed. Thank you for all your tireless editorial work.

@syg syg closed this Oct 31, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 31, 2019

Okay. Did you discuss @allenwb's suggestion of "Bindings Record"?

@syg
Copy link
Contributor

syg commented Oct 31, 2019

Okay. Did you discuss @allenwb's suggestion of "Bindings Record"?

Briefly. The conclusion was the improvement of "Bindings Record" over "Environment" isn't worth the amount of churn in the spec.

@bakkot
Copy link
Contributor

bakkot commented Oct 31, 2019

The "recast Lexical Environment as a Record" part of this PR still seems valuable; @jmdyck would you be willing to open it as a separate PR? I can do so if you'd prefer.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 1, 2019

The "recast Lexical Environment as a Record" part of this PR still seems valuable

Yeah, I was thinking that myself, but it might as well wait until there's a decision on PR #1697.

@jmdyck jmdyck deleted the scope-records branch March 1, 2021 14:26
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.

8 participants