From 9cdce03e16645775a6dbfb566604a3cc9360f313 Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Wed, 16 Jan 2019 23:59:28 +0000 Subject: [PATCH] [dart2js] Improve null receiver guard removal near JS code Null receiver guards (which look like "t1.toString;") are removed in more cases where the following JS fragment would throw a TypeError on the same value. Change-Id: I3872f00c90432077199542f4485b8e991f82fa21 Reviewed-on: https://dart-review.googlesource.com/c/89765 Commit-Queue: Stephen Adams Reviewed-by: Sigmund Cherem --- pkg/compiler/lib/src/native/behavior.dart | 105 ++++++++++-------- pkg/compiler/lib/src/native/js.dart | 58 ++++++---- pkg/compiler/lib/src/ssa/optimize.dart | 45 ++------ .../dart2js/js/js_spec_string_test.dart | 7 +- .../dart2js/js/js_throw_behavior_test.dart | 30 ++++- 5 files changed, 136 insertions(+), 109 deletions(-) diff --git a/pkg/compiler/lib/src/native/behavior.dart b/pkg/compiler/lib/src/native/behavior.dart index 61900e46f89..07e03da2ac0 100644 --- a/pkg/compiler/lib/src/native/behavior.dart +++ b/pkg/compiler/lib/src/native/behavior.dart @@ -38,15 +38,15 @@ class SpecialType { } /// Description of the exception behaviour of native code. -/// -/// TODO(sra): Replace with something that better supports specialization on -/// first argument properties. class NativeThrowBehavior { - static const NativeThrowBehavior NEVER = const NativeThrowBehavior._(0); - static const NativeThrowBehavior MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS = - const NativeThrowBehavior._(1); - static const NativeThrowBehavior MAY = const NativeThrowBehavior._(2); - static const NativeThrowBehavior MUST = const NativeThrowBehavior._(3); + static const NativeThrowBehavior NEVER = NativeThrowBehavior._(0); + static const NativeThrowBehavior MAY = NativeThrowBehavior._(1); + + /// Throws only if first argument is null. + static const NativeThrowBehavior NULL_NSM = NativeThrowBehavior._(2); + + /// Throws if first argument is null, then may throw. + static const NativeThrowBehavior NULL_NSM_THEN_MAY = NativeThrowBehavior._(3); final int _bits; const NativeThrowBehavior._(this._bits); @@ -55,43 +55,73 @@ class NativeThrowBehavior { /// Does this behavior always throw a noSuchMethod check on a null first /// argument before any side effect or other exception? - // TODO(sra): Extend NativeThrowBehavior with the concept of NSM guard - // followed by other potential behavior. - bool get isNullNSMGuard => this == MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS; + bool get isNullNSMGuard => this == NULL_NSM || this == NULL_NSM_THEN_MAY; /// Does this behavior always act as a null noSuchMethod check, and has no /// other throwing behavior? - bool get isOnlyNullNSMGuard => - this == MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS; + bool get isOnlyNullNSMGuard => this == NULL_NSM; /// Returns the behavior if we assume the first argument is not null. NativeThrowBehavior get onNonNull { - if (this == MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS) return NEVER; + if (this == NULL_NSM) return NEVER; + if (this == NULL_NSM_THEN_MAY) return MAY; return this; } String toString() { if (this == NEVER) return 'never'; if (this == MAY) return 'may'; - if (this == MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS) return 'null(1)'; - if (this == MUST) return 'must'; + if (this == NULL_NSM) return 'null(1)'; + if (this == NULL_NSM_THEN_MAY) return 'null(1)+may'; return 'NativeThrowBehavior($_bits)'; } /// Canonical list of marker values. /// /// Added to make [NativeThrowBehavior] enum-like. - static const List values = const [ + static const List values = [ NEVER, - MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS, MAY, - MUST, + NULL_NSM, + NULL_NSM_THEN_MAY, ]; /// Index to this marker within [values]. /// /// Added to make [NativeThrowBehavior] enum-like. int get index => values.indexOf(this); + + /// Deserializer helper. + static NativeThrowBehavior _bitsToValue(int bits) { + switch (bits) { + case 0: + return NEVER; + case 1: + return MAY; + case 2: + return NULL_NSM; + case 3: + return NULL_NSM_THEN_MAY; + default: + return null; + } + } + + /// Sequence operator. + NativeThrowBehavior then(NativeThrowBehavior second) { + if (this == NEVER) return second; + if (this == MAY) return MAY; + if (this == NULL_NSM_THEN_MAY) return NULL_NSM_THEN_MAY; + assert(this == NULL_NSM); + if (second == NEVER) return this; + return NULL_NSM_THEN_MAY; + } + + /// Choice operator. + NativeThrowBehavior or(NativeThrowBehavior other) { + if (this == other) return this; + return MAY; + } } /// A summary of the behavior of a native element. @@ -190,21 +220,8 @@ class NativeBehavior { behavior.codeTemplateText = codeTemplateText; behavior.codeTemplate = js.js.parseForeignJS(codeTemplateText); } - switch (throwBehavior) { - case 0: - behavior.throwBehavior = NativeThrowBehavior.NEVER; - break; - case 1: - behavior.throwBehavior = - NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS; - break; - case 2: - behavior.throwBehavior = NativeThrowBehavior.MAY; - break; - case 3: - behavior.throwBehavior = NativeThrowBehavior.MUST; - break; - } + behavior.throwBehavior = NativeThrowBehavior._bitsToValue(throwBehavior); + assert(behavior.throwBehavior._bits == throwBehavior); behavior.isAllocation = isAllocation; behavior.useGvn = useGvn; return behavior; @@ -319,12 +336,12 @@ class NativeBehavior { /// indicated with 'no-static'. The flags 'effects' and 'depends' must be /// used in unison (either both are present or none is). /// - /// The values are 'never', 'may', 'must', and 'null(1)'. - /// The default if unspecified is 'may'. 'null(1)' means that the template - /// expression throws if and only if the first template parameter is `null` - /// or `undefined`. - /// TODO(sra): Can we simplify to must/may/never and add null(1) by - /// inspection as an orthogonal attribute? + /// The values are 'never', 'may', 'null(1)', and + /// 'null(1)+may'. The default if unspecified is 'may'. 'null(1)' means + /// that the template expression throws if and only if the first template + /// parameter is `null` or `undefined`, and 'null(1)+may' throws if the + /// first argument is `null` / `undefined`, and then may throw for other + /// reasons. /// /// values are 'true' and 'false'. The default if unspecified /// is 'false'. @@ -478,14 +495,14 @@ class NativeBehavior { }); } - const throwsOption = const { + const throwsOption = { 'never': NativeThrowBehavior.NEVER, - 'null(1)': NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS, 'may': NativeThrowBehavior.MAY, - 'must': NativeThrowBehavior.MUST + 'null(1)': NativeThrowBehavior.NULL_NSM, + 'null(1)+may': NativeThrowBehavior.NULL_NSM_THEN_MAY, }; - const boolOptions = const {'true': true, 'false': false}; + const boolOptions = {'true': true, 'false': false}; SideEffects sideEffects = processEffects(reportError, values['effects'], values['depends']); diff --git a/pkg/compiler/lib/src/native/js.dart b/pkg/compiler/lib/src/native/js.dart index 1b065a13e37..0c8319756bf 100644 --- a/pkg/compiler/lib/src/native/js.dart +++ b/pkg/compiler/lib/src/native/js.dart @@ -131,26 +131,18 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { return visit(node); } - // TODO(sra): Add [sequence] functionality to NativeThrowBehavior. /// Returns the combined behavior of sequential execution of code having /// behavior [first] followed by code having behavior [second]. static NativeThrowBehavior sequence( NativeThrowBehavior first, NativeThrowBehavior second) { - if (first == NativeThrowBehavior.MUST) return first; - if (second == NativeThrowBehavior.MUST) return second; - if (second == NativeThrowBehavior.NEVER) return first; - if (first == NativeThrowBehavior.NEVER) return second; - // Both are one of MAY or MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS. - return NativeThrowBehavior.MAY; + return first.then(second); } - // TODO(sra): Add [choice] functionality to NativeThrowBehavior. /// Returns the combined behavior of a choice between two paths with behaviors /// [first] and [second]. static NativeThrowBehavior choice( NativeThrowBehavior first, NativeThrowBehavior second) { - if (first == second) return first; // Both paths have same behaviour. - return NativeThrowBehavior.MAY; + return first.or(second); } NativeThrowBehavior visit(js.Node node) { @@ -173,12 +165,16 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { return NativeThrowBehavior.NEVER; } + NativeThrowBehavior visitArrayInitializer(js.ArrayInitializer node) { + return node.elements.map(visit).fold(NativeThrowBehavior.NEVER, sequence); + } + + NativeThrowBehavior visitArrayHole(js.ArrayHole node) { + return NativeThrowBehavior.NEVER; + } + NativeThrowBehavior visitObjectInitializer(js.ObjectInitializer node) { - NativeThrowBehavior result = NativeThrowBehavior.NEVER; - for (js.Property property in node.properties) { - result = sequence(result, visit(property)); - } - return result; + return node.properties.map(visit).fold(NativeThrowBehavior.NEVER, sequence); } NativeThrowBehavior visitProperty(js.Property node) { @@ -191,6 +187,17 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { } NativeThrowBehavior visitCall(js.Call node) { + js.Expression target = node.target; + if (target is js.PropertyAccess && _isFirstInterpolatedProperty(target)) { + // #.f(...): Evaluate selector 'f', dereference, evaluate arguments, and + // finally call target. + NativeThrowBehavior result = + sequence(visit(target.selector), NativeThrowBehavior.NULL_NSM); + for (js.Expression argument in node.arguments) { + result = sequence(result, visit(argument)); + } + return sequence(result, NativeThrowBehavior.MAY); // Target may throw. + } return NativeThrowBehavior.MAY; } @@ -242,7 +249,7 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { } NativeThrowBehavior visitThrow(js.Throw node) { - return NativeThrowBehavior.MUST; + return sequence(visit(node.expression), NativeThrowBehavior.MAY); } NativeThrowBehavior visitPrefix(js.Prefix node) { @@ -269,6 +276,7 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { // global names are almost certainly not reference errors, e.g 'Array'. switch (node.name) { case 'Array': + case 'Math': case 'Object': return NativeThrowBehavior.NEVER; default: @@ -277,20 +285,26 @@ class ThrowBehaviorVisitor extends js.BaseVisitor { } NativeThrowBehavior visitAccess(js.PropertyAccess node) { - // TODO(sra): We need a representation where the nsm guard behaviour is - // maintained when combined with other throwing behaviour. js.Node receiver = node.receiver; NativeThrowBehavior first = visit(receiver); NativeThrowBehavior second = visit(node.selector); - if (receiver is js.InterpolatedExpression && - receiver.isPositional && - receiver.nameOrPosition == 0) { - first = NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS; + if (_isFirstInterpolatedProperty(node)) { + first = NativeThrowBehavior.NULL_NSM; } else { first = NativeThrowBehavior.MAY; } return sequence(first, second); } + + bool _isFirstInterpolatedProperty(js.PropertyAccess node) { + js.Node receiver = node.receiver; + if (receiver is js.InterpolatedExpression && + receiver.isPositional && + receiver.nameOrPosition == 0) { + return true; + } + return false; + } } diff --git a/pkg/compiler/lib/src/ssa/optimize.dart b/pkg/compiler/lib/src/ssa/optimize.dart index 40917cf5abc..d63181efe5d 100644 --- a/pkg/compiler/lib/src/ssa/optimize.dart +++ b/pkg/compiler/lib/src/ssa/optimize.dart @@ -14,7 +14,7 @@ import '../elements/entities.dart'; import '../elements/types.dart'; import '../inferrer/abstract_value_domain.dart'; import '../inferrer/types.dart'; -import '../js/js.dart' as js; +//import '../js/js.dart' as js; import '../js_backend/allocator_analysis.dart' show JAllocatorAnalysis; import '../js_backend/backend.dart'; import '../js_backend/native_data.dart' show NativeData; @@ -1846,31 +1846,6 @@ class SsaDeadCodeEliminator extends HGraphVisitor implements OptimizationPhase { if (foreign.inputs.length < 1) return false; if (foreign.inputs.first != receiver) return false; if (foreign.throwBehavior.isNullNSMGuard) return true; - - // TODO(sra): Fix NativeThrowBehavior to distinguish MAY from - // throws-nsm-on-null-followed-by-MAY and remove all the code below. - - // We look for a template of the form - // - // #.something -or- #.something() - // - // where # is substituted by receiver. - js.Template template = foreign.codeTemplate; - js.Node node = template.ast; - // #.something = ... - if (node is js.Assignment) { - js.Assignment assignment = node; - node = assignment.leftHandSide; - } - - // #.something - if (node is js.PropertyAccess) { - js.PropertyAccess access = node; - if (access.receiver is js.InterpolatedExpression) { - js.InterpolatedExpression hole = access.receiver; - return hole.isPositional && hole.nameOrPosition == 0; - } - } return false; } @@ -1949,14 +1924,18 @@ class SsaDeadCodeEliminator extends HGraphVisitor implements OptimizationPhase { if (!instruction.usedBy.isEmpty) return false; if (isTrivialDeadStore(instruction)) return true; if (instruction.sideEffects.hasSideEffects()) return false; - if (instruction.canThrow(_abstractValueDomain) && - instruction.onlyThrowsNSM() && - hasFollowingThrowingNSM(instruction)) { - return true; + if (instruction.canThrow(_abstractValueDomain)) { + if (instruction.onlyThrowsNSM() && hasFollowingThrowingNSM(instruction)) { + // [instruction] is a null reciever guard that is followed by an + // instruction that fails the same way (by accessing a property of + // `null` or `undefined`). + return true; + } + return false; } - return !instruction.canThrow(_abstractValueDomain) && - instruction is! HParameterValue && - instruction is! HLocalSet; + if (instruction is HParameterValue) return false; + if (instruction is HLocalSet) return false; + return true; } void visitGraph(HGraph graph) { diff --git a/tests/compiler/dart2js/js/js_spec_string_test.dart b/tests/compiler/dart2js/js/js_spec_string_test.dart index 1e7080d8fa8..0dc8344526f 100644 --- a/tests/compiler/dart2js/js/js_spec_string_test.dart +++ b/tests/compiler/dart2js/js/js_spec_string_test.dart @@ -270,12 +270,11 @@ void main() { testWithSideEffects(' returns:A|B|C; creates:A; ', returns: ['A', 'B', 'C'], creates: ['A']); - test('throws:must', expectedThrows: NativeThrowBehavior.MUST); test('throws:may', expectedThrows: NativeThrowBehavior.MAY); test('throws:never', expectedThrows: NativeThrowBehavior.NEVER); - test('throws:null(1)', - expectedThrows: - NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS); + test('throws:null(1)', expectedThrows: NativeThrowBehavior.NULL_NSM); + test('throws:null(1)+may', + expectedThrows: NativeThrowBehavior.NULL_NSM_THEN_MAY); test('new:true', expectedNew: true); test('new:false', expectedNew: false); diff --git a/tests/compiler/dart2js/js/js_throw_behavior_test.dart b/tests/compiler/dart2js/js/js_throw_behavior_test.dart index 22fb73388ba..0c3ecea5288 100644 --- a/tests/compiler/dart2js/js/js_throw_behavior_test.dart +++ b/tests/compiler/dart2js/js/js_throw_behavior_test.dart @@ -16,9 +16,9 @@ void test(String source, NativeThrowBehavior expectedThrowBehavior) { void main() { final MAY = NativeThrowBehavior.MAY; - final MUST = NativeThrowBehavior.MUST; final NEVER = NativeThrowBehavior.NEVER; - final NULL_NSM = NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS; + final NULL_NSM = NativeThrowBehavior.NULL_NSM; + final NULL_NSM_THEN_MAY = NativeThrowBehavior.NULL_NSM_THEN_MAY; test('0', NEVER); test('void 0', NEVER); @@ -86,6 +86,7 @@ void main() { test('console', MAY); test('Array', NEVER); + test('Math', NEVER); test('Object', NEVER); test('typeof #', NEVER); @@ -93,8 +94,25 @@ void main() { test('typeof foo.#', MAY); test('typeof #.foo', NULL_NSM); - test('throw 123', MUST); - test('throw #', MUST); - test('throw #.x', MUST); // Could be better: is also an NSM guard. - test('throw #.x = 123', MUST); + test('throw 123', MAY); + test('throw #', MAY); + test('throw #.x', NULL_NSM_THEN_MAY); + test('throw #.x = 123', MAY); + + test('#.f()', NULL_NSM_THEN_MAY); + test('#.f(#, #)', NULL_NSM_THEN_MAY); + test('#[#](#, #)', NULL_NSM_THEN_MAY); + test('#[f()](#, #)', MAY); // f() evaluated before + + test('[]', NEVER); + test('[,,6,,,]', NEVER); + test('[#.f()]', NULL_NSM_THEN_MAY); + test('[,,#.f(),,f(),,]', NULL_NSM_THEN_MAY); + test('[,,f(),,#.f(),,]', MAY); + + test('{}', NEVER); + test('{one: 1}', NEVER); + test('{one: #.f()}', NULL_NSM_THEN_MAY); + test('{one: #.f(), two: f()}', NULL_NSM_THEN_MAY); + test('{one: f(), two: #.f()}', MAY); }