Skip to content

Requestify AbstractStorageDecl::hasStorage(). #66529

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.

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

Merged

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Jun 10, 2023

  • Explanation: The hasStorage() computation is used in many places to determine the signatures of other declarations. It currently needs to expand accessor macros, which causes a number of cyclic references. Provide a simplified request to determine hasStorage without expanding or resolving macros, breaking a common pattern of cycles when using macros. This requires some level of additional documentation for accessor macros to say what accessors they could generate, because we need to know whether a given accessor macro will make a property computed (or not) without expanding it. There is a small-but-necessary language adjustment to be paired with this.
  • Scope: Fairly broad: the hasStorage() computation is core to determining whether a particular property is stored or not. Its value was determined based on a heavier-weight computation of the access pattern for the property, which required creating all accessors in advance.
  • Risk: Moderate: it is plausible that the hasStorage computation could be out-of-sync with the access-pattern computation in some corner case not exercised by any code we've tested, causing a regression. The hasStorage computation is so central to everything in the compiler that this seems unlikely.
  • Reviewed by: @hborla
  • Issue: rdar://109668383
  • Original pull request: Requestify AbstractStorageDecl::hasStorage(). #66482

@DougGregor DougGregor requested a review from a team as a code owner June 10, 2023 06:30
@DougGregor
Copy link
Member Author

@swift-ci please test

The `hasStorage()` computation is used in many places to determine the
signatures of other declarations. It currently needs to expand accessor
macros, which causes a number of cyclic references. Provide a
simplified request to determine `hasStorage` without expanding or
resolving macros, breaking a common pattern of cycles when using
macros.

Fixes rdar://109668383.
… impl info

Slightly cleanup, and make an assertion less strict in the face of invalid
code.
Now that we've made accessor macro expansion more lazy, ensure that
when querying for init accessors (e.g., to build a memberwise
initializer), we also expand any accessor macros that might produce an
init accessor.

This is a partial step toward the real goal, which is that
`AbstractStorageDecl::getAccessor()` should lazily expand macros if
needed.

Update the Observable macro to document that it produces an `init`
accessor.
Remove an early iteration of cycle-breaking in `PatternBindingEntryRequest`
that has been subsumed by the lazy computation of
`AbstractStorageDecl::hasStorage()`. We can now directly use
`hasStorage()` here.
@DougGregor DougGregor force-pushed the requestify-has-storage-5.9 branch from 8702fe7 to beeadb2 Compare June 11, 2023 18:01
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test Windows

@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

Prior test run looks like it tripped over some CI issues, trying again

@DougGregor
Copy link
Member Author

@swift-ci please test Windows

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please test Windows

@DougGregor DougGregor merged commit 34f78df into swiftlang:release/5.9 Jun 12, 2023
@DougGregor DougGregor deleted the requestify-has-storage-5.9 branch June 12, 2023 20:22
QuietMisdreavus added a commit that referenced this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants