fix: ensure strict mode when evaluating in JIT #30122
Conversation
benlesh
added
area: core
benlesh
added
the
action: review
|
@petebacondarwin To your knowledge, the current approach here should keep source maps correct, yeah? |
|
I believe sourcemaps will be fine. |
IgorMinar
added
target: patch
| @@ -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])) { | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
MERGE_ASSISTANCE: @alxhub is on vacation, so my approval should be global |
mhevery
added
the
merge: caretaker note
|
@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. |
|
Comments are leading trivia and don't count as statements. |
AndrewKushnir
added
target: major
|
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
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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.