Commit 67c4b089 authored by Erik Luo's avatar Erik Luo Committed by Commit Bot

Revert "DevTools: use autocomplete's possibleSideEffect to bypass throwOnSideEffect"

This reverts commit fcaafdd7.

Reason for revert: we'd like to be more strict about side effects

Original change's description:
> DevTools: use autocomplete's possibleSideEffect to bypass throwOnSideEffect
> 
> Allow Console to evaluate without throwOnSideEffect when the entire
> expression meets the same strict conditions used by JSAutocomplete.
> 
> Bug: 810176
> Change-Id: If5c80250cd75d81eec1e495d0414903213fac930
> Reviewed-on: https://chromium-review.googlesource.com/1028934
> Commit-Queue: Erik Luo <luoe@chromium.org>
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Reviewed-by: Joel Einbinder <einbinder@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555142}

TBR=dgozman@chromium.org,einbinder@chromium.org,luoe@chromium.org

Change-Id: Iec31996445e98945a0a9a9b67e572d17bafdcbeb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 810176
Reviewed-on: https://chromium-review.googlesource.com/1039063Reviewed-by: default avatarErik Luo <luoe@chromium.org>
Commit-Queue: Erik Luo <luoe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555269}
parent dedd5ba3
Test side effect check requirement tester.
Expression "var x"
PASS: needsSideEffectCheck: true, expected: true
Expression "import x"
PASS: needsSideEffectCheck: true, expected: true
Expression "new x()"
PASS: needsSideEffectCheck: true, expected: true
Expression "for (;;) {}"
PASS: needsSideEffectCheck: true, expected: true
Expression "while () {}"
PASS: needsSideEffectCheck: true, expected: true
Expression "123"
PASS: needsSideEffectCheck: false, expected: false
Expression "1+2"
PASS: needsSideEffectCheck: false, expected: false
Expression "x++"
PASS: needsSideEffectCheck: true, expected: true
Expression "foo+name"
PASS: needsSideEffectCheck: false, expected: false
Expression "foo()+name"
PASS: needsSideEffectCheck: true, expected: true
Expression "foo();name"
PASS: needsSideEffectCheck: true, expected: true
Expression "name"
PASS: needsSideEffectCheck: false, expected: false
Expression "name[0]"
PASS: needsSideEffectCheck: false, expected: false
Expression "name[1+2]"
PASS: needsSideEffectCheck: false, expected: false
Expression "name.foo.bar"
PASS: needsSideEffectCheck: false, expected: false
Expression "name.foo[0].bar"
PASS: needsSideEffectCheck: false, expected: false
Expression "name.foo().bar"
PASS: needsSideEffectCheck: true, expected: true
Expression "syntax_error,,"
PASS: needsSideEffectCheck: true, expected: true
(async function() {
await TestRunner.loadModule('formatter');
await TestRunner.loadModule('object_ui');
TestRunner.addResult('Test side effect check requirement tester.');
await check(`var x`, true);
await check(`import x`, true);
await check(`new x()`, true);
await check(`for (;;) {}`, true);
await check(`while () {}`, true);
await check(`123`, false);
await check(`1+2`, false);
await check(`x++`, true);
await check(`foo+name`, false);
await check(`foo()+name`, true);
await check(`foo();name`, true);
await check(`name`, false);
await check(`name[0]`, false);
await check(`name[1+2]`, false);
await check(`name.foo.bar`, false);
await check(`name.foo[0].bar`, false);
await check(`name.foo().bar`, true);
await check(`syntax_error,,`, true);
TestRunner.completeTest();
async function check(text, expected) {
const actual = (await Formatter.formatterWorkerPool().hasPossibleSideEffects(text));
const differ = actual !== expected;
TestRunner.addResult(`Expression "${text}"\n${differ ? 'FAIL: ' : 'PASS: '}needsSideEffectCheck: ${actual}, expected: ${expected}`);
}
})();
...@@ -81,14 +81,11 @@ Console.ConsolePrompt = class extends UI.Widget { ...@@ -81,14 +81,11 @@ Console.ConsolePrompt = class extends UI.Widget {
return; return;
} }
// Checking for possible side effects uses the same logic as JavaScriptAutocomplete.
// This grows the set of previewable expressions without whitelisting.
const hasPossibleSideEffects = await Formatter.formatterWorkerPool().hasPossibleSideEffects(text);
const options = { const options = {
expression: SDK.RuntimeModel.wrapObjectLiteralExpressionIfNeeded(text), expression: SDK.RuntimeModel.wrapObjectLiteralExpressionIfNeeded(text),
includeCommandLineAPI: true, includeCommandLineAPI: true,
generatePreview: true, generatePreview: true,
throwOnSideEffect: hasPossibleSideEffects, throwOnSideEffect: true,
timeout: 500 timeout: 500
}; };
const result = await executionContext.evaluate(options, true /* userGesture */, false /* awaitPromise */); const result = await executionContext.evaluate(options, true /* userGesture */, false /* awaitPromise */);
......
...@@ -49,7 +49,7 @@ Formatter.FormatterWorkerPool = class { ...@@ -49,7 +49,7 @@ Formatter.FormatterWorkerPool = class {
this._workerTasks.set(worker, null); this._workerTasks.set(worker, null);
this._processNextTask(); this._processNextTask();
task.callback(event.data); task.callback(event.data ? event.data : null);
} }
/** /**
...@@ -239,14 +239,6 @@ Formatter.FormatterWorkerPool = class { ...@@ -239,14 +239,6 @@ Formatter.FormatterWorkerPool = class {
return /** @type {!Promise<?{baseExpression: string, possibleSideEffects:boolean}>} */ ( return /** @type {!Promise<?{baseExpression: string, possibleSideEffects:boolean}>} */ (
this._runTask('findLastExpression', {content})); this._runTask('findLastExpression', {content}));
} }
/**
* @param {string} content
* @return {!Promise<boolean>}
*/
hasPossibleSideEffects(content) {
return /** @type {!Promise<boolean>} */ (this._runTask('hasPossibleSideEffects', {content}));
}
}; };
Formatter.FormatterWorkerPool.MaxWorkers = 2; Formatter.FormatterWorkerPool.MaxWorkers = 2;
......
...@@ -87,9 +87,6 @@ self.onmessage = function(event) { ...@@ -87,9 +87,6 @@ self.onmessage = function(event) {
case 'findLastExpression': case 'findLastExpression':
postMessage(FormatterWorker.findLastExpression(params.content)); postMessage(FormatterWorker.findLastExpression(params.content));
break; break;
case 'hasPossibleSideEffects':
postMessage(FormatterWorker.hasPossibleSideEffects(params.content));
break;
default: default:
console.error('Unsupport method name: ' + method); console.error('Unsupport method name: ' + method);
} }
...@@ -374,18 +371,9 @@ FormatterWorker.findLastExpression = function(content) { ...@@ -374,18 +371,9 @@ FormatterWorker.findLastExpression = function(content) {
let baseExpression = parsedContent.substring(baseNode.start, parsedContent.length - suffix.length); let baseExpression = parsedContent.substring(baseNode.start, parsedContent.length - suffix.length);
if (baseExpression.startsWith('{')) if (baseExpression.startsWith('{'))
baseExpression = `(${baseExpression})`; baseExpression = `(${baseExpression})`;
const possibleSideEffects = FormatterWorker._possibleSideEffects(/** @type {!ESTree.Node} */ (baseNode));
return {baseExpression, possibleSideEffects};
};
/**
* @param {!ESTree.Node} node
* @return {boolean}
*/
FormatterWorker._possibleSideEffects = function(node) {
const sideEffectFreeTypes = new Set([ const sideEffectFreeTypes = new Set([
'MemberExpression', 'Identifier', 'BinaryExpression', 'Literal', 'TemplateLiteral', 'TemplateElement', 'MemberExpression', 'Identifier', 'BinaryExpression', 'Literal', 'TemplateLiteral', 'TemplateElement',
'ObjectExpression', 'ArrayExpression', 'Property', 'ThisExpression', 'Program', 'ExpressionStatement' 'ObjectExpression', 'ArrayExpression', 'Property', 'ThisExpression'
]); ]);
let possibleSideEffects = false; let possibleSideEffects = false;
const sideEffectwalker = new FormatterWorker.ESTreeWalker(node => { const sideEffectwalker = new FormatterWorker.ESTreeWalker(node => {
...@@ -394,25 +382,8 @@ FormatterWorker._possibleSideEffects = function(node) { ...@@ -394,25 +382,8 @@ FormatterWorker._possibleSideEffects = function(node) {
if (possibleSideEffects) if (possibleSideEffects)
return FormatterWorker.ESTreeWalker.SkipSubtree; return FormatterWorker.ESTreeWalker.SkipSubtree;
}); });
sideEffectwalker.walk(node); sideEffectwalker.walk(/** @type {!ESTree.Node} */ (baseNode));
return possibleSideEffects; return {baseExpression, possibleSideEffects};
};
/**
* @param {string} text
* @return {boolean}
*/
FormatterWorker.hasPossibleSideEffects = function(text) {
if (text.length > 10000)
return true;
let ast = null;
try {
ast = acorn.parse(text, {ecmaVersion: 9});
} catch (e) {
}
if (!ast)
return true;
return FormatterWorker._possibleSideEffects(ast);
}; };
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment