Skip to content

Commit 2fcfdd7

Browse files
scheglovCommit Queue
authored andcommitted
Fine. Avoid opaque API uses when lazily loading class members.
Lazy loading of a class’s members inadvertently recorded opaque API uses via `ClassElementImpl.fragments`/`firstFragment`. This polluted result requirements with non-API dependencies and could trigger unnecessary rebuilds when reading from cached bundles. This change removes those spurious dependencies: - Add `ClassElementImpl._fragments` to iterate fragments without touching the tracked `fragments` getter. - Add `ClassElementImpl.ensureReadMembersForFragments()` and use it in `BundleReader` when materializing a class’s members, avoiding any dependency on `fragments` during deserialization. - Simplify `LinkedElementFactory` by calling `parentElement.ensureReadMembers()` instead of poking constructor lists to force member loading. - Keep the public `fragments` getter behavior (including opaque tracking) for true API reads; internal reads now use the private helper. A regression test exercises a scenario where class `B` references `A` in a default parameter. Linking after a cache reload no longer records opaque uses of `A`/`B` members, preserving dependency precision. Found with https://dart-review.googlesource.com/c/sdk/+/446480 Change-Id: Ib8d2bea359956d1a24b1117bcb6840dfb3a68253 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/446561 Reviewed-by: Paul Berry <[email protected]> Reviewed-by: Johnni Winther <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>
1 parent a84147a commit 2fcfdd7

File tree

5 files changed

+187
-17
lines changed

5 files changed

+187
-17
lines changed

pkg/analyzer/lib/src/dart/element/element.dart

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -214,14 +214,7 @@ class ClassElementImpl extends InterfaceElementImpl implements ClassElement {
214214
@trackedDirectlyOpaque
215215
List<ClassFragmentImpl> get fragments {
216216
globalResultRequirements?.recordOpaqueApiUse(this, 'fragments');
217-
return [
218-
for (
219-
ClassFragmentImpl? fragment = _firstFragment;
220-
fragment != null;
221-
fragment = fragment.nextFragment
222-
)
223-
fragment,
224-
];
217+
return _fragments;
225218
}
226219

227220
@trackedIndirectly
@@ -362,6 +355,17 @@ class ClassElementImpl extends InterfaceElementImpl implements ClassElement {
362355
@trackedIncludedInId
363356
ElementKind get kind => ElementKind.CLASS;
364357

358+
List<ClassFragmentImpl> get _fragments {
359+
return [
360+
for (
361+
ClassFragmentImpl? fragment = _firstFragment;
362+
fragment != null;
363+
fragment = fragment.nextFragment
364+
)
365+
fragment,
366+
];
367+
}
368+
365369
@override
366370
@trackedDirectlyOpaque
367371
T? accept<T>(ElementVisitor2<T> visitor) {
@@ -381,6 +385,12 @@ class ClassElementImpl extends InterfaceElementImpl implements ClassElement {
381385
builder.writeClassElement(this);
382386
}
383387

388+
void ensureReadMembersForFragments() {
389+
for (var fragment in _fragments) {
390+
fragment.ensureReadMembers();
391+
}
392+
}
393+
384394
@Deprecated('Use isExtendableOutside instead')
385395
@override
386396
@trackedIndirectly

pkg/analyzer/lib/src/summary2/bundle_reader.dart

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -272,16 +272,16 @@ class LibraryReader {
272272
_lazyRead((offset) {
273273
element.deferReadMembers(() {
274274
_reader.runAtOffset(offset, () {
275-
for (var fragment in element.fragments) {
276-
fragment.ensureReadMembers();
277-
}
278-
275+
element.ensureReadMembersForFragments();
279276
element.fields = _readFieldElements();
280277
element.getters = _readGetterElements();
281278
element.setters = _readSetterElements();
282279
_readVariableGetterSetterLinking();
283280
element.methods = _readMethodElements();
284-
if (!element.isMixinApplication) {
281+
if (element.isMixinApplication) {
282+
// Create synthetic constructors and associate with references.
283+
element.constructors;
284+
} else {
285285
element.constructors = _readConstructorElements();
286286
}
287287
});

pkg/analyzer/lib/src/summary2/linked_element_factory.dart

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,7 @@ class LinkedElementFactory {
144144

145145
// Only classes delay creating children.
146146
if (parentElement is ClassElementImpl) {
147-
var firstFragment = parentElement.firstFragment;
148-
// TODO(scheglov): directly ask to read all?
149-
firstFragment.constructors;
150-
parentElement.constructors;
147+
parentElement.ensureReadMembers();
151148
}
152149

153150
var element = reference.element;

pkg/analyzer/test/src/dart/analysis/driver_test.dart

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55949,6 +55949,105 @@ int get b => 0;
5594955949
);
5595055950
}
5595155951

55952+
test_precision_noOpaqueApiUse() async {
55953+
// Lowest layer: `A` is used by `B`.
55954+
var a = newFile('$testPackageLibPath/a.dart', r'''
55955+
class A {
55956+
const A();
55957+
}
55958+
''');
55959+
55960+
// Middle layer: `B` references `A` in a default value.
55961+
newFile('$testPackageLibPath/b.dart', r'''
55962+
import 'a.dart';
55963+
55964+
class B {
55965+
const B([Object? x = const A()]);
55966+
}
55967+
''');
55968+
55969+
configuration
55970+
..withSchedulerStatus = false
55971+
..withGetLibraryByUri = false
55972+
..withLinkBundleEvents = true
55973+
..withLibraryManifest = true
55974+
..withResultRequirements = true;
55975+
55976+
var driver = driverFor(testFile);
55977+
var collector = DriverEventCollector(driver, idProvider: idProvider);
55978+
55979+
// 1) Link `a.dart` and `b.dart`.
55980+
collector.getLibraryByUri('B1', 'package:test/b.dart');
55981+
await assertEventsText(collector, r'''
55982+
[operation] linkLibraryCycle SDK
55983+
[operation] linkLibraryCycle
55984+
package:test/a.dart
55985+
declaredClasses
55986+
A: #M0
55987+
interface: #M1
55988+
requirements
55989+
[operation] linkLibraryCycle
55990+
package:test/b.dart
55991+
declaredClasses
55992+
B: #M2
55993+
interface: #M3
55994+
requirements
55995+
libraries
55996+
package:test/a.dart
55997+
exportedTopLevels
55998+
A: #M0
55999+
Object: <null>
56000+
interfaces
56001+
A
56002+
requestedConstructors
56003+
new: #M4
56004+
''');
56005+
56006+
// 2) Touch the lowest layer: unload both `a.dart` and `b.dart`.
56007+
modifyFile2(a, r'''
56008+
class A {
56009+
const A(); // comment
56010+
}
56011+
''');
56012+
driver.changeFile2(a);
56013+
56014+
// 3) Top layer: `test.dart` depends on `B`, and so `A` transitively.
56015+
newFile(testFile.path, r'''
56016+
import 'b.dart';
56017+
final v = B();
56018+
''');
56019+
56020+
// Link `test.dart`. During linking, we will need formal parameters of the
56021+
// constructor `B()`, which will trigger loading resolution data for its
56022+
// formal parameter `x = const A()`. This will require loading members,
56023+
// specifically constructors, of `A`.
56024+
//
56025+
// There was a bug when loading members of `A` and `B` caused recording
56026+
// opaque API use (`firstFragment` and `fragments`).
56027+
collector.getLibraryByUri('T1', 'package:test/test.dart');
56028+
await assertEventsText(collector, r'''
56029+
[operation] readLibraryCycleBundle
56030+
package:test/a.dart
56031+
[operation] readLibraryCycleBundle
56032+
package:test/b.dart
56033+
[operation] linkLibraryCycle
56034+
package:test/test.dart
56035+
declaredGetters
56036+
v: #M5
56037+
declaredVariables
56038+
v: #M6
56039+
requirements
56040+
libraries
56041+
package:test/b.dart
56042+
exportedTopLevels
56043+
B: #M2
56044+
interfaces
56045+
B
56046+
requestedConstructors
56047+
new: #M7
56048+
''');
56049+
}
56050+
5595256051
test_req_classElement_noName() async {
5595356052
newFile(testFile.path, r'''
5595456053
class {}

pkg/analyzer/test/src/summary/elements/class_test.dart

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19065,6 +19065,70 @@ library
1906519065
''');
1906619066
}
1906719067

19068+
test_classAlias_constructors_reading() async {
19069+
newFile('$testPackageLibPath/a.dart', r'''
19070+
mixin M {}
19071+
19072+
class A {
19073+
const A.named();
19074+
}
19075+
19076+
class B = A with M;
19077+
''');
19078+
19079+
var library = await buildLibrary('''
19080+
import 'a.dart';
19081+
const x = B.named();
19082+
''');
19083+
checkElementText(library, r'''
19084+
library
19085+
reference: <testLibrary>
19086+
fragments
19087+
#F0 <testLibraryFragment>
19088+
element: <testLibrary>
19089+
libraryImports
19090+
package:test/a.dart
19091+
topLevelVariables
19092+
#F1 hasInitializer x (nameOffset:23) (firstTokenOffset:23) (offset:23)
19093+
element: <testLibrary>::@topLevelVariable::x
19094+
initializer: expression_0
19095+
InstanceCreationExpression
19096+
constructorName: ConstructorName
19097+
type: NamedType
19098+
name: B @27
19099+
element2: package:test/a.dart::@class::B
19100+
type: B
19101+
period: . @28
19102+
name: SimpleIdentifier
19103+
token: named @29
19104+
element: package:test/a.dart::@class::B::@constructor::named
19105+
staticType: null
19106+
element: package:test/a.dart::@class::B::@constructor::named
19107+
argumentList: ArgumentList
19108+
leftParenthesis: ( @34
19109+
rightParenthesis: ) @35
19110+
staticType: B
19111+
getters
19112+
#F2 synthetic x (nameOffset:<null>) (firstTokenOffset:<null>) (offset:23)
19113+
element: <testLibrary>::@getter::x
19114+
topLevelVariables
19115+
const hasInitializer x
19116+
reference: <testLibrary>::@topLevelVariable::x
19117+
firstFragment: #F1
19118+
type: B
19119+
constantInitializer
19120+
fragment: #F1
19121+
expression: expression_0
19122+
getter: <testLibrary>::@getter::x
19123+
getters
19124+
synthetic static x
19125+
reference: <testLibrary>::@getter::x
19126+
firstFragment: #F2
19127+
returnType: B
19128+
variable: <testLibrary>::@topLevelVariable::x
19129+
''');
19130+
}
19131+
1906819132
test_classAlias_constructors_requiredParameters() async {
1906919133
var library = await buildLibrary('''
1907019134
class A<T extends num> {

0 commit comments

Comments
 (0)