[dart2js] js_ast.Name does not need to be Comparable
- js_ast.Name is no longer Comparable
- _NameReference is used on demand rather than for every occurrence.
This saves 50MB in the big link scenario.
- remove unused asVariableUse() method
Change-Id: I37f55044d394b7e047ca88c527641471bc94c641
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198981
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Joshua Litt <joshualitt@google.com>
diff --git a/pkg/compiler/lib/src/common/codegen.dart b/pkg/compiler/lib/src/common/codegen.dart
index 9ea7506..ca116b4 100644
--- a/pkg/compiler/lib/src/common/codegen.dart
+++ b/pkg/compiler/lib/src/common/codegen.dart
@@ -766,12 +766,6 @@
}
@override
- int compareTo(js.Name other) {
- assert(_value != null, 'value not set for $this');
- return _value.compareTo(other);
- }
-
- @override
Iterable<js.Node> get containedNodes {
return _value != null ? [_value] : const [];
}
diff --git a/pkg/compiler/lib/src/js_backend/namer.dart b/pkg/compiler/lib/src/js_backend/namer.dart
index 645d026..ba2dfd1 100644
--- a/pkg/compiler/lib/src/js_backend/namer.dart
+++ b/pkg/compiler/lib/src/js_backend/namer.dart
@@ -559,14 +559,6 @@
entity.rootOfScope, () => new NamingScope());
}
- /// Return a reference to the given [name].
- ///
- /// This is used to ensure that every use site of a name has a unique node so
- /// that we can properly attribute source information.
- jsAst.Name _newReference(jsAst.Name name) {
- return new _NameReference(name);
- }
-
/// Disambiguated name for [constant].
///
/// Unique within the global-member namespace.
@@ -581,7 +573,7 @@
result = getFreshName(_constantScope, longName);
_constantNames[constant] = result;
}
- return _newReference(result);
+ return result;
}
/// Proposed name for [constant].
@@ -946,7 +938,7 @@
newName = getFreshName(globalScope, name);
internalGlobals[name] = newName;
}
- return _newReference(newName);
+ return newName;
}
/// Generates a unique key for [library].
@@ -986,7 +978,7 @@
newName = getFreshName(globalScope, proposedName);
globals[element] = newName;
}
- return _newReference(newName);
+ return newName;
}
/// Returns the disambiguated name for an instance method or field
@@ -1027,7 +1019,7 @@
userInstanceMembers[key] = newName;
userInstanceMembersOriginalName[key] = '$originalName';
}
- return _newReference(newName);
+ return newName;
}
/// Returns the disambiguated name for the instance member identified by
@@ -1051,7 +1043,7 @@
// TODO(sigmund): consider plumbing the original name instead.
userInstanceMembersOriginalName[key] = name;
}
- return _newReference(newName);
+ return newName;
}
/// Forces the public instance member with [originalName] to have the given
@@ -1099,7 +1091,7 @@
internalInstanceMembers[element] = newName;
}
}
- return _newReference(newName);
+ return newName;
}
/// Disambiguated name for the given operator.
@@ -1114,7 +1106,7 @@
newName = getFreshName(instanceScope, operatorIdentifier);
userInstanceOperators[operatorIdentifier] = newName;
}
- return _newReference(newName);
+ return newName;
}
String _generateFreshStringForName(String proposedName, NamingScope scope,
diff --git a/pkg/compiler/lib/src/js_backend/namer_names.dart b/pkg/compiler/lib/src/js_backend/namer_names.dart
index 233f053..fe43140 100644
--- a/pkg/compiler/lib/src/js_backend/namer_names.dart
+++ b/pkg/compiler/lib/src/js_backend/namer_names.dart
@@ -6,19 +6,47 @@
abstract class _NamerName extends jsAst.Name {
int get _kind;
- _NamerName get _target => this;
+
+ @override
+ _NamerName withSourceInformation(
+ jsAst.JavaScriptNodeSourceInformation newSourceInformation) {
+ if (sourceInformation == newSourceInformation) return this;
+ final name = this; // Variable needed for type promotion in next line.
+ final underlying = name is _NameReference ? name._target : this;
+ return _NameReference(underlying, newSourceInformation);
+ }
+
+ int _compareSameKind(covariant _NamerName);
@override
String toString() {
if (DEBUG_MODE) {
return 'Name($key)';
}
- throw new UnsupportedError("Cannot convert a name to a string");
+ throw UnsupportedError("Cannot convert a name to a string");
}
}
enum _NamerNameKinds { StringBacked, Getter, Setter, Async, Compound, Token }
+/// An arbitrary but stable sorting comparison method.
+int compareNames(jsAst.Name aName, jsAst.Name bName) {
+ _NamerName dereference(jsAst.Name name) {
+ if (name is ModularName) return dereference(name.value);
+ if (name is _NameReference) return dereference(name._target);
+ if (name is _NamerName) return name;
+ throw UnsupportedError('Cannot find underlying _NamerName: $name');
+ }
+
+ _NamerName a = dereference(aName);
+ _NamerName b = dereference(bName);
+
+ int aKind = a._kind;
+ int bKind = b._kind;
+ if (bKind != aKind) return bKind - aKind;
+ return a._compareSameKind(b);
+}
+
class StringBackedName extends _NamerName {
@override
final String name;
@@ -31,26 +59,18 @@
String get key => name;
@override
- operator ==(other) {
- if (other is _NameReference) other = other._target;
+ operator ==(Object other) {
+ if (other is _NameReference) return this == other._target;
if (identical(this, other)) return true;
- return (other is StringBackedName) && other.name == name;
+ return other is StringBackedName && name == other.name;
}
@override
int get hashCode => name.hashCode;
@override
- int compareTo(jsAst.Name other) {
- _NamerName otherNamerName;
- if (other is ModularName) {
- otherNamerName = other.value;
- } else {
- otherNamerName = other;
- }
- otherNamerName = otherNamerName._target;
- if (otherNamerName._kind != _kind) return otherNamerName._kind - _kind;
- return name.compareTo(otherNamerName.name);
+ int _compareSameKind(StringBackedName other) {
+ return name.compareTo(other.name);
}
}
@@ -72,11 +92,12 @@
String get key => prefix.key + base.key;
@override
- bool operator ==(other) {
- if (other is _NameReference) other = other._target;
+ bool operator ==(Object other) {
+ if (other is _NameReference) return this == other._target;
if (identical(this, other)) return true;
- if (other is! _PrefixedName) return false;
- return other.base == base && other.prefix == prefix;
+ return other is _PrefixedName &&
+ base == other.base &&
+ prefix == other.prefix;
}
@override
@@ -86,22 +107,10 @@
bool get isFinalized => prefix.isFinalized && base.isFinalized;
@override
- int compareTo(jsAst.Name other) {
- _NamerName otherNamerName;
- if (other is ModularName) {
- otherNamerName = other.value;
- } else {
- otherNamerName = other;
- }
- otherNamerName = otherNamerName._target;
- if (otherNamerName._kind != _kind) return otherNamerName._kind - _kind;
- _PrefixedName otherSameKind = otherNamerName;
- int result = prefix.compareTo(otherSameKind.prefix);
+ int _compareSameKind(_PrefixedName other) {
+ int result = compareNames(prefix, other.prefix);
if (result == 0) {
- result = prefix.compareTo(otherSameKind.prefix);
- if (result == 0) {
- result = base.compareTo(otherSameKind.base);
- }
+ result = compareNames(base, other.base);
}
return result;
}
@@ -150,25 +159,24 @@
@override
String get name {
- if (_cachedName == null) {
- _cachedName = _parts.map((jsAst.Name name) => name.name).join();
- }
- return _cachedName;
+ return _cachedName ??= _parts.map((jsAst.Name name) => name.name).join();
}
@override
String get key => _parts.map((_NamerName name) => name.key).join();
@override
- bool operator ==(other) {
- if (other is _NameReference) other = other._target;
+ bool operator ==(Object other) {
+ if (other is _NameReference) return this == other._target;
if (identical(this, other)) return true;
- if (other is! CompoundName) return false;
- if (other._parts.length != _parts.length) return false;
- for (int i = 0; i < _parts.length; ++i) {
- if (other._parts[i] != _parts[i]) return false;
+ if (other is CompoundName) {
+ if (other._parts.length != _parts.length) return false;
+ for (int i = 0; i < _parts.length; ++i) {
+ if (_parts[i] != other._parts[i]) return false;
+ }
+ return true;
}
- return true;
+ return false;
}
@override
@@ -183,22 +191,10 @@
}
@override
- int compareTo(jsAst.Name other) {
- _NamerName otherNamerName;
- if (other is ModularName) {
- otherNamerName = other.value;
- } else {
- otherNamerName = other;
- }
- otherNamerName = otherNamerName._target;
- if (otherNamerName._kind != _kind) return otherNamerName._kind - _kind;
- CompoundName otherSameKind = otherNamerName;
- if (otherSameKind._parts.length != _parts.length) {
- return otherSameKind._parts.length - _parts.length;
- }
- int result = 0;
+ int _compareSameKind(CompoundName other) {
+ int result = _parts.length.compareTo(other._parts.length);
for (int pos = 0; result == 0 && pos < _parts.length; pos++) {
- result = _parts[pos].compareTo(otherSameKind._parts[pos]);
+ result = compareNames(_parts[pos], other._parts[pos]);
}
return result;
}
@@ -225,19 +221,16 @@
}
@override
- int compareTo(covariant _NamerName other) {
- other = other._target;
- if (other._kind != _kind) return other._kind - _kind;
- TokenName otherToken = other;
- return key.compareTo(otherToken.key);
+ int _compareSameKind(TokenName other) {
+ return key.compareTo(other.key);
}
@override
void markSeen(jsAst.TokenCounter counter) => _rc++;
@override
- bool operator ==(other) {
- if (other is _NameReference) other = other._target;
+ bool operator ==(Object other) {
+ if (other is _NameReference) return this == other._target;
if (identical(this, other)) return true;
return false;
}
@@ -254,9 +247,18 @@
}
}
+/// A [_NameReference] behaves like the underlying (target) [_NamerName] but
+/// carries its own [JavaScriptNodeSourceInformation].
+// TODO(sra): See if this functionality can be moved into js_ast.
class _NameReference extends _NamerName implements jsAst.AstContainer {
+ final _NamerName _target;
+ final jsAst.JavaScriptNodeSourceInformation _sourceInformation;
+
+ _NameReference(this._target, this._sourceInformation);
+
@override
- _NamerName _target;
+ jsAst.JavaScriptNodeSourceInformation get sourceInformation =>
+ _sourceInformation;
@override
int get _kind => _target._kind;
@@ -266,27 +268,19 @@
@override
Iterable<jsAst.Node> get containedNodes => [_target];
- _NameReference(this._target);
-
@override
String get name => _target.name;
@override
- int compareTo(jsAst.Name other) {
- _NamerName otherNamerName;
- if (other is ModularName) {
- otherNamerName = other.value;
- } else {
- otherNamerName = other;
- }
- return _target.compareTo(otherNamerName);
+ int _compareSameKind(_NameReference other) {
+ throw StateError('Should have been dereferenced: $this');
}
@override
bool get isFinalized => _target.isFinalized;
@override
- bool operator ==(other) => _target == other;
+ bool operator ==(Object other) => _target == other;
@override
int get hashCode => _target.hashCode;
diff --git a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
index 5eb2990..26556d7 100644
--- a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
+++ b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
@@ -21,7 +21,7 @@
import '../../js_backend/custom_elements_analysis.dart';
import '../../js_backend/inferred_data.dart';
import '../../js_backend/interceptor_data.dart';
-import '../../js_backend/namer.dart' show Namer, StringBackedName;
+import '../../js_backend/namer.dart' show Namer, StringBackedName, compareNames;
import '../../js_backend/native_data.dart';
import '../../js_backend/runtime_types.dart' show RuntimeTypesChecks;
import '../../js_backend/runtime_types_codegen.dart' show TypeCheck;
@@ -998,7 +998,7 @@
"${interceptorMap[name]}, new ${interceptor}.");
interceptorMap[name] = interceptor;
}
- names.sort();
+ names.sort(compareNames);
return names.map((js.Name name) {
SpecializedGetInterceptor interceptor = interceptorMap[name];
js.Expression code =
@@ -1099,7 +1099,7 @@
"${interceptorMap[name]}, new ${interceptor}.");
interceptorMap[name] = interceptor;
}
- names.sort();
+ names.sort(compareNames);
return names.map((js.Name name) {
OneShotInterceptor interceptor = interceptorMap[name];
js.Expression code =
diff --git a/pkg/compiler/test/codegen/model_data/regress_36222.dart b/pkg/compiler/test/codegen/model_data/regress_36222.dart
index 6a5dbe9..0ed6089 100644
--- a/pkg/compiler/test/codegen/model_data/regress_36222.dart
+++ b/pkg/compiler/test/codegen/model_data/regress_36222.dart
@@ -14,7 +14,12 @@
return x + y;
}
- /*member: A.foo:elided,stubCalls=[foo$2:call$2(arg0,arg1),foo$2:main_A_defaultFoo$closure(0)]*/
+ /*member: A.foo:
+ elided,
+ stubCalls=[
+ foo$2:call$2(2),
+ foo$2:main_A_defaultFoo$closure(0)]
+ */
final BinaryFunc foo;
}
diff --git a/pkg/compiler/test/codegen/model_test.dart b/pkg/compiler/test/codegen/model_test.dart
index 9e694ee..6748f8b 100644
--- a/pkg/compiler/test/codegen/model_test.dart
+++ b/pkg/compiler/test/codegen/model_test.dart
@@ -14,7 +14,6 @@
import 'package:compiler/src/diagnostics/diagnostic_listener.dart';
import 'package:compiler/src/elements/entities.dart';
import 'package:compiler/src/js/js.dart' as js;
-import 'package:compiler/src/js_backend/namer.dart';
import 'package:compiler/src/js_emitter/model.dart';
import 'package:compiler/src/js_model/element_map.dart';
import 'package:compiler/src/js_model/js_world.dart';
@@ -99,7 +98,6 @@
String name;
if (selector is js.Name) {
name = selector.key;
- fixedNameCall = selector is StringBackedName;
} else if (selector is js.LiteralString) {
/// Call to fixed backend name, so we include the argument
/// values to test encoding of optional parameters in native
diff --git a/pkg/js_ast/lib/src/nodes.dart b/pkg/js_ast/lib/src/nodes.dart
index 2868346..903026e 100644
--- a/pkg/js_ast/lib/src/nodes.dart
+++ b/pkg/js_ast/lib/src/nodes.dart
@@ -445,8 +445,6 @@
return clone;
}
- VariableUse asVariableUse() => null;
-
bool get isCommaOperator => false;
bool get isFinalized => true;
@@ -1001,15 +999,15 @@
abstract class Declaration implements VariableReference {}
-/// An implementation of [Name] represents a potentially late bound name in
-/// the generated ast.
+/// [Name] is an extension point to allow a JavaScript AST to contain
+/// identifiers that are bound later. This is used in minification.
///
-/// While [Name] implements comparable, there is no requirement on the actual
-/// implementation of [compareTo] other than that it needs to be stable.
-/// In particular, there is no guarantee that implementations of [compareTo]
-/// will implement some form of lexicographic ordering like [String.compareTo].
-abstract class Name extends Literal
- implements Declaration, Parameter, Comparable<Name> {
+/// [Name] is a [Literal] so that it can occur as a property access selector.
+//
+// TODO(sra): Figure out why [Name] is a Declaration and Parameter, and where
+// that is used. How should the printer know if an occurrence of a Name is meant
+// to be a Literal or a Declaration (which includes a VariableUse)?
+abstract class Name extends Literal implements Declaration, Parameter {
T accept<T>(NodeVisitor<T> visitor) => visitor.visitName(this);
R accept1<R, A>(NodeVisitor1<R, A> visitor, A arg) =>
@@ -1017,6 +1015,12 @@
Name _clone();
+ /// Returns the text of this name.
+ ///
+ /// May throw if the text has not been decided. Typically the text is decided
+ /// in some finalization phase that happens before the AST is printed.
+ String get name;
+
/// Returns a unique [key] for this name.
///
/// The key is unrelated to the actual name and is not intended for human
@@ -1395,8 +1399,6 @@
VariableUse _clone() => new VariableUse(name);
- VariableUse asVariableUse() => this;
-
String toString() => 'VariableUse($name)';
}
