-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[5.7] Fix issues blocking enablement of -target-min-inlining-version min
#59176
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
tshortli
merged 14 commits into
swiftlang:release/5.7
from
tshortli:target-min-inlining-blockers-5.7
Jun 1, 2022
Merged
[5.7] Fix issues blocking enablement of -target-min-inlining-version min
#59176
tshortli
merged 14 commits into
swiftlang:release/5.7
from
tshortli:target-min-inlining-blockers-5.7
Jun 1, 2022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lability diagnostics when `-target-min-inlining-version min` is specified. Previously, an extension decl was always considered exported (externally visible to module clients) as long as it extended an exported type. Extensions need to either contain some externally visible member (e.g. a public method) or implement a conformance to a public protcol, though, to actually be exported. Without this fix, the compiler incorrectly requires internal extensions to types that are always available at runtime to have declared availability which would be a nuisance for library authors. As part of testing this change, I expanded the attr_inlinable_available.swift test case significantly and that prompted me to scrap the copy of the test specific to macCatalyst as it seemed like needing to keep the two tests in sync was going to be a liability going forward. I replaced the deleted test with a couple of more targeted tests that use `-dump-type-refinement-contexts` to verify the effect of `-target-min-inlining-version min` on the root refinement context. Again a macCatalyst version of the test is required because we don't have bots that are configured to make the macCatalyst runtime the "target" OS. Resolves rdar://91382040
…ailability_versions.swift test to distinguish between test cases for types that are only available starting in a certain release versus types that are marked strictly unavailable.
…xtension nominal type, superclass, etc.) on declarations that are explicitly unavailable. Resolves rdar://92179327
…ended type in an extension declaration when the following conditions are met: 1. The extension is missing explicit availability. 2. The required availability is before the deployment target. This exception is needed because there are many existing resilient libraries with extensions containing public members and no availability declared for the extension itself. Under existing rules, these decls are not diagnosed because the deployment target of the library is typically high enough that the extended type is implicitly considered available. Now that we're improving availability type checking for resilient libraries, however, these decls are being diagnosed. We need to land the diagnostic without breaking source compatibility so that library authors can identify and fix the issues. Resolves rdar://92621567
…f corner cases: - unavailable declarations and unavailable containers - SPI declarations and spi containers - property initializer expressions - property wrappers The tests document existing behavior of the availability checker, including several bugs. The FIXMEs in the test serve as a todo list of issues to resolve before the flag can be adopted widely.
…whether the decl is explicitly unavailable and the context is also unavailable. If those conditions are met, treat the decl as if it were always available since unavailable code is allowed to reference unavailable decls. Previously, `TypeChecker::checkDeclarationAvailability()` would behave this way for most explicitly unavailable decls by _accident_. An explicitly unavailable decl has no introduction version, and the existing logic therefore would fail to find a lower bound on the availability of the decl. The edge case that exposed the fragility of this logic was an unavailable extension containing a member with it's own explicit availability. The unavailability of the extension ought to trump the availability of the member, but the existing logic couldn't detect that. The compiler also ought to diagnose the conflicting availability annotations but I'd like to address that separately. Resolves rdar://92551870
…d unavailable API declarations with `-target-min-inlining-version min` specified. There's not much benefit to more accurate enforcement of availability in these decls since API clients can't use them and there's a lot of existing code that would be needlessly diagnosed without these exceptions. Resolves rdar://92716633
…text::Reason` enum to `APIBoundary`. I missed needing to rename this in swiftlang#58707.
…using the deployment target when the init would not be exposed to module clients. Without this, the initializers of public properties in API modules could be misdiagnosed as potentially unavailable to clients of the module, even though the expression will only ever execute on the deployment target or higher. While developing this fix I discovered a few other bugs that will need to be addressed separately: - The availability of the inferred type of a property is not diagnosed. Previously this mattered less since the initializer expression and the var declaration itself would both be diagnosed with the same availability. - The availability of the property wrapper type of a `VarDecl` appears to not be diagnosed at all. - Members of frozen structs with property wrappers result in broken swiftinterfaces because the synthesized storage is printed explicitly, confusing the diagnostics. Resolves rdar://92713589
…emove the OS versions for other platforms from availability attributes. Upcoming changes to the diagnostics make it difficult to verify correct behavior when only matching a fraction of a diagnostic, so the test needs to run on a single platform in order to have stable diagnostics to match. The `availability_refinement_contexts_target_min_inlining.swift` test case runs on all ABI stable platforms and verifies that an appropriate version floor is used for each, so running this test suite on multiple platforms is somewhat redundant.
…ce when is specified.
…ve clarity for authors of API libraries. When decls are diagnosed as potentially unavailable on an OS earlier than the deployment target, the message will now indicate that the issue would be faced by clients of the module. Resolves rdar://93466875
… availability of extensions. The primary motivation for this change is to reduce unnecessary availability diagnostics for API library authors. Many API libraries contain existing extension decls that lack declared availability where the extension introduces additional members to the extended type in the same release that the extended type was declared. Others contain extensions where the extension itself does not have declared availability but each of the members do. In both cases, the code is safe as written so the extra diagnostics would be a nuisance. Resolves rdar://93630782
…ype when `-target-min-inlining-version min` is specified. As a concession to source compatibility for API libraries, downgrade diagnostics about inheritance from a less available type when the following conditions are met: 1. The inherited type is only potentially unavailable before the deployment target. 2. The inheriting type is `@usableFromInline`. Resolves rdar://92747826
@swift-ci please test |
@swift-ci please test source compatibility |
nkcsgexi
approved these changes
May 31, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd like to take this to diagnose availability-related errors more precisely.
tbkka
approved these changes
May 31, 2022
I ran a baseline to see whether the source compatibility failures were expected and it seems like they are. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-pick of the following PRs to
release/5.7
:#42585
#58417
#58654
#58680
#58707
#58870
#58963
#59040
#59065
Resolves rdar://91382040&92179327&92551870&92621567&92713589&92716633&92747826&93466875&93630782