-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Special cases of new.target #1377
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
Conversation
|
@Microsoft/chakra-es, @farzonl, could you take look pls? |
lib/Parser/Parse.cpp
Outdated
| if ((this->m_grfscr & fscrEvalCode) != 0 && (this->m_grfscr & fscrIndirect) == 0) | ||
| { | ||
| Js::JavascriptFunction * caller = nullptr; | ||
| if (Js::JavascriptStackWalker::GetNonLamdaCaller(&caller, m_scriptContext)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fix the spelling of this API while we're here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we walking the call stack looking for the nearest non-lambda caller here? Or am I misreading the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Thanks.
-- Paul
From: suwc [mailto:[email protected]]
Sent: Wednesday, August 3, 2016 11:43 AM
To: Microsoft/ChakraCore [email protected]
Cc: Paul Leathers [email protected]; Team mention [email protected]
Subject: Re: [Microsoft/ChakraCore] Special cases of new.target (#1377)
In lib/Parser/Parse.cpphttps://github.com//pull/1377#discussion_r73395674:
}-}
- if ((this->m_grfscr & fscrEvalCode) != 0 && (this->m_grfscr & fscrIndirect) == 0)
- {
Js::JavascriptFunction * caller = nullptr;if (Js::JavascriptStackWalker::GetNonLamdaCaller(&caller, m_scriptContext))
Lamda => Lambda?
Will do.
In reply to: 73384891#1377 (comment)
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1377/files/f89a104fd72f56eba5a59d03df8279f89162b642#r73395674, or mute the threadhttps://github.com/notifications/unsubscribe-auth/APF8ROruobVPRSzvWs2YSV62ni9DvM2sks5qcOExgaJpZM4Jb24l.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, walking to find the nearest non-lambda caller to see if it's global function (SyntaxError) or else (okay).
In reply to: 73385747 [](ancestors = 73385747)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. That's not actually what we want. We want to find the JS caller, whatever it is, and if it's a lambda we want to look at the non-lambda function (if any) that encloses it lexically. So consider this case:
let l = () => { eval('new.target'); }
(function() { return l(); })();
And, conversely, this one:
var f = (function() { return () => { eval('new.target') } })();
f();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/es6/ES6NewTarget.js
Outdated
| }, | ||
| /* | ||
|
|
||
| // Blocked by 'ReferenceError: '<_lexicalNewTargetSymbol>' is undefined' bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the comment if this isn't blocked anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@pleath @boingoing @akroshg @Microsoft/chakra-es |
|
👍 |
|
LGTM |
Fix some special cases of 'new.target' reference, e.g.: - 'new.target' inside direct eval() in global function throws SyntaxError - 'new.target' inside indirect eval() throws SyntaxError - 'new.target' inside function evaluates to undefined for function call - 'new.target' inside function evaluates to said function for constructor call - lambda+eval in global function called in a function (SyntaxError) - lambda+eval in a function called by global function (okay) - other nested/compounded cases with eval(), arrow functions, and functions
Merge pull request #1377 from suwc:build/suwc/buddy Fix some special cases of 'new.target' reference, e.g.: - 'new.target' inside direct eval() in global function throws SyntaxError - 'new.target' inside indirect eval() throws SyntaxError - 'new.target' inside function evaluates to undefined for function call - 'new.target' inside function evaluates to said function for constructor call - lambda+eval in global function called in a function (SyntaxError) - lambda+eval in a function called by global function (okay) - other nested/compounded cases with eval(), arrow functions, and functions
Fix some special cases of 'new.target' reference, e.g.: