[MemberLoading] Split storage and non-storage member loading by j-hui · Pull Request #87065 · swiftlang/swift

j-hui · 2026-02-07T23:27:07Z

getStoredProperties() shouldn't call getMembers(), because can have the side effect of un-lazily loading everything. Instead, only ensure that storage-relevant members are loaded and iterate through those.

This patch set builds on #87026 (which merged separately) to avoid the import order from catastrophically messing up every single module interface test (which happened anyway; see the mongo diff there).

This patch set is necessary for the lazy importing work in #87016.

rdar://156949141

Restructure this function into a state-holding class with a recursive method, to make it easier to thread state around.

j-hui · 2026-02-07T23:27:18Z

@swift-ci please test

j-hui · 2026-02-08T00:05:15Z

Please ignore the changes in lib/AST/ASTPrinter.cpp and most of the test changes when reviewing this. The only test changes are:

test/Interop/Cxx/class/linked-records-module-interface.swift
test/Interop/Cxx/namespace/templates-with-forward-decl-module-interface.swift

where I had to reorder a couple of members.

(These ASTPrinter-related changes should disappear from the diff when #87026 / 9dc1174 lands)

Doing so allows further granularity when lazily loading members.

j-hui · 2026-02-08T01:18:01Z

@swift-ci please test

j-hui · 2026-02-08T08:07:24Z

@swift-ci please test

slavapestov · 2026-02-08T20:39:33Z

lib/Sema/TypeCheckStorage.cpp

    visitMember(actorSystemVar);
-  for (auto *member : implDecl->getMembers())
+  implDecl->loadStorageMembers();
+  for (auto *member : implDecl->getCurrentMembersWithoutLoading())


Should enumerateStoredPropertiesAndMissing() be unified with computeLoweredProperties() in some way?

Oh sure! I could do that if you think it'll benefit the code organization.

Ok so I took a crack at this. I think it's certainly possible to unify enumerateStoredPropertiesAndMissing() and InitializablePropertiesRequest::evaluate(), and perhaps computeLoweredProperties().

However, I'm a bit hesitant to land such a change in this patch. For one thing, I wanted to keep this diff small and focused on the functional change, which is the removal of getMembers().

The other thing is that there are slight discrepancies between these functions. For instance, InitializablePropertiesRequest::evaluate() directly iterates through the members of decl, whereas the others iterate through decl's implementation context. There's also that knownStoredProperties set that prevents enumerateStoredPropertiesAndMissing() from visiting members more than once. I think these differences should be inconsequential, but I'm not confident and I don't know how best to validate this without spending some more time reading code.

Would it be alright if I punt this unification to a follow-up PR?

slavapestov · 2026-02-08T20:41:04Z

lib/Sema/TypeCheckStorage.cpp

  };

-  for (auto *member : decl->getMembers())
+  decl->loadStorageMembers();


This looks like another spot where we duplicate visiting stored properties and dealing with property wrappers and so on.

slavapestov · 2026-02-08T20:42:29Z

lib/AST/DeclContext.cpp


-  for (auto *existingMember : getMembers()) {
+  addParsedMembers();
+  for (auto *existingMember : getCurrentMembersWithoutLoading()) {


It looks like addMemberPreservingSourceOrder() is only called in one place in the parser, can we assert here that getParentSourceFile() != nullptr? Or is it necessary to change this function at all? getMembers() should be the same as getParsedMembers() in this case.

I didn't realize this was only called by the parser, specifically for IDE-related tasks. Using the tests from #87016, I confirmed that this doesn't seem to affect the importedd Clang decls case I'm ultimately interested in.

I'm happy to back out of this change.

(And am happy to make this change in a follow-up if it makes sense.)

getStoredProperties() shouldn't call getMembers(), because can have the side effect of un-lazily loading everything. Instead, only ensure that storage-relevant members are loaded and iterate through those.

j-hui · 2026-02-09T07:50:55Z

@swift-ci please test

j-hui · 2026-02-09T16:24:33Z


Flaky Tests (1):
  Swift(windows-x86_64) :: Constraints/warn_long_compile.swift

Hmm, I don't think this is related to this.

j-hui · 2026-02-09T16:24:50Z

@swift-ci please test windows platform

j-hui · 2026-02-09T17:22:04Z

@swift-ci please test windows platform

Xazax-hun

Overall looks great, thanks!

Xazax-hun · 2026-02-09T18:24:18Z

lib/AST/DeclContext.cpp

-    /*lazyLoader=*/nullptr);
-  contextInfo->loader->loadAllMembers(const_cast<Decl *>(container),
-                                      contextInfo->memberData);
+  if (FirstDeclAndLazyMembers.getInt() == LazyMemberStatus::AllLoaded)


Would it make sense to move this check to the very front?

Behaviorally speaking, doing so shouldn't make a difference to whether we call loadStorageMembers(), so the only impact would be whether this would have the side effect of adding parsed members when AddedParsedMembers is false. Originally, calling getMembers() would always have that side effect, and what I am doing here is preserving that behavior.

Unfortunately at this time I don't know enough about the scenarios where AddedParsedMembers is false to determine when it is safe to not call it, so I'd prefer to defer that to a follow-up PR.

[cxx-interop] [NFC] Refactor loadAllMembersOfRecordDecl

812f479

Restructure this function into a state-holding class with a recursive method, to make it easier to thread state around.

j-hui marked this pull request as ready for review February 8, 2026 00:02

j-hui requested review from Xazax-hun, beccadax, egorzhdan, hborla, hnrklssn, ian-twilightcoder, slavapestov, susmonteiro, tshortli, xedin and xymus as code owners February 8, 2026 00:02

[MemberLoading] Split storage and non-storage member loading

5bd319a

Doing so allows further granularity when lazily loading members.

j-hui force-pushed the load-storage-members branch from 7a4c932 to 155b794 Compare February 8, 2026 01:17

j-hui force-pushed the load-storage-members branch from 155b794 to f765ff4 Compare February 8, 2026 08:07

slavapestov reviewed Feb 8, 2026

View reviewed changes

[MemberLoading] Only load storage members when that's all we need

3b4a298

getStoredProperties() shouldn't call getMembers(), because can have the side effect of un-lazily loading everything. Instead, only ensure that storage-relevant members are loaded and iterate through those.

j-hui force-pushed the load-storage-members branch from f765ff4 to 3b4a298 Compare February 9, 2026 07:50

j-hui requested a review from slavapestov February 9, 2026 07:51

Xazax-hun approved these changes Feb 9, 2026

View reviewed changes

j-hui merged commit 03530d6 into swiftlang:main Feb 10, 2026
6 checks passed

j-hui mentioned this pull request Feb 10, 2026

[cxx-interop] Import record members lazily #87016

Merged

Conversation

j-hui commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-hui commented Feb 7, 2026

Uh oh!

j-hui commented Feb 8, 2026

Uh oh!

j-hui commented Feb 8, 2026

Uh oh!

j-hui commented Feb 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-hui commented Feb 9, 2026

Uh oh!

j-hui commented Feb 9, 2026

Uh oh!

j-hui commented Feb 9, 2026

Uh oh!

j-hui commented Feb 9, 2026

Uh oh!

Xazax-hun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

j-hui commented Feb 7, 2026 •

edited

Loading