Skip to content

[5.9][CodeCompletion] Fix an issue that causes an iterator to be invalidated while iterating #66559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2023
Merged

[5.9][CodeCompletion] Fix an issue that causes an iterator to be invalidated while iterating #66559

merged 1 commit into from
Jun 13, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jun 12, 2023

  • Explanation: While iterating top level declarations to generate code completion results, we were triggering additional macro expansions, which caused new top-level results to get generated, invalidating the iterator and causing undefined memory access. Store the top-level declarations into a temporary data structure before reporting them, decoupling the iteration of top-level values and potentially triggered, removing the potential for undefined memory access.
  • Scope: Undefined memory access using code completion when the source file contained an expanded conformance macro
  • Risk: We are storing all top-level results of the current module in a temporary data structure which could result in regressed performance if the module is very large but I doubt it
  • Testing: I found a local reproducer using the macOS SDK and made sure that it doesn’t reproduce anymore with the fix.
  • Issue: rdar://109202157
  • Reviewer: @bnbarham on [CodeCompletion] Fix an issue that causes an iterator to be invalidated while iterating #66517

@ahoppen
[CodeCompletion] Fix an issue that causes an iterator to be invalidat…
2028274 
…ed while iterating

If we have 'addinitstotoplevel' enabled, calling `foundDecl` on `this` can cause macros to get expanded, which can then cause new members ot get added to 'TopLevelValues', invalidating iterator over `TopLevelDecls` in `SourceLookupCache::lookupVisibleDecls`.

Technically `foundDecl` should not expand macros or discover new top level members in any way because those newly discovered decls will not be added to the code completion results. However, it's preferrable to miss results than to silently invalidate a collection, resulting in hard-to-diagnose crashes.
Thus, store all the decls found by `CurrModule->lookupVisibleDecls` in a vector first and only call `this->foundDecl` once we have left the iteration loop over `TopLevelDecls`.

I have not been able to reduce this to a test case that doesn’t rely on the `Observation` module in the SDK but here is the test case with which I was able to reproduce the issue very reliably.

```swift
import Foundation
import Observation

@observable class MyObject {}

extension MyObject {}

// RUN: ~/sbin/sourcekitd-test \
// RUN:   -req=complete.open \
// RUN:   -req-opts=addinitstotoplevel=1 \
// RUN:   -pos=8:1 \
// RUN:   %s \
// RUN:   -- \
// RUN:   %s \
// RUN:   -Xfrontend \
// RUN:   -load-plugin-library \
// RUN:   -Xfrontend \
// RUN:   /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/host/plugins/libObservationMacros.dylib \
// RUN:   -sdk \
// RUN:   /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk
```

rdar://109202157
@ahoppen ahoppen requested a review from a team as a code owner June 12, 2023 16:24
@ahoppen
Copy link
Member Author

ahoppen commented Jun 12, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 12, 2023

@swift-ci Please test macOS

@ahoppen ahoppen merged commit 31dd2ad into swiftlang:release/5.9 Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@airspeedswift airspeedswift airspeedswift approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ahoppen @airspeedswift