Skip to content

[5.5] SIL: Private setters need at least hidden visibility for key paths in more cases. #38534

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

Conversation

jckarter
Copy link
Contributor

Explanation: Forming a key path to a public (or internal with -enable-testing) property with a private(set) setter from another file in the same module would hit linker errors, because the private setter's symbol was not exposed to the key path literals from other files. This was previously fixed for declarations with effective internal visibility, but would break when testing was enabled, or when the getter was public.

Issue: rdar://78523318

Testing: Test case from Radar; Swift CI

Reviewed by: @slavapestov

Risk: Low, generalizes an existing fix to cover more cases

Scope: Improves an existing bug fix. Some users are experiencing this as a regression because they enable testing and their code breaks again after receiving the initial fix.

… more cases.

My original fix only addressed the issue for when the property was exactly internal, so
we would still run into problems with keypaths and `private(set)` when `-enable-testing`
is on, or when referring to `public` properties with private setters from the same module.
This generalizes the rule, so that the setter entry point for any property with
at least internal visibility also has at least internal visibility, even if the setter
is semantically less visible. Fixes rdar://78523318.
@jckarter jckarter requested a review from a team as a code owner July 21, 2021 16:51
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@swift-ci Please nominate

@jckarter jckarter merged commit d5b0c2a into swiftlang:release/5.5 Jul 22, 2021
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