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

Should eval?.() be direct eval? #2062

Closed
rkirsling opened this issue Jun 24, 2020 · 44 comments
Closed

Should eval?.() be direct eval? #2062

rkirsling opened this issue Jun 24, 2020 · 44 comments

Comments

@rkirsling
Copy link
Member

rkirsling commented Jun 24, 2020

Issue-ization of tc39/test262#2667 (comment).

eval?.() is specified as indirect eval, which is evidently intentional, but speaking as a reviewer and implementer, I'm not sure how many of us understood this to be the case.

In particular, JSC / SM / V8 all implemented and shipped what we expected the spec to say—i.e., that "code with ?. behaves the same as code without it when the LHS is non-nullish"—so we have a divergent reality here.

Whichever way we resolve this, I think it's necessary to have a proper discussion about it.
I'll also create a PR for moving to direct eval, at least for demonstration's sake.

@rkirsling
Copy link
Member Author

rkirsling commented Jun 24, 2020

My understanding of the factors to weigh:

  • Direct eval would be more consistent behavior and thus easier for users to remember (though it's true that few will ever need to know this).

  • Indirect eval could be more useful among those that do need to know: eval?.() is easier to write than (0,eval)(), whereas if it's the same as eval() then there's no reason to write it at all.

  • Direct eval is (potentially) more straightfoward to implement, since there isn't any additional binding to incur indirection here and AST-wise one can just wrap the existing eval call node with an Optional node.

  • Spec-wise, I understand that some folks would rather not ever introduce new cases of direct eval. (I wasn't thinking of this as a "new" case so much as rewrapping an "old" case, but since the direct eval algorithm text is strictly associated with CallExpression, it turns out we would need to duplicate it for OptionalExpression...)

@claudepache
Copy link
Contributor

eval?.() is specified as indirect eval, which is evidently intentional,

It wasn’t exactly intentional; rather, it was intentionally unintentional. When I wrote the spec, I deliberately chose to ignore what I considered to be a non-issue.

IOW, the currently specced semantics is a consequence of how direct eval was specced at the time of introduction of optional chaining, not a deliberate decision.

@anba
Copy link
Contributor

anba commented Jun 24, 2020

* Direct eval is (potentially) more straightfoward to implement, since there isn't any additional binding to incur indirection here and AST-wise one can just wrap the existing eval call node with an Optional node.

FWIW SpiderMonkey was able to quickly change from direct to indirect eval, because for us it was a one line change: mozilla/gecko-dev@8cc6255

I understand that other implementations may have more trouble here.

Unless someone else from the SpiderMonkey team (cc @codehag) has any concerns about the change to indirect eval, I'll probably won't revert the commit for now, so we can report to TC39 if we encounter any web-compatibility issues (, which I expect to be rather unlikely 😄).

@codehag
Copy link
Contributor

codehag commented Jun 24, 2020

I am following it. Will report back if there are any issues. I also do not suspect that there will be any.

@littledan
Copy link
Member

FWIW I'm happy with eval?.() not being a new direct eval case. I don't really see the motivation for adding more direct eval cases. If we leave the semantics as is, it would probably be helpful to add a note indicating that this case is treated as indirect eval, to avoid confusing future implementers and other spec readers.

@rkirsling
Copy link
Member Author

@anba, @codehag: Fair enough—this discussion is definitely predicated on both ways still being web-compatible, so if we found that not to be the case, that'd be the strongest deciding factor!

@claudepache: Thanks, that's a really helpful clarification. So then it's much like the delete x?.y case, where we did what "comes for free" in the spec, even though it requires conscious effort in implementation. I do think eval?.() is a bit more concerning though, since delete x?.y still follows the aforementioned rule of thumb.

@littledan: Adding more direct eval cases is certainly not a goal in itself, though it seems that not doing so might be considered a goal among certain delegates. My own goal here is really just to have the discussion now that we forgot to have during review, and I'm trying to evaluate by priority of constituencies.

To resummarize in paragraph form:

If eval?.() were something we expected or encouraged the average user to write, then I think it would need to be direct eval to avoid confusion. Since that's not case, the situation is fuzzier. Below the level of userland, either the implementation needs to add a restriction for direct eval or the spec needs to add an allowance for it, so that part is just a question of which side of the line the burden falls on.

@devsnek
Copy link
Member

devsnek commented Jun 24, 2020

FWIW it looks like updating V8 to make this an indirect eval would be a single-line-ish change.

@rkirsling
Copy link
Member Author

JSC would be as well; I'm sorry if I made it sound like this would imply a major refactor.

I just want our decision here to be conscious and not accidental.

@claudepache
Copy link
Contributor

@rkirsling

So then it's much like the delete x?.y case, where we did what "comes for free" in the spec, even though it requires conscious effort in implementation.

No, delete x?.y has exactly one reasonable semantics. It is much more like document.all?.foo, where different people have different expectations.

Note that eval has very peculiar semantics; for instance, it is the only function object that behaves differently depending on the name of the binding it is stored in. So, I am not particularly concerned if your particular “rule of thumb” about ?. is not satisfied either.

@claudepache
Copy link
Contributor

A third option is to make eval?.(x) a Syntax Error.

@rkirsling
Copy link
Member Author

@claudepache
Sorry, I was referring specifically to your comment that "I deliberately chose to ignore what I considered to be a non-issue"—I mean that we allowed delete x?.y because it was zero cost to do so spec-side, though this did require some acrobatics implementation-side; similarly, the spec for eval?.() is currently written in a way that's zero cost spec-side but does require a conscious check implementation-side.

I do see your point in analogizing with document.all though—if we say that we resolved that issue by choosing not to proliferate undesirable legacy semantics, then I suppose the same argument could apply for indirect eval.

@ljharb
Copy link
Member

ljharb commented Jun 25, 2020

A third option is to make eval?.(x) a Syntax Error.

wouldn't that mean that function eval() {} ; eval?.() would be a syntax error? or would we only let it error when it's %eval%?

@devsnek
Copy link
Member

devsnek commented Jun 25, 2020

fwiw function eval() {} is already a syntax error in strict mode. I don't think eval?.() should be a syntax error though.

@ExE-Boss
Copy link
Contributor

FWIW, babel and TypeScript also transpile eval?.() as a direct eval.

@nicolo-ribaudo
Copy link
Member

Note that Babel's behavior was accidental (but my opinion matches our implementation: eval?.() should be direct eval for symmetry with eval()).

@caridy
Copy link
Contributor

caridy commented Jun 29, 2020

cc @erights

I second @littledan's comment. Indirect eval should be just fine in this case.

@erights
Copy link

erights commented Jun 30, 2020

@claudepache says

A third option is to make eval?.(x) a Syntax Error.

I agree. I suggest the "Principle of Least Damaging Surprise" as a name for the principle we followed for the -x**y case.

There's a syntactic construct S that some would guess has meaning M1 and some would guess has meaning M2.

  • If we give it meaning M1, then programs written by those who thought it had meaning M2 might silently deviate for their authors intentions at runtime. We need to examine how damaging M1 behavior is to programs written assuming M2.
  • and vice versa of course. How damaging is M2 behavior to programs written assuming M1? Well, if the deviation is only a surprising throw, that's not too bad. If it is a different number, that's very bad.
  • If we make S a syntax error, then both audiences are surprised, raising the number surprised. However, the consequence of the surprise is "Hmmm, that didn't work. How do I write my program so it will be accepted?" This happens only during development, not at runtime on some customer's device.

In this case, I don't think it matters much because I don't think anyone will naively write this. However, even if the effect is minor, syntax error causes the least damage.

@rkirsling
Copy link
Member Author

rkirsling commented Jun 30, 2020

While I think the error approach is worthy of mention, I don't think I agree that it's less surprising in this case. It might have potential to be more surprising here, for the reason that @ljharb was approaching above—even in strict mode, I can freely set globalThis.eval to null or another function, so eval?.() really can be nullish or, in any case, something other than %eval%.

Given that eval has this undeniable identifier-ness, I think either semantics would still be preferable to an error.

@claudepache
Copy link
Contributor

@rkirsling Whatever we do, it will be more surprising for someone. The idea of the syntax error, is not to make the behaviour less surprising, but to make it impossible to get wrong in case they have the wrong intuition. The fact that it is an early error means that there is very low chance that it will appear by accident in production.

@allenwb
Copy link
Member

allenwb commented Jun 30, 2020

I'm not sure whether direct eval or indirect eval is the correct question.

As specified, eval() is a statically detected special form variant of CallExpression. The runtime semantics of the special form is roughly:

if the current value of `eval` is %eval% (ie, the built-in eval function of the CallExpression's realm)
   then do a direct eval using the CallExpression's argument
   else  use the normal CallExpression semantics (ie, just do  normal call behavior including error checks)

So it isn't choosing between a direct eval or an indirect eval, it is choosing between a direct eval and a normal call. It might be an attempt to call something that is not callable (eg, undefined) or it might be calling any other function.

So, what does a normal programmer expect when writing eval(expr). If they have not guarded it with a predicate then they should be expecting a direct eval. If they wanted an indirect eval (whose semantics is quite different from direct eval) they would have coded it using a formulation that forces an indirect eval.

What should they expect if they intentionally wrote eval?.(expr). Presumably based upon orthogonal composition of features, the exact same behavior as eval(expr) except that the else-clause of the semantics has an additional null/undefined guard.

The simplest thing from a programmer's perspective is to just threat ?. in a call as an orthogonal feature. Any special case treatment, including a syntax error for eval just adds to the already too large set of arbitrary special case rules that JavaScript programmers need to learn.

@syg
Copy link
Contributor

syg commented Jun 30, 2020

The simplest thing from a programmer's perspective is to just threat ?. in a call as an orthogonal feature.

Well stated. @allenwb's argument here matches my thinking exactly.

@leobalter
Copy link
Member

So, what does a normal programmer expect when writing eval(expr). If they have not guarded it with a predicate then they should be expecting a direct eval. If they wanted an indirect eval (whose semantics is quite different from direct eval) they would have coded it using a formulation that forces an indirect eval.

I am not sure we can assume what they want for indirect or direct evals in here.

What should they expect if they intentionally wrote eval?.(expr). (...)

If I understand correctly, we are setting some sort of a default path for usages of eval calls that are not straight up eval(x). I could say that even affects globalThis.eval(x) or window.eval(x) as indirect evals would be enough reason for such a thing like eval?.(x) being indirect as well.

I don't worry too much which direction we take here, but I believe we are creating an exception if we change the spec, rather than enforcing an intended default behavior.

Today, what we have as default is: direct eval is only direct calls to a name eval that is locally available. Indirect eval is everything else, even an expression that evaluates to eval such as (0, eval) or window.eval.

If we change the specs as proposed, we add a new specific rule such as: direct eval is also possible through optional chain. We remove something out from the "everything else" bag of indirect eval.


On another note, I don't think it's ECMAScript's responsibility to guard developers against eval?.(x), I'd avoid making it a syntax error.

@allenwb
Copy link
Member

allenwb commented Jul 2, 2020

If we change the specs as proposed, we add a new specific rule such as: direct eval is also possible through optional chain. We remove something out from the "everything else" bag of indirect eval.

No, I don't think you understand how "direct eval" is statically recognized by the specification in 12.3.6.1. The guard that is used to recognize a direct eval is:

If Type(ref) is Reference, IsPropertyReference(ref) is false, and GetReferencedName(ref) is "eval", then

The intent of this guard is to ensure that the MemberExpression expression that provides the function value is exactly the Identifier eval. So, eval(x) will usually be a direct eval (it must be the correct Realm specific value) but globalThis.eval(x) window.eval(x), or foo?.eval(x) is never recognized as an direct eval.

If the evaluation abstraction operation for MemberExpression?.(x) uses the same guarded logic as 12.3.6.1 then eval?.(x); will work exactly as eval(x); for cases where eval binds to its normal built-in value and exactly like let $$foo=eval; $$foo?.(x)` for all other cases.

@leobalter
Copy link
Member

I understand how it is spec'ed and I'm glad implementations are consistent, as anything from my Test262 perspective.

My point of view here is as a web dev, out of my personal intuition, putting any spec work aside and having close to 0 experience as implementor. As a web dev, writing JS code, I'd expect eval?.(x) to be indirect because I see it as odd as eval coming in from a property reference.

As I mentioned, my intuition goes as we are proposing an exception for the general rule where direct evals were limited to roughly MemberExpression Arguments (out of the CoverCallExpressionAndAsyncArrowHead). We are now extending the rule to, as you mentioned, MemberExpression ?. Arguments.

I'm not objecting to this change, but I don't think it's the best path.

@hax
Copy link
Member

hax commented Jul 10, 2020

I think it's very hard to say how developers expect eval?.() be direct or indirect eval.

On one side, I believe many developers simply explain eval?.(x) as eval != null ? eval(x) : undefined so it should be direct eval.

On the other side, some developers (especially those have much deeper comprehension of direct/indirect eval) will expect it as indirect eval because they understand direct eval as an operator (think eval(x) as typeof(x)), and if the form is not look like operator it should be treated as indirect eval (typeof?.(x) is invalid so eval?.(x) is not "operator" and must be indirect call).

Both side seems reasonable, so my first feeling is we should make it as syntax error.

But we already can write let eval = () => {}; eval(), so make eval?.() as syntax error also introduce surprise, though I guess no one write such code in real world.

If we couldn't make it a syntax error, I guess maybe we can make eval?.() throw TypeError if eval is %eval%. Though it's a little bit magical. (eval is magical anyway :-)

@erights
Copy link

erights commented Jul 10, 2020

Lending further support for syntax error, notice that

let eval = () => {}; eval()

is already a syntax error in strict code.

@allenwb
Copy link
Member

allenwb commented Jul 10, 2020

@erights

let eval = () => {}; eval()

The syntax error in strict mode comes from the assignment, not the CallExpression. If just the assignment was done within non-strict code, the strict mode CallExpression would go ahead and invoke the arrow function. I don't see how the strict mode restriction on assignment to eval is relevant to discussing the semantics of eval?.().

A more relevant consideration may be consistency with other syntactic forms that look like calls. For example, import(). Presumably import?.() is a syntax error because it isn't explicitly allowed by the grammar.

@ljharb
Copy link
Member

ljharb commented Jul 10, 2020

In that case tho, import isn't an identifier like eval is - import can never be nullish so that import() would fail.

@hax
Copy link
Member

hax commented Jul 11, 2020

Consider let eval = ... already syntax error in strict codes and modules, It seems the only potential use case of eval?.(s) is assuming someone or some env will nullify globalThis.eval. But consider the CSP, it still could throw, so I guess in practice, u'd better write canEval && eval(s) (or try eval(s) as https://github.com/isiahmeadows/proposal-try-expression) instead of eval?.(s) .

So I think make it a syntax error may be the best option.

@erights
Copy link

erights commented Jul 11, 2020

@allenwb

The syntax error in strict mode comes from the assignment, not the CallExpression. If just the assignment was done within non-strict code,

First, I'll take this rare opportunity to say " declaration, not assignment ". In fact assignment would be allowed strict, which actually weakens the point I am trying to explain. In any case, that point is

For programmers just writing normal strict code, it would be very rare for eval to be bound to anything other than %eval%. I took an earlier comment to raise the issue of a programmer being surprised if they just innocently happen to use eval for one of their own function names. This will not arise innocently among programmers writing strict code. Therefore, the syntax error damages no one.

Programmers who bind eval sloppy and then use it strict are in it deep enough that they'll know what to do when their weird code is rejected by this syntax error.

@allenwb
Copy link
Member

allenwb commented Jul 11, 2020

I agree that it is doubtful whether there is any reasonable reason for anybody to write eval?.(arg). But that doesn't mean it should be a syntax error.

Quote myself from #2062 (comment)

The simplest thing from a programmer's perspective is to just threat ?. in a call as an orthogonal feature. Any special case treatment, including a syntax error for eval just adds to the already too large set of arbitrary special case rules that JavaScript programmers need to learn.

This basically a complexity budget issue. Don't make the language unnecessarily more complex by adding a nanny-style error. Better for linters to have a rule about eval?.()

@erights
Copy link

erights commented Jul 11, 2020

This basically a complexity budget issue.

Thank you. As you appreciate, I love arguments from complexity budget. I agree that a SyntaxError here makes the spec and the formal language more complex. However, from the point of view of the programmer, coping with a system that silently deviates from programmer expectation is way more complex than coping with a syntax error.

I also agree that this is a dangerous argument if overused. The key here is that even sophisticated programmers, knowing all the rest of the language well, will arrive at different expectations. If one of these were clearly dominant over the other, once explained, I would probably advocate that. But if neither dominates, better to stop the lurking surprise before runtime.

I should also say that I don't really care about this one specifically. Approximately no one will write eval?.( on purpose. But it is a nice example to reinforce the precedent set by

-2**3

I will certainly not block consensus on this issue. Whatever everyone else can agree on is fine enough with me.

@rkirsling
Copy link
Member Author

rkirsling commented Jul 12, 2020

To me, the crux of -2**3 being a syntax error is not that two people could disagree on how to interpret it, but rather that a single person is led to interpret it differently depending on whitespace. The same argument applies to a || b ?? c .

eval?.()'s two possible interpretations, on the other hand, are reflective of two separate camps:

  • Those favoring full orthogonality of ?. expect that eval?.() is exactly eval() when eval is non-nullish.
  • Those intimately familiar with (0,eval)() and kin may expect that direct eval is nothing other than eval(), that calling eval once it has passed through another operator is necessarily indirect.

We also had two separate camps surrounding how foo?.() itself should be interpreted. This meant either way having to instruct users via MDN and the like, but I doubt many folks would regret choosing one instead of scrapping optional call altogether. And I believe this edge case is far less controversial than that. 🙂

@hax
Copy link
Member

hax commented Jul 12, 2020

I doubt many folks would regret choosing one instead of scrapping optional call altogether.

[off topic] Yeah, I have to say I'm one of them. I've heard a programmer complain that he have to explained it 4 times to the reviewers for foo?.(x), and two times are for the same person 😅 I also have seen a bug which the author want foo?.[key] but accidently write foo?.(key).

Better for linters to have a rule about eval?.()

I may already comment about linters many times: every new linter rule have cost (development, documentation, discussion, adoption, configuration, deployment, etc.) in the whole js ecosystem, which have a much large scale than tc39, so the cost is much bigger than we realize.

@rkirsling
Copy link
Member Author

Closing; we decided to leave the spec as-is in today's meeting.

@hax
Copy link
Member

hax commented Jul 20, 2020

So x |> eval will also be indirect. I feel it will surprise more people than eval?.() case...

@ljharb
Copy link
Member

ljharb commented Jul 20, 2020

i would not expect x |> eval() to be indirect, since that's sugar for the eval() appearing inside a function body. That should be discussed on the pipeline proposal.

@hax
Copy link
Member

hax commented Jul 20, 2020

It seems the meeting notes shows some delegates object direct eval because of x |> eval? Maybe I misunderstand.

since that's sugar for the eval()

But eval?.() could also be seen as sugar for eval != null ? eval() : undefined ...

@leobalter
Copy link
Member

@hax the objection is strictly to eval?.(x) but there is room for discussions wrt pipeline operator.

@ljharb
Copy link
Member

ljharb commented Jul 20, 2020

I would classify it as, making this one direct would force the pipeline case, as well as unknowable future cases, to all be direct as well; leaving this indirect still leaves the design space open.

@hax
Copy link
Member

hax commented Jul 20, 2020

In my opinion, we are just adding one and one ad-hoc rules which hard to infer and remember.

@kmiller68
Copy link
Contributor

In my opinion, we are just adding one and one ad-hoc rules which hard to infer and remember.

I kinda agree. I had assumed from the meeting that we had settled this for all the new operators but it sounds like that's not the case?

But eval?.() could also be seen as sugar for eval != null ? eval() : undefined ...

That's not quite a general rule, if you replace eval with (eval = bar()) you don't evaluate bar() twice. That said, I doubt most people think of that nuance...

@jridgewell
Copy link
Member

I had assumed from the meeting that we had settled this for all the new operators but it sounds like that's not the case?

That was my understanding. This sets precedence that new call syntaxes that invoke eval will be indirect evals. Only the literal source-text eval(…) can produce a direct eval.

@ljharb
Copy link
Member

ljharb commented Jul 21, 2020

My mistake, that seems fine to me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.