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

fix: ensure strict mode when evaluating in JIT #30122

Closed
wants to merge 2 commits into from

Conversation

@benlesh
Copy link
Contributor

commented Apr 25, 2019

To help prevent regressions that could be caused by poor compiler behavior, this fix ensures that all code executed by the JIT evaluator is in strict mode.

@googlebot googlebot added the cla: yes label Apr 25, 2019

@ngbot ngbot bot added this to the needsTriage milestone Apr 25, 2019

@benlesh benlesh force-pushed the benlesh:jit_use_strict branch from 9e92c12 to 8901bc3 Apr 25, 2019

@benlesh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@benlesh benlesh marked this pull request as ready for review Apr 25, 2019

@benlesh benlesh requested a review from angular/fw-compiler as a code owner Apr 25, 2019

@benlesh benlesh force-pushed the benlesh:jit_use_strict branch from 8901bc3 to 65f9fdd Apr 25, 2019

@benlesh benlesh requested a review from mhevery Apr 25, 2019

@benlesh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@petebacondarwin To your knowledge, the current approach here should keep source maps correct, yeah?

@benlesh benlesh requested review from petebacondarwin and alxhub Apr 25, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I believe sourcemaps will be fine.

@benlesh benlesh force-pushed the benlesh:jit_use_strict branch from e2225ca to 03187a2 Apr 26, 2019

@@ -31,6 +31,13 @@ export class JitEvaluator {
createSourceMaps: boolean): {[key: string]: any} {
const converter = new JitEmitterVisitor(reflector);
const ctx = EmitterVisitorContext.createRoot();
// Ensure generated code is in strict mode
if (statements.length > 0 && !isUseStrictStatement(statements[0])) {

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Apr 26, 2019

Contributor

@benlesh is it possible that the first statement is a comment node or we strip them out and they do not show up at this stage? May be add a test to verify this?

This comment has been minimized.

Copy link
@benlesh

benlesh Apr 26, 2019

Author Contributor

@AndrewKushnir I'm not sure. I know it's not a party foul if we accentally add two "use strict"; statements. I guess, ideally we just don't generate code like that, and this if block could even be removed.

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Apr 26, 2019

Contributor

@benlesh can we add a test to check that? If it's possible to have comment nodes in statements at this stage, the check might need to be updated to skip through the leading comments and check first non-comment statement.

This comment has been minimized.

Copy link
@benlesh

benlesh Apr 26, 2019

Author Contributor

Really the only way to be sure, is to strip all "use strict"; statements from statements, then add one at the top. It really seems like overkill, though. Perhaps we should just remove this check entirely and let duplicates happen? Ideally we catch these things in other tests.

@mhevery

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

MERGE_ASSISTANCE: @alxhub is on vacation, so my approval should be global

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@benlesh do you have a presubmit for this PR? I think we may need to run a global presubmit, since this change touches the code that is used for both VE and Ivy, so it'd be better to check all targets in case these targets contain some code that may start to throw after enforcing strict mode.

@benlesh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Comments are leading trivia and don't count as statements.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

As discussed offline, due to the relatively high-risk of these changes, we'll merge then the week of May 6th (after ng-conf), adding "blocked" label for now. Thank you.

@benlesh benlesh force-pushed the benlesh:jit_use_strict branch from 03187a2 to 372828e May 8, 2019

@alxhub alxhub closed this in 452f121 May 8, 2019

alxhub added a commit that referenced this pull request May 8, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

@nilsmehlhorn nilsmehlhorn referenced this pull request Jun 17, 2019

Merged

Enable strict-mode #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.