Normative: function implementation hiding

[michaelficarra]

This PR implements the currently stage 2 function implementation hiding proposal. I am opening it now to get reviewer feedback and editor sign-off, and to start having a linkable spec text rendering. It is my intention to present this proposal for stage 3 at the June 2020 TC39 meeting.

[michaelficarra]

FYI I would still like feedback on alternative directive names at tc39/proposal-function-implementation-hiding#3. I don't really love the current ones, but haven't heard anything better.

[michaelficarra]

Ping @wycats @waldemarhorwat @mikesamuel. You are the stage 2 reviewers for this proposal.

[michaelficarra]

Ping @wycats @waldemarhorwat @mikesamuel once more. Please review before the upcoming meeting. I would like this proposal to advance to stage 3.

[erights]

Hi @michaelficarra , has anything changed since our discussion? IOW, is it essentially the same as what I approved in that discussion?

Can I see a rendered form of this? I do not want to evaluate this by reading ecmarkup source.

[michaelficarra]

@erights The only changes were the ones we talked about in our last call. See the most recently closed issues, particularly tc39/proposal-function-implementation-hiding#32, tc39/proposal-function-implementation-hiding#31, and tc39/proposal-function-implementation-hiding#30.

You can see a rendering of any PRs to ecma262, including this one, by clicking "Details" next to the words "Deploy preview ready!" in the CI checks below.

[erights]

You can see a rendering of any PRs to ecma262, including this one, by clicking "Details" next to the words "Deploy preview ready!" in the CI checks below.

Good to know, thanks!

[waldemarhorwat]

I'm having trouble reviewing what GetDirectiveContext is doing. It seems to make some unstated assumptions about what happens to be on the execution context stack at the time it is called. I can't tell whether that matches the lexical scope for all of the possible call paths (and note that Scope is passed as a separate parameter and strict mode scoping is done differently). Furthermore, more call paths may be added in the future, so relying on this kind of a hidden side effect seems brittle.

[waldemarhorwat]

If I understand it correctly, GetDirectiveContext seems broken. Just filed an issue for it.

[michaelficarra]

@waldemarhorwat You're right, I think I had a bug. I've removed GetDirectiveContext in 19687c8 since it wasn't really helpful anymore. Please see if that resolves your issues. If we don't use the execution context stack to track the directive state here, what alternative would you recommend? /cc @allenwb

[ljharb]

Although I think the pragma contents will still need some bikeshedding, from an editor standpoint, I'm good!

[michaelficarra]

I'm happy to bikeshed names during plenary, though sad to waste committee time on it. Haven't gotten much feedback from other delegates on tc39/proposal-function-implementation-hiding#3, unfortunately, so it would be irresponsible not to at least bring it up.

[mikesamuel]

Sorry for the delay. Looks mostly good. Some questions below.

[mikesamuel]

+1. As a reviewer, I sign off on this for stage 3.

[waldemarhorwat]

@waldemarhorwat You're right, I think I had a bug. I've removed GetDirectiveContext in 19687c8 since it wasn't really helpful anymore. Please see if that resolves your issues. If we don't use the execution context stack to track the directive state here, what alternative would you recommend? /cc @allenwb

As far as I can tell, the bug is fixed. The thing that makes me uneasy is that the use of a running execution context to implement lexical scoping makes it difficult to follow what's happening here — I had to do a deep trace of code throughout the spec to find all possible things that could be on the running execution context stack when OrdinaryFunctionCreate is called. Module and function lexical scopes push onto the execution context, but class lexical scopes don't, which is weird and not apparent without looking at the implementation. I'm with @allenwb here.

[erights]

@waldemarhorwat , I would also like to see lexical concepts avoid needless dependencies on dynamic concepts. How should this be stated?

[michaelficarra]

@waldemarhorwat @erights @allenwb I've update this PR to use static information for deriving [[HasSourceTextAvailable]] and [[PresentInStackTraces]], as opposed to tracking that information at run time via the Execution Context stack. I think this is a nice improvement. Of course, eval still uses the running Execution Context, but I'm pretty sure that's inevitable. Let me know if this satisfies your concerns.

[allenwb]

In general, I think this update has move in the right direction. However, eval code is still dependent upon the running execution context rather than being lexically based. As I said in #1739 (comment) "...there should be no significant difference in the handling of these new directives from how the strict mode direct[ive] is currently handled." But that isn't the case in this updated PR.

You have to take into account that eval is really two different things depending upon whether it is a direct eval or an indirect eval. Direct eval and indirect eval code have different lexical contexts (ignoring here for simplicity the additional differences between strict and non-strict eval code) that are statically knowable without reference to the current execution environment (notice that while it is not always knowable that a particular eval-like expression is a direct or indirect eval (or an eval at all). its possible direct and indirect eval lexical contexts are statically knowable so all that needs to be determine at execution time is whether or not the eval is direct as is done in https://tc39.es/ecma262/#sec-function-calls-runtime-semantics-evaluation 6.a.iv-vi .

[bakkot]

@allenwb

"...there should be no significant difference in the handling of these new directives from how the strict mode direct[ive] is currently handled."

I disagree with this. The strict mode directive is currently handled by defining a new kind of code, in prose, in a way which has caused pain in the past. I do not think the appropriate way to introduce these directives is to add two new kinds of code.

I'm fine with changing this PR to be more static; I think that should be doable by introducing definitions for PresentInStackTraces and HasSourceTextAvailable of CallExpression : CoverCallExpressionAndAsyncArrowHead for use in the function call semantics for direct evals. But I don't think we should try to precisely match how "use strict" works.

[allenwb]

"...there should be no significant difference in the handling of these new directives from how the strict mode direct[ive] is currently handled."

I disagree with this. The strict mode directive is currently handled by defining a new kind of code, in prose, in a way which has caused pain in the past. I do not think the appropriate way to introduce these directives is to add two new kinds of code.

Sorry, I should clarify. I wasn't talking about how you approach specifying the determination and semantics of the different kinds of directives. I was talking about the rules (that a human needs to learn) concerning how the extent of the affect of directive is determined. The extent of affect of a lexically enclosing 'use strict' directive upon an direct or indirect eval should be the same as the extent of affect of a lexically enclosing 'hide source' (etc.) directive on the same eval. In other words, what a human needs to know to determining whether a 'use strict' applies should be the same as what they need to know to determine if a 'hide source'applies. And we know, that for 'use strict' this doesn't require any knowledge of the execution environment (other than determining whether an apparent direct eval really is one).

I don't care strongly about how exactly this is expressed in the specification but I agree that the kinds of code prose may be outliving its usefulness.

[bakkot]

In other words, what a human needs to know to determining whether a 'use strict' applies should be the same as what they need to know to determine if a 'hide source' applies. And we know, that for 'use strict' this doesn't require any knowledge of the execution environment (other than determining whether an apparent direct eval really is one).

Oh, I see. That's always been true: in an earlier version of this PR the behavior was specified using knowledge of the execution environment, but that was just for editorial convenience; the actual semantics have always been static and have not changed across revisions of this PR. I had thought your concern was just about the editorial mechanism by which it was specified.

Anyway, sounds like we're on the same page.

[michaelficarra]

@allenwb Thanks for the quick feedback. @bakkot helped me to remove all usage of execution contexts. Can you review again to see if this is acceptable now?

[michaelficarra]

@waldemarhorwat or @erights Can you review the spec text here before the next meeting? Note that I will be asking to split the proposal and only advance the "hide source" directive.

[erights]

I have just reviewed with this split in mind. The portions of this relating just to hiding source text, as well as the resulting suppression of position and source location within a stack frame but no more, LGTM.

This is still a Draft so this comment is adequate, yes?

[michaelficarra]

@erights Yes that is perfect. Thank you!

[erights]

I see at https://github.com/tc39/ecma262/pull/1739/files#r351043373 that @allenwb also prefers orthogonal "hide source" and "hide stack" directives. The resulting thread asks for a use case.

Here's one: When computing across a membrane, the stack frames of the membrane mechanism itself are often a distraction to the debugging experience. Debugger often do have better ways of handling the suppression of such unwanted detail. But for so-called printf debugging, looking at stacks dumped to logs, this could reduce the verbosity a lot. The Error stacks proposal will give better knobs for this as well.

My point is that I can imagine wanting to suppress stack traces for reasons that would not imply that I also want to suppress source.

Altogether, I am happy to see this go forward with only "hide source";. We should revisit the hiding of stack frames as part of the error stacks proposal.

[michaelficarra]

FYI I've split the proposal into two commits in this PR to illustrate the intended proposal split more clearly.

[syg]

I think I get what you're trying to do here, but magically rematerializing the instance of the ancestor |CallExpression| that resulted in the particular direct eval that led to the instance of this |CallExpression| is harder to think about than I'd like. I wonder if there's a way to write this in a more obvious top-down manner, but the interleaving of static and run-time semantics definitely makes that very challenging.

One option is to have an argument-taking Static Semantics where direct eval can pass in the inherited "is sensitive" or "hide source" state. Not sure if that's a net editorial improvement...

[bakkot]

One option is to have an argument-taking Static Semantics where direct eval can pass in the inherited "is sensitive" or "hide source" state. Not sure if that's a net editorial improvement...

The problem is that we need the relevant bit at this point, during runtime semantics. Having it at the point that you are doing the eval call doesn't really help unless you propagate that information on some runtime construct like lexical environments.

[syg]

Hm, I see. If we can't come up with more obvious verbiage here, I think that sentence warrants some note that all this is doing is trying to reflect propagate the "lexical" nesting of the directives across (space and?) time, due to direct evals.

[bakkot]

We could add a NOTE reading something like

NOTE: Sensitive Directives apply to nested functions, including to code within a direct call to eval.

I'm not in love with that wording but don't have a better one off the top of my head.

[ljharb]

first commit LGTM otherwise

[bakkot]

There are 13 existing uses of the phrase "occurs within", none of which match this definition but all of which get linked as a consequence of defining it here (e.g. "Every other finite time value t is defined relative to the greatest preceding time value s that is such a multiple, and represents the instant that occurs within the same UTC day as s but follows it by t − s milliseconds".)

I think it's probably fine not to mark this as a definition.

[michaelficarra]

I would like our new use of it to be linked though. How do I do that?

[jmdyck]

Well, you could use an explicit <emu-xref> on the occurrences you want to link, or an <emu-not-ref> on the ones you don't want to link, but I think both of those approaches are sub-optimal. It would be better to come up with a different (distinct) phrase for this sense of "occurs within". E.g., "is a descendant of" or "descends from": either is a natural extension of the "child" terminology already established for parse trees, and neither occurs anywhere else in the spec.