From 130d6199c3d6eda427c02214c954d99590432d8e Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 18 Jul 2023 18:54:26 +0000 Subject: [PATCH] Field promotion: make the core promotability algorithm sharable; fix bugs In the following code, it's not safe for the field `C._f` to undergo type promotion, because a variable with static type `C` might have type `D` at runtime, in which case `C._f` will get dispatched to `noSuchMethod`, which is not guaranteed to return a stable result. class C { final int? _f; } class D implements C { noSuchMethod(_) => ...; } foo(C c) { if (c._f != null) { print(c._f + 1); // UNSAFE! } } Therefore, in order to determine which fields are promotable, the implementations need to analyze enough of the class hierarchy to figure out which field accesses might get dispatched to `noSuchMethod`. Currently, the CFE does this by following its usual algorithm for generating `noSuchMethod` forwarders before trying to determine which fields are promotable. The analyzer, on the other hand, doesn't have an algorithm for generating `noSuchMethod` forwarders (since it doesn't implement execution semantics); so instead it has its own logic to figure out when a `noSuchMethod` forwarder is needed for a field, and disable promotion for that field. But there's a chicken-and-egg problem in the CFE: the CFE needs to determine which fields are promotable before doing top-level inference (since the initializers of top-level fields might make use of field promotion, affecting their inferred types--see #50522). But it doesn't decide where `noSuchMethod` forwarders are needed until after top-level inference (because the same phase that generates `noSuchMethod` forwarders also generates forwarders that do runtime covariant type-checking, and so it has to run after all top level types have been inferred). To fix the chicken-and-egg problem, I plan to rework the CFE so that it uses the same algorithm as the analyzer to determine which fields are promotable. This CL makes a first step towards that goal, by reworking the analyzer's field promotability algorithm into a form where it can be shared with the CFE, and moving it to `package:_fe_analyzer_shared`. Since this required a fairly substantial rewrite, I went ahead and fixed #52938 in the process. Fixes #52938. Change-Id: I9e68f51b3ea9a967f55f15bdc445cc1c0efdabdd Bug: https://github.com/dart-lang/sdk/issues/52938 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313293 Reviewed-by: Johnni Winther Reviewed-by: Konstantin Shcheglov Reviewed-by: Sigmund Cherem Commit-Queue: Paul Berry --- .../lib/src/field_promotability.dart | 355 ++++++++++++++++++ pkg/_fe_analyzer_shared/pubspec.yaml | 1 + .../test/field_promotability_test.dart | 197 ++++++++++ .../lib/src/summary2/field_promotability.dart | 265 ------------- .../lib/src/summary2/library_builder.dart | 93 ++++- .../dart/resolution/field_promotion_test.dart | 181 +++++++++ .../test/spell_checking_list_code.txt | 9 + 7 files changed, 834 insertions(+), 267 deletions(-) create mode 100644 pkg/_fe_analyzer_shared/lib/src/field_promotability.dart create mode 100644 pkg/_fe_analyzer_shared/test/field_promotability_test.dart delete mode 100644 pkg/analyzer/lib/src/summary2/field_promotability.dart diff --git a/pkg/_fe_analyzer_shared/lib/src/field_promotability.dart b/pkg/_fe_analyzer_shared/lib/src/field_promotability.dart new file mode 100644 index 00000000000..d61f947d334 --- /dev/null +++ b/pkg/_fe_analyzer_shared/lib/src/field_promotability.dart @@ -0,0 +1,355 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:_fe_analyzer_shared/src/util/dependency_walker.dart'; + +/// Information about a [Class] that is necessary for computing the set of +/// private `noSuchMethod` getters the compiler will generate. +class ClassInfo { + /// The [_InterfaceNode] for the [Class]. + final _InterfaceNode _interfaceNode; + + /// The [_ImplementedNode] for the [Class]. + final _ImplementedNode _implementedNode; + + ClassInfo(this._interfaceNode, this._implementedNode); +} + +/// This class examines all the [Class]es in a library and determines which +/// fields are promotable within that library. +/// +/// Note: the term [Class] is used in a general sense here; it can represent any +/// class, mixin, or enum declared in the user's program. +/// +/// A field is considered promotable if all of the following are true: +/// - It is final +/// - Its name is private +/// - The library doesn't contain any non-final fields, concrete getters, or +/// `noSuchMethod` getters with the same name. +/// +/// These rules were chosen because they are sufficient to determine that all +/// reads of the field that complete normally will either throw an exception, +/// return a single stable object, or return objects with a single stable +/// runtime type; this ensures that type promotion is safe. +/// +/// The reason the field must be private is because if it isn't, then at +/// runtime, a read of the field might resolve to a getter or a non-final field +/// in a subclass that's implemented in some other library, and hence the result +/// of the read won't be stable. +/// +/// Note that as a result of the third bullet item, any private name that's +/// associated with a non-final field, concrete getter, or `noSuchMethod` getter +/// renders all fields with the same name unpromotable within the library. The +/// reason for this is that since most Dart classes can also be used as +/// interfaces (via an `implements` clause), a read that statically resolves to +/// a private final field might not actually resolve to that field at runtime; +/// it might resolve to some other field or getter with the same name. (In +/// principle it would be possible to narrow the set of possible resolution +/// targets through whole-program analysis, or by careful consideration of class +/// modifiers and public vs. private class names; however that would make the +/// analysis much more complex, and also make it difficult to explain to users +/// when to expect field promotion to work; so we've elected to simply assume +/// that all fields and getters with the same name might potentially alias to +/// one another). +/// +/// Since all fields with the same name within the library have the same +/// promotability, this class doesn't attempt to assign a promotability boolean +/// to each field; instead it computes the set of private names for which field +/// promotion is not allowed. +/// +/// A word about `noSuchMethod` getters: a `noSuchMethod` getter is a +/// `noSuchMethod` forwarder that implements a getter. The compiler generates +/// `noSuchMethod` forwarders when a concrete [Class] inherits a member into its +/// interface (via an `implements` clause), but it doesn't contain or inherit a +/// concrete implementation of that member. (Note that this is only legal if the +/// class contains or inherits an implementation of `noSuchMethod`). If the name +/// of the inherited member is accessible in the [Class]'s library (i.e. it's +/// either a public name or it's a private name that belongs to the [Class]'s +/// library), then the synthetic `noSuchMethod` forwarder calls `noSuchMethod` +/// and passes along the return value (casting it to the proper type). If the +/// name of the inherited member *isn't* accessible in the [Class]'s library, +/// then the synthetic `noSuchMethod` forwarder throws an exception. +/// +/// `noSuchMethod` getters that forward to `noSuchMethod` defeat field promotion +/// for the same reason that ordinary getters do (they aren't guaranteed to +/// return a stable value). However, `noSuchMethod` getters that simply throw an +/// exception do not defeat field promotion, because the exception prevents any +/// code that relies on field promotion soundness from being reachable. Since +/// only private fields are eligible from promotion in the first place, this +/// means that it's only necessary to search the current library for +/// `noSuchMethod` getters that might defeat field promotion (because a +/// `noSuchMethod` getter in another library that's associated with a private +/// name in *this* library will always throw). +/// +/// Note that it's possible that one class will have a final field with a given +/// private name, while another class will have a method with the same name (or +/// a `noSuchMethod` forwarder for such a method). If this happens, then an +/// attempt to read the field might at runtime resolve to a tear-off of the +/// corresponding method. This is ok (it's not necessary to suppress promotion +/// of the field), because even though the resulting tear-offs might not be +/// stable in the sense of always being identical, they will nonetheless always +/// have the same runtime type. Hence, we can completely ignore methods when +/// computing which fields in the library are promotable. +abstract class FieldPromotability { + /// The set of field names in the library that have been determined to be + /// unsafe to promote. + final Set _unpromotableFieldNames = {}; + + /// Map from a [Class] object to the [_ImplementedNode] that records the names + /// of concrete fields and getters declared in or inherited by the [Class]. + final Map> _implementedNodes = + new Map.identity(); + + /// Map from a [Class] object to the [_InterfaceNode] that records the names + /// of getters in the interface for a [Class]. + final Map> _interfaceNodes = new Map.identity(); + + /// List of information about concrete [Class]es in the library. + final List> _concreteInfoList = []; + + /// Records the presence of [class_] in the library. The client should call + /// this method once for each class, mixin, and enum declared in the library. + /// + /// Returns a [ClassInfo] object describing the class. After calling this + /// method, the client should call [addField] and [addGetter] to record all + /// the non-synthetic instance fields and getters in the class. + ClassInfo addClass(Class class_, {required bool isAbstract}) { + ClassInfo classInfo = new ClassInfo( + _getInterfaceNode(class_), _getImplementedNode(class_)); + + if (!isAbstract) { + _concreteInfoList.add(classInfo); + } + + return classInfo; + } + + /// Records that the [Class] described by [classInfo] contains a non-synthetic + /// instance field with the given [name]. + /// + /// [isFinal] indicates whether the field is a final field. [isAbstract] + /// indicates whether the field is abstract. + /// + /// A return value of `true` indicates that this field *might* wind up being + /// promotable; a return value of `false` indicates that it *definitely* isn't + /// promotable. + bool addField(ClassInfo classInfo, String name, + {required bool isFinal, required bool isAbstract}) { + // Public fields are never promotable, so we may safely ignore fields with + // public names. + if (!name.startsWith('_')) { + return false; + } + + // Record the field name for later use in computation of `noSuchMethod` + // getters. + classInfo._interfaceNode._directNames.add(name); + if (!isAbstract) { + classInfo._implementedNode._directNames.add(name); + } + + if (!isFinal) { + // The field isn't final, so it isn't promotable, nor is any other field + // in the library with the same name. + _unpromotableFieldNames.add(name); + } + + // If the field is final, it might wind up being promotable. + return isFinal; + } + + /// Records that the [Class] described by [classInfo] contains a non-synthetic + /// instance getter with the given [name]. + /// + /// [isAbstract] indicates whether the getter is abstract. + void addGetter(ClassInfo classInfo, String name, + {required bool isAbstract}) { + // Public fields are never promotable, so we may safely ignore getters with + // public names. + if (!name.startsWith('_')) { + return; + } + + // Record the getter name for later use in computation of `noSuchMethod` + // getters. + classInfo._interfaceNode._directNames.add(name); + if (!isAbstract) { + classInfo._implementedNode._directNames.add(name); + + // The getter is concrete, so no fields with the same name are promotable. + _unpromotableFieldNames.add(name); + } + } + + /// Computes the set of private field names which are not safe to promote in + /// the library. + /// + /// The client should call this method once after all [Class]es, fields, and + /// getters have been recorded using [addClass], [addField], and [addGetter]. + Set computeUnpromotablePrivateFieldNames() { + // The names of private non-final fields and private getters have already + // been added to [_unpromotableFieldNames] by [addField] and [addGetter]. So + // all that remains to do is figure out which field names are unpromotable + // due to the presence of `noSuchMethod` getters. To do this, we'll need to + // walk the class hierarchy and gather (a) the names of private instance + // getters in each class's interface, and (b) the names of private instance + // fields and getters in each class's implementation. + _ClassHierarchyWalker interfaceWalker = + new _ClassHierarchyWalker(); + _ClassHierarchyWalker implementedWalker = + new _ClassHierarchyWalker(); + + // For each concrete class in the library, + for (ClassInfo info in _concreteInfoList) { + // Compute names of getters in the interface. + _InterfaceNode interfaceNode = info._interfaceNode; + interfaceWalker.walk(interfaceNode); + Set interfaceNames = interfaceNode._transitiveNames!; + + // Compute names of actually implemented getters. + _ImplementedNode implementedNode = info._implementedNode; + implementedWalker.walk(implementedNode); + Set implementedNames = implementedNode._transitiveNames!; + + // `noSuchMethod` getters will be generated for getters that are in the + // interface, but not actually implemented; consequently, fields with + // these names are not safe to promote. + for (String name in interfaceNames) { + if (!implementedNames.contains(name)) { + _unpromotableFieldNames.add(name); + } + } + } + + return _unpromotableFieldNames; + } + + /// Returns an iterable of the direct superclasses of [class_]. If + /// [ignoreImplements] is `true`, then superclasses reached through an + /// `implements` clause are ignored. + /// + /// This is used to gather the transitive closure of fields and getters + /// present in a class's interface and implementation. Therefore, it does not + /// matter whether the client uses a fully sugared model of mixins (in which + /// `class C extends B with M1, M2 {}` is represented as a single class with + /// direct superclasses `B`, `M1`, and `M2`) or a fully desugared model (in + /// which `class C extends B with M1, M2` represents a class `C` with + /// superclass `B&M1&M2`, which in turn has supertypes `B&M1` and `M2`, etc.) + Iterable getSuperclasses(Class class_, + {required bool ignoreImplements}); + + /// Gets or creates the [_ImplementedNode] for [class_]. + _ImplementedNode _getImplementedNode(Class class_) => + _implementedNodes[class_] ??= new _ImplementedNode(this, class_); + + /// Gets or creates the [_InterfaceNode] for [class_]. + _InterfaceNode _getInterfaceNode(Class class_) => + _interfaceNodes[class_] ??= new _InterfaceNode(this, class_); +} + +/// Dependency walker that traverses the graph of a class's type hierarchy, +/// gathering the transitive closure of field and getter names. +/// +/// This is based on the [DependencyWalker] class, which implements Tarjan's +/// strongly connected component's algorithm in order to efficiently handle +/// cycles in the class hierarchy (which the analyzer tolerates). +class _ClassHierarchyWalker + extends DependencyWalker<_Node> { + @override + void evaluate(_Node v) => evaluateScc([v]); + + @override + void evaluateScc(List<_Node> scc) { + // Gather the names directly declared in all the classes in this strongly + // connected component, plus all the names in the transitive closure of the + // strongly connected components this component depends on. + Set transitiveNames = {}; + for (_Node node in scc) { + transitiveNames.addAll(node._directNames); + for (_Node dependency in Node.getDependencies(node)) { + Set? namesFromDependency = dependency._transitiveNames; + if (namesFromDependency != null) { + transitiveNames.addAll(namesFromDependency); + } + } + } + // Store this list of names in all the nodes of this strongly connected + // component. + for (_Node node in scc) { + node._transitiveNames = transitiveNames; + } + } +} + +/// Data structure tracking the set of private fields and getters a [Class] +/// concretely implements. +/// +/// This data structure extends [_Node] so that we can efficiently walk the +/// superclass chain (without having to worry about circularities) in order to +/// include getters concretely implemented in superclasses and mixins. +class _ImplementedNode extends _Node { + _ImplementedNode(super.fieldPromotability, super.element); + + @override + List<_Node> computeDependencies() { + // We need to gather field and getter names from the transitive closure of + // superclasses, following `with` and `extends` edges but not `implements` + // edges. So the set of dependencies of this node is the set of immediate + // superclasses, ignoring `implements`. + List<_Node> dependencies = []; + for (Class supertype in _fieldPromotability.getSuperclasses(_class, + ignoreImplements: true)) { + dependencies.add(_fieldPromotability._getImplementedNode(supertype)); + } + return dependencies; + } +} + +/// Data structure tracking the set of getters in a [Class]'s interface. +/// +/// This data structure extends [_Node] so that we can efficiently walk the +/// class hierarchy (without having to worry about circularities) in order to +/// include getters defined in superclasses, mixins, and interfaces. +class _InterfaceNode extends _Node { + _InterfaceNode(super.fieldPromotability, super.element); + + @override + List<_Node> computeDependencies() { + // We need to gather field and getter names from the transitive closure of + // superclasses, following `with`, `extends`, `implements`, and `on` edges. + // So the set of dependencies of this node is the set of immediate + // superclasses, including `implements`. + List<_Node> dependencies = []; + for (Class supertype in _fieldPromotability.getSuperclasses(_class, + ignoreImplements: false)) { + dependencies.add(_fieldPromotability._getInterfaceNode(supertype)); + } + return dependencies; + } +} + +/// A node in either the graph of a class's type hierarchy, recording the +/// information necessary for computing the set of private `noSuchMethod` +/// getters the compiler will generate. +abstract class _Node extends Node<_Node> { + /// A reference back to the [FieldPromotability] object. + final FieldPromotability _fieldPromotability; + + /// The [Class] represented by this node. + final Class _class; + + /// The names of getters declared by [_class] directly. + final Set _directNames = {}; + + /// The names of getters declared by [_class] and its superinterfaces. + /// + /// Populated when [_ClassHierarchyWalker] encounters a strongly connected + /// component. + Set? _transitiveNames; + + _Node(this._fieldPromotability, this._class); + + @override + bool get isEvaluated => _transitiveNames != null; +} diff --git a/pkg/_fe_analyzer_shared/pubspec.yaml b/pkg/_fe_analyzer_shared/pubspec.yaml index 4bdb5859ed8..f9e535dc048 100644 --- a/pkg/_fe_analyzer_shared/pubspec.yaml +++ b/pkg/_fe_analyzer_shared/pubspec.yaml @@ -16,6 +16,7 @@ dependencies: # best practice for packages is to specify their compatible version ranges. # See also https://dart.dev/tools/pub/dependencies. dev_dependencies: + checks: any test: any dependency_overrides: diff --git a/pkg/_fe_analyzer_shared/test/field_promotability_test.dart b/pkg/_fe_analyzer_shared/test/field_promotability_test.dart new file mode 100644 index 00000000000..ae1228e0152 --- /dev/null +++ b/pkg/_fe_analyzer_shared/test/field_promotability_test.dart @@ -0,0 +1,197 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:_fe_analyzer_shared/src/field_promotability.dart'; +import 'package:checks/checks.dart'; +import 'package:test/scaffolding.dart'; + +main() { + test('final private field is promotable', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + check(_TestFieldPromotability().run([c])).unorderedEquals({}); + check(f.isPossiblyPromotable).isTrue(); + }); + + test('final public field is not promotable', () { + var f = Field('f', isFinal: true); + var c = Class(fields: [f]); + // Note that the set returned by `_TestFieldPromotability.run` is just the + // set of *private* field names that are unpromotable, so even though `f` + // is not promotable, the returned set is empty. + check(_TestFieldPromotability().run([c])).unorderedEquals({}); + check(f.isPossiblyPromotable).isFalse(); + }); + + test('non-final private field is not promotable', () { + var f = Field('_f'); + var c = Class(fields: [f]); + check(_TestFieldPromotability().run([c])).unorderedEquals({'_f'}); + check(f.isPossiblyPromotable).isFalse(); + }); + + group('concrete getter renders a private field non-promotable:', () { + test('in a concrete class', () { + var c = Class(fields: [Field('_f', isFinal: true)]); + var d = Class(getters: [Getter('_f')]); + check(_TestFieldPromotability().run([c, d])).unorderedEquals({'_f'}); + }); + + test('in an abstract class', () { + var c = Class(fields: [Field('_f', isFinal: true)]); + var d = Class(isAbstract: true, getters: [Getter('_f')]); + check(_TestFieldPromotability().run([c, d])).unorderedEquals({'_f'}); + }); + }); + + test('abstract getter does not render a private field non-promotable', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]); + check(_TestFieldPromotability().run([c, d])).unorderedEquals({}); + check(f.isPossiblyPromotable).isTrue(); + }); + + test('public concrete getter is ignored', () { + // Since public fields are never promotable, there's no need for the + // algorithm to keep track of public concrete getters. + var f = Field('f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(getters: [Getter('f')]); + // Therefore the set returned by `_TestFieldPromotability.run` is empty. + check(_TestFieldPromotability().run([c, d])).unorderedEquals({}); + check(f.isPossiblyPromotable).isFalse(); + }); + + group('unimplemented getter renders a field non-promotable:', () { + test('induced by getter', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = + Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]); + var e = Class(implements: [d]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({'_f'}); + }); + + test('induced by field', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(isAbstract: true, fields: [Field('_f', isFinal: true)]); + var e = Class(implements: [d]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({'_f'}); + }); + }); + + test('unimplemented getter in an abstract class is ok', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(isAbstract: true, getters: [Getter('_f', isAbstract: true)]); + var e = Class(isAbstract: true, implements: [d]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({}); + check(f.isPossiblyPromotable).isTrue(); + }); + + test('unimplemented abstract field renders a field non-promotable:', () { + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class( + isAbstract: true, + fields: [Field('_f', isAbstract: true, isFinal: true)]); + var e = Class(extendsOrMixesIn: [d]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({'_f'}); + }); + + test('implementations are inherited transitively', () { + // `e` inherits `f` from `c` via `d`, so no `noSuchMethod` forwarder is + // needed, and therefore promotion is allowed. + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(extendsOrMixesIn: [c]); + var e = Class(extendsOrMixesIn: [d], implements: [c]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({}); + check(f.isPossiblyPromotable).isTrue(); + }); + + test('interfaces are inherited transitively', () { + // `e` inherits the interface for `f` from `c` via `d`, so a `noSuchMethod` + // forwarder is needed, and therefore promotion is not allowed. + var f = Field('_f', isFinal: true); + var c = Class(fields: [f]); + var d = Class(isAbstract: true, implements: [c]); + var e = Class(implements: [d]); + check(_TestFieldPromotability().run([c, d, e])).unorderedEquals({'_f'}); + }); + + test('class hierarchy circularities are handled', () { + // Since it's a compile error to have a circularity in the class hierarchy, + // all we need to check is that the algorithm terminates; we don't check the + // result. + var c = Class(extendsOrMixesIn: []); + var d = Class(extendsOrMixesIn: [c]); + c.extendsOrMixesIn.add(d); + var e = Class(extendsOrMixesIn: [d]); + _TestFieldPromotability().run([c, d, e]); + }); +} + +class Class { + final List extendsOrMixesIn; + final List implements; + final bool isAbstract; + final List fields; + final List getters; + + Class( + {this.extendsOrMixesIn = const [], + this.implements = const [], + this.isAbstract = false, + this.fields = const [], + this.getters = const []}); +} + +class Field { + final String name; + final bool isFinal; + final bool isAbstract; + late final bool isPossiblyPromotable; + + Field(this.name, {this.isFinal = false, this.isAbstract = false}); +} + +class Getter { + final String name; + final bool isAbstract; + + Getter(this.name, {this.isAbstract = false}); +} + +class _TestFieldPromotability extends FieldPromotability { + @override + Iterable getSuperclasses(Class class_, + {required bool ignoreImplements}) { + if (ignoreImplements) { + return class_.extendsOrMixesIn; + } else { + return [...class_.extendsOrMixesIn, ...class_.implements]; + } + } + + Set run(Iterable classes) { + // Iterate through all the classes, enums, and mixins in the library, + // recording the non-synthetic instance fields and getters of each. + for (var class_ in classes) { + var classInfo = addClass(class_, isAbstract: class_.isAbstract); + for (var field in class_.fields) { + field.isPossiblyPromotable = addField(classInfo, field.name, + isFinal: field.isFinal, isAbstract: field.isAbstract); + } + for (var getter in class_.getters) { + addGetter(classInfo, getter.name, isAbstract: getter.isAbstract); + } + } + + // Compute the set of field names that are not promotable. + return computeUnpromotablePrivateFieldNames(); + } +} diff --git a/pkg/analyzer/lib/src/summary2/field_promotability.dart b/pkg/analyzer/lib/src/summary2/field_promotability.dart deleted file mode 100644 index 1aa6d339a50..00000000000 --- a/pkg/analyzer/lib/src/summary2/field_promotability.dart +++ /dev/null @@ -1,265 +0,0 @@ -// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file -// for details. All rights reserved. Use of this source code is governed by a -// BSD-style license that can be found in the LICENSE file. - -import 'package:_fe_analyzer_shared/src/util/dependency_walker.dart'; -import 'package:analyzer/dart/element/element.dart'; -import 'package:analyzer/src/dart/element/element.dart'; -import 'package:analyzer/src/summary2/library_builder.dart'; -import 'package:collection/collection.dart'; - -/// This class computes which fields are promotable in a library. -class FieldPromotability { - final LibraryBuilder _libraryBuilder; - - /// Fields that might be promotable, if not marked unpromotable later. - final List _potentiallyPromotableFields = []; - - /// The set of field names that are not safe to promote. - /// 1. There is a non-final field with this name. - /// 2. There is a concrete getter with this name. - /// 3. Has a `noSuchMethod` forwarder. - final Set _unpromotableFieldNames = {}; - - /// Getters actually implemented by the key element. - final Map _implementedNodes = - Map.identity(); - - /// Getters in the interface of the key element. - final Map _interfaceNodes = Map.identity(); - - /// Information about concrete [InterfaceElement]s. - final List<_InterfaceElementInfo> _concreteInfoList = []; - - FieldPromotability(this._libraryBuilder); - - void perform() { - for (var unitElement in _libraryBuilder.element.units) { - for (var class_ in unitElement.classes) { - _addInterfaceElement(class_, isAbstract: class_.isAbstract); - } - for (var enum_ in unitElement.enums) { - _addInterfaceElement(enum_, isAbstract: false); - } - for (var mixin_ in unitElement.mixins) { - _addInterfaceElement(mixin_, isAbstract: true); - } - } - - var interfaceWalker = _Walker(); - var implementedWalker = _Walker(); - - for (var info in _concreteInfoList) { - // Compute names of getters in the interface. - var interfaceNode = info._interfaceNode; - interfaceWalker.walk(interfaceNode); - var interfaceNames = interfaceNode._transitiveNames!; - - // Compute names of actually implemented getters. - var implementedNode = info._implementedNode; - implementedWalker.walk(implementedNode); - var implementedNames = implementedNode._transitiveNames!; - - // noSuchMethod forwarders will be generated for getters that are - // in the interface, but not actually implemented; consequently, - // fields with these names are not safe to promote. - for (var name in interfaceNames) { - if (!implementedNames.contains(name)) { - _unpromotableFieldNames.add(name); - } - } - } - - for (var field in _potentiallyPromotableFields) { - if (!_unpromotableFieldNames.contains(field.name)) { - field.isPromotable = true; - } - } - } - - void _addInterfaceElement( - InterfaceElement element, { - required bool isAbstract, - }) { - var interfaceInfo = _InterfaceElementInfo(this, element, - _getInterfaceNode(element), _getImplementedNode(element)); - - if (!isAbstract) { - _concreteInfoList.add(interfaceInfo); - } - } - - /// Gets or creates the [_ImplementedNode] for [element]. - _ImplementedNode _getImplementedNode(InterfaceElement element) => - _implementedNodes[element] ??= _ImplementedNode(this, element); - - /// If the [element] is not in the [_libraryBuilder], returns `null`. - /// Otherwise, invokes [_getImplementedNode]. - _ImplementedNode? _getImplementedNodeOrNull(InterfaceElement element) { - if (element.library == _libraryBuilder.element) { - return _getImplementedNode(element); - } - return null; - } - - /// Gets or creates the [_InterfaceNode] for [element]. - _InterfaceNode _getInterfaceNode(InterfaceElement element) => - _interfaceNodes[element] ??= _InterfaceNode(this, element); - - /// If the [element] is not in the [_libraryBuilder], returns `null`. - /// Otherwise, invokes [_getInterfaceNode]. - _InterfaceNode? _getInterfaceNodeOrNull(InterfaceElement element) { - if (element.library == _libraryBuilder.element) { - return _getInterfaceNode(element); - } - return null; - } -} - -/// Data structure tracking the set of getters a class concretely implements. -/// -/// This data structure extends [_Node] so that we can efficiently walk the -/// superclass chain (without having to worry about circularities) in order to -/// include getters concretely implemented in superclasses and mixins. -class _ImplementedNode extends _Node { - _ImplementedNode(super.fieldPromotability, super.element); - - @override - List<_Node> computeDependencies() { - return [_element.supertype, ..._element.mixins] - .whereNotNull() - .map((type) => type.element) - .map(_fieldPromotability._getImplementedNodeOrNull) - .whereNotNull() - .toList(); - } -} - -/// Information about an [InterfaceElement]. -class _InterfaceElementInfo { - final FieldPromotability _fieldPromotability; - final InterfaceElement _element; - final _InterfaceNode _interfaceNode; - final _ImplementedNode _implementedNode; - - _InterfaceElementInfo( - this._fieldPromotability, - this._element, - this._interfaceNode, - this._implementedNode, - ) { - for (var field in _element.fields) { - field as FieldElementImpl; - _addFieldElement(field); - } - - for (var accessor in _element.accessors) { - _addPropertyAccessorElement(accessor); - } - } - - void _addFieldElement(FieldElementImpl element) { - if (element.isStatic || element.isSynthetic) { - return; - } - - var name = element.name; - if (!name.startsWith('_')) { - return; - } - - _interfaceNode._directNames.add(name); - _implementedNode._directNames.add(name); - - if (element.isFinal) { - _fieldPromotability._potentiallyPromotableFields.add(element); - } else { - _fieldPromotability._unpromotableFieldNames.add(name); - } - } - - void _addPropertyAccessorElement(PropertyAccessorElement element) { - if (!element.isGetter || element.isStatic || element.isSynthetic) { - return; - } - - var name = element.name; - if (!name.startsWith('_')) { - return; - } - - _interfaceNode._directNames.add(name); - - if (!element.isAbstract) { - _fieldPromotability._unpromotableFieldNames.add(name); - _implementedNode._directNames.add(name); - } - } -} - -/// Data structure tracking the set of getters in a class's interface. -/// -/// This data structure extends [_Node] so that we can efficiently walk the -/// class hierarchy (without having to worry about circularities) in order to -/// include getters defined in superclasses, mixins, and interfaces. -class _InterfaceNode extends _Node { - _InterfaceNode(super.fieldPromotability, super.element); - - @override - List<_Node> computeDependencies() { - var directInterfaces = [ - _element.supertype, - ..._element.mixins, - ..._element.interfaces - ]; - return directInterfaces - .whereNotNull() - .map((type) => type.element) - .map(_fieldPromotability._getInterfaceNodeOrNull) - .whereNotNull() - .toList(); - } -} - -/// Dependency walker node allowing us to efficiently walk the class hierarchy -/// and accumulate getter names. -abstract class _Node extends Node<_Node> { - /// A reference back to the [FieldPromotability] object. - final FieldPromotability _fieldPromotability; - - /// The element represented by this node. - final InterfaceElement _element; - - /// The names of getters declared by [_element] directly. - final Set _directNames = {}; - - /// The names of getters declared by [_element] and its superinterfaces. - Set? _transitiveNames; - - _Node(this._fieldPromotability, this._element); - - @override - bool get isEvaluated => _transitiveNames != null; -} - -class _Walker extends DependencyWalker<_Node> { - @override - void evaluate(_Node v) => evaluateScc([v]); - - @override - void evaluateScc(List<_Node> scc) { - var transitiveNames = {}; - for (var node in scc) { - transitiveNames.addAll(node._directNames); - for (var dependency in Node.getDependencies(node)) { - var namesFromDependency = dependency._transitiveNames; - if (namesFromDependency != null) { - transitiveNames.addAll(namesFromDependency); - } - } - } - for (var node in scc) { - node._transitiveNames = transitiveNames; - } - } -} diff --git a/pkg/analyzer/lib/src/summary2/library_builder.dart b/pkg/analyzer/lib/src/summary2/library_builder.dart index a4c6ec6d3bb..835a37454fe 100644 --- a/pkg/analyzer/lib/src/summary2/library_builder.dart +++ b/pkg/analyzer/lib/src/summary2/library_builder.dart @@ -2,6 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:_fe_analyzer_shared/src/field_promotability.dart'; import 'package:analyzer/dart/analysis/features.dart'; import 'package:analyzer/dart/analysis/utilities.dart'; import 'package:analyzer/dart/element/element.dart'; @@ -19,7 +20,6 @@ import 'package:analyzer/src/summary2/constructor_initializer_resolver.dart'; import 'package:analyzer/src/summary2/default_value_resolver.dart'; import 'package:analyzer/src/summary2/element_builder.dart'; import 'package:analyzer/src/summary2/export.dart'; -import 'package:analyzer/src/summary2/field_promotability.dart'; import 'package:analyzer/src/summary2/link.dart'; import 'package:analyzer/src/summary2/macro_application.dart'; import 'package:analyzer/src/summary2/metadata_resolver.dart'; @@ -279,7 +279,7 @@ class LibraryBuilder { return; } - FieldPromotability(this).perform(); + _FieldPromotability(this).perform(); } void declare(String name, Reference reference) { @@ -998,6 +998,95 @@ class LinkingUnit { }); } +/// This class examines all the [InterfaceElement]s in a library and determines +/// which fields are promotable within that library. +class _FieldPromotability extends FieldPromotability { + /// The [_libraryBuilder] for the library being analyzed. + final LibraryBuilder _libraryBuilder; + + /// Fields that might be promotable, if not marked unpromotable later. + final List _potentiallyPromotableFields = []; + + _FieldPromotability(this._libraryBuilder); + + @override + Iterable getSuperclasses(InterfaceElement class_, + {required bool ignoreImplements}) { + List result = []; + var supertype = class_.supertype; + if (supertype != null) { + result.add(supertype.element); + } + for (var m in class_.mixins) { + result.add(m.element); + } + if (!ignoreImplements) { + for (var interface in class_.interfaces) { + result.add(interface.element); + } + if (class_ is MixinElement) { + for (var constraint in class_.superclassConstraints) { + result.add(constraint.element); + } + } + } + return result; + } + + /// Computes which fields are promotable and updates their `isPromotable` + /// properties accordingly. + void perform() { + // Iterate through all the classes, enums, and mixins in the library, + // recording the non-synthetic instance fields and getters of each. + for (var unitElement in _libraryBuilder.element.units) { + for (var class_ in unitElement.classes) { + _handleMembers(addClass(class_, isAbstract: class_.isAbstract), class_); + } + for (var enum_ in unitElement.enums) { + _handleMembers(addClass(enum_, isAbstract: false), enum_); + } + for (var mixin_ in unitElement.mixins) { + _handleMembers(addClass(mixin_, isAbstract: true), mixin_); + } + } + + // Compute the set of field names that are not promotable. + var unpromotableFieldNames = computeUnpromotablePrivateFieldNames(); + + // Set the `isPromotable` bit for each field element that *is* promotable. + for (var field in _potentiallyPromotableFields) { + if (!unpromotableFieldNames.contains(field.name)) { + field.isPromotable = true; + } + } + } + + /// Records all the non-synthetic instance fields and getters of [class_] into + /// [classInfo]. + void _handleMembers( + ClassInfo classInfo, InterfaceElementImpl class_) { + for (var field in class_.fields) { + if (field.isStatic || field.isSynthetic) { + continue; + } + + var isPotentiallyPromotable = addField(classInfo, field.name, + isFinal: field.isFinal, isAbstract: field.isAbstract); + if (isPotentiallyPromotable) { + _potentiallyPromotableFields.add(field); + } + } + + for (var accessor in class_.accessors) { + if (!accessor.isGetter || accessor.isStatic || accessor.isSynthetic) { + continue; + } + + addGetter(classInfo, accessor.name, isAbstract: accessor.isAbstract); + } + } +} + class _FlushElementOffsets extends GeneralizingElementVisitor { @override void visitElement(covariant ElementImpl element) { diff --git a/pkg/analyzer/test/src/dart/resolution/field_promotion_test.dart b/pkg/analyzer/test/src/dart/resolution/field_promotion_test.dart index a63b907088f..0a04b22490d 100644 --- a/pkg/analyzer/test/src/dart/resolution/field_promotion_test.dart +++ b/pkg/analyzer/test/src/dart/resolution/field_promotion_test.dart @@ -115,6 +115,49 @@ PropertyAccess '''); } + test_class_field_abstract() async { + // Even though an abstract non-final field is just syntactic sugar for an + // abstract getter/setter pair (and thus in principle shouldn't prevent + // promotion), there's no way to implement it without introducing either a + // getter or a non-final field (either of which would prevent promotion). So + // the implementation goes ahead and prevents promotion even if there's no + // implementation yet, to reduce churn for the user. + await assertNoErrorsInCode(''' +abstract class B { + abstract int? _foo; +} + +// Suppress "unused field" warning on `B._foo`. +int? f(B b) => b._foo; + +class C { + final int? _foo; + C(this._foo); +} + +void g(C c) { + if (c._foo != null) { + c._foo; + } +} +'''); + var node = findNode.prefixed('c._foo;'); + assertResolvedNodeText(node, r''' +PrefixedIdentifier + prefix: SimpleIdentifier + token: c + staticElement: self::@function::g::@parameter::c + staticType: C + period: . + identifier: SimpleIdentifier + token: _foo + staticElement: self::@class::C::@getter::_foo + staticType: int? + staticElement: self::@class::C::@getter::_foo + staticType: int? +'''); + } + test_class_field_invocation_prefixedIdentifier_nullability() async { await assertNoErrorsInCode(''' class C { @@ -669,6 +712,105 @@ PrefixedIdentifier '''); } + test_implemented_via_other_library() async { + // When determining the set of fields/getters in a class's implementation, + // it's necessary to traverse the whole class hierarchy, including classes + // outside the current library, because a class outside the current library + // may extend a class inside the current library. In the example below, + // `c._foo` is promotable because class E doesn't contain any `noSuchMethod` + // getters, due to the fact that it inherits an implementation of `_foo` + // from `C` via `D`. + newFile('$testPackageLibPath/other.dart', ''' +import 'test.dart'; + +class D extends C { + D(super.foo); +} +'''); + await assertNoErrorsInCode(''' +import 'other.dart'; + +class C { + final int? _foo; + C(this._foo); +} +class E extends D implements C { + E(super.foo); + noSuchMethod(_) => 12345; +} + +void f(C c) { + if (c._foo != null) { + c._foo; + } +} +'''); + var node = findNode.prefixed('c._foo;'); + assertResolvedNodeText(node, r''' +PrefixedIdentifier + prefix: SimpleIdentifier + token: c + staticElement: self::@function::f::@parameter::c + staticType: C + period: . + identifier: SimpleIdentifier + token: _foo + staticElement: self::@class::C::@getter::_foo + staticType: int + staticElement: self::@class::C::@getter::_foo + staticType: int +'''); + } + + test_interface_via_other_library() async { + // When determining the set of fields/getters in a class's interface, it's + // necessary to traverse the whole class hierarchy, including classes + // outside the current library, because a class outside the current library + // may extend or implement a class inside the current library. In the + // example below, `c._foo` is not promotable because class E contains a + // `noSuchMethod` getter for `_foo`, due to the fact that its interface + // inherits `_foo` from `C` via `D`. + newFile('$testPackageLibPath/other.dart', ''' +import 'test.dart'; + +class D extends C { + D(super.foo); +} +'''); + await assertNoErrorsInCode(''' +import 'other.dart'; + +class C { + final int? _foo; + C(this._foo); +} +class E implements D { + noSuchMethod(_) => 12345; +} + +void f(C c) { + if (c._foo != null) { + c._foo; + } +} +'''); + var node = findNode.prefixed('c._foo;'); + assertResolvedNodeText(node, r''' +PrefixedIdentifier + prefix: SimpleIdentifier + token: c + staticElement: self::@function::f::@parameter::c + staticType: C + period: . + identifier: SimpleIdentifier + token: _foo + staticElement: self::@class::C::@getter::_foo + staticType: int? + staticElement: self::@class::C::@getter::_foo + staticType: int? +'''); + } + test_language219() async { await assertNoErrorsInCode(''' // @dart = 2.19 @@ -703,6 +845,45 @@ PropertyAccess '''); } + test_mixin_on_clause() async { + // The type mentioned in a a mixin's "on" clause contributes to its + // interface. This needs to be accounted for when determining whether a + // `noSuchMethod` getter will be synthesized. In the example below, + // `c._foo` is not promotable because class D contains a `noSuchMethod` + // getter for `_foo`. + await assertNoErrorsInCode(''' +mixin M on C {} +class C { + final int? _foo; + C(this._foo); +} +class D implements M { + noSuchMethod(_) => 12345; +} + +void f(C c) { + if (c._foo != null) { + c._foo; + } +} +'''); + var node = findNode.prefixed('c._foo;'); + assertResolvedNodeText(node, r''' +PrefixedIdentifier + prefix: SimpleIdentifier + token: c + staticElement: self::@function::f::@parameter::c + staticType: C + period: . + identifier: SimpleIdentifier + token: _foo + staticElement: self::@class::C::@getter::_foo + staticType: int? + staticElement: self::@class::C::@getter::_foo + staticType: int? +'''); + } + test_super_get() async { await assertNoErrorsInCode(''' class B { diff --git a/pkg/front_end/test/spell_checking_list_code.txt b/pkg/front_end/test/spell_checking_list_code.txt index 16eb3cf057b..ab89db40c62 100644 --- a/pkg/front_end/test/spell_checking_list_code.txt +++ b/pkg/front_end/test/spell_checking_list_code.txt @@ -173,6 +173,7 @@ buffers buggy builder`s bulk +bullet bump bypassing c @@ -366,6 +367,7 @@ deepest deeply def defaulting +defeat degrades degree del @@ -469,6 +471,7 @@ efficient efficiently eight eighth +elected elem eliminating elt @@ -514,6 +517,7 @@ estimation et eval evar +examines exchanging execute executor @@ -984,6 +988,7 @@ nnbd node's nomenclature nominality +nonetheless nonexistent nonimplementation nonzero @@ -1288,6 +1293,7 @@ removal remover renames render +renders reordered reparse repeating @@ -1528,6 +1534,7 @@ subtraction subtracts suffixed suffixing +sugared suggests suite sum @@ -1615,6 +1622,7 @@ tokenized tokenizer tolerate tolerated +tolerates toplevel topological tops @@ -1629,6 +1637,7 @@ transforming translation traversal traverse +traverses traversing trees tricky