Skip to content

Implement canImport test for submodule - #34094

Merged
CodaFi merged 1 commit into
swiftlang:mainfrom
ApolloZhu:fix-canImport-submodule
Jan 4, 2022
Merged

Implement canImport test for submodule#34094
CodaFi merged 1 commit into
swiftlang:mainfrom
ApolloZhu:fix-canImport-submodule

Conversation

@ApolloZhu

@ApolloZhu ApolloZhu commented Sep 26, 2020

Copy link
Copy Markdown
Contributor

This PR extends canImport to check for submodule availability.

Forums Thread: https://forums.swift.org/t/41196

Motivation

Currently, canImport only supports checking for top-level module availability while many have submodules with different availability. For example, while CIFilter was introduced in iOS 5, CIFilterBuiltins is only available on iOS 13+. Therefore, one might want to write code like this:

#if canImport(CoreImage.CIFilterBuiltins)
    ^ Unexpected platform condition argument: expected identifier

import CoreImage.CIFilterBuiltins
...
#endif

Changes needed for The Swift Programming Language Reference

This PR would mean:

+ module-name → import-path
- module-name → identifier

For housekeeping, because import OpenGL.(GL3, GL3.Ext) is (likely) never implemented, unless we're planning to have a separate PR extending import to support operators (e.g. import func SomeModule.+++) as mentioned in #34094 (comment), we should update:

+ import-path → identifier | identifier . import-path
- import-path → import-path-identifier | import-path-identifier . import-path
- import-path-identifier → identifier | operator 

Alternatives Considered

While the initial concern could be addressed by introducing #if availablesimiliar to how @available/#available works but at the top level, clang modules may specify other requirements, therefore allowing canImport to check for submodule availability directly should be a more well-rounded and direct approach.

Questions/Concerns

  • Is this a bug fix or does it have to go through Swift Evolution? According to the original proposal SE-0075, "#if canImport(module-name) tests for module support by name." Are submodule names module names?
  • Current implementation only extends support to clang submodules. I saw a few FIXMEs in other module loaders mentioning submodule support is not in place yet. Does this change have to wait for those to be implemented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now emits diagnostics at the module name instead canImport

#if canImport(NotExist)
-   ^
+             ^

Comment thread lib/ClangImporter/ClangImporter.cpp Outdated
Comment thread test/Parse/ConditionalCompilation/can_import_submodule_missing_requirement.swift Outdated
@ApolloZhu
ApolloZhu requested a review from CodaFi September 27, 2020 12:17
Comment thread include/swift/AST/Import.h Outdated

@CodaFi CodaFi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the implementation and a request: please give this a rebase.

Thank you for tackling this.

@ApolloZhu
ApolloZhu requested a review from CodaFi October 14, 2020 14:56
@ApolloZhu

Copy link
Copy Markdown
Contributor Author

Thanks for pointing that out! Fixed

@CodaFi CodaFi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks great.

@CodaFi

CodaFi commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

Please go to swift-evolution and ask if this is a feature or a bug fix. I have confidence that they'll greenlight this, but it's always good to get the community's stamp.

@CodaFi CodaFi added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Oct 14, 2020
@ApolloZhu

Copy link
Copy Markdown
Contributor Author

Will ask in the forum. Thanks for all the suggestions

@ApolloZhu

Copy link
Copy Markdown
Contributor Author

@amartini51

Copy link
Copy Markdown
Member

Thanks for calling out the impact on the reference and grammar of "The Swift Programming Language". I'll follow this PR so I can update the documentation as needed.

The grammar allows operators in an import is probably for the form that allows you to specify a specific declaration to import. (Git history shows me that grammar rule dates back to the original Swift 1 version of the book.) For example, something like import func SomeModule.+++ would import only the function that implements the +++ operator from SomeModule. However, that form of import doesn't appear to actually work, and I'm unsure whether it was ever actually part of the language's implementation.

@ApolloZhu

Copy link
Copy Markdown
Contributor Author

So far the community response agrees this is a simple bug fix and no further changes need to be made. What shall we do next?

@ApolloZhu

ApolloZhu commented Feb 6, 2021

Copy link
Copy Markdown
Contributor Author

I know the community is busy discussing new concurrency proposals, but should I submit/pitch a simple Swift Evolution proposal for this change as well?

@CodaFi

CodaFi commented Jun 10, 2021

Copy link
Copy Markdown
Contributor

@ApolloZhu

Please rebase this when you can and tag me here and we'll get this merged.

@ApolloZhu
ApolloZhu force-pushed the fix-canImport-submodule branch from d08b713 to ac56a4e Compare July 6, 2021 02:13
@ApolloZhu

ApolloZhu commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

I updated from main and squashed all commits into one (since there were too many merge commits in between). Ready for review by @CodaFi.


Since our last review of this PR, @nkcsgexi introduced a version check in canImport. Based on their current implementation and a few .tbd files I viewed, it seems that submodules don't really have a version. Assuming my observation is correct, we have to consider what to do about:

#if canImport(A.B.C, _version: 3)

#if canImport(A.B.C, _underlyingVersion: 4)

Two solutions:

  • (Current approach) It type checks, checks if submodule exists, and validates the version of the top module.
  • It should NOT type check, and should instead be expressed as:
    #if canImport(A, _version: 3) && canImport(A.B.C)
  • Sweep this under the rug and decide when _version and _underlyingVersion goes into public

@ApolloZhu
ApolloZhu requested a review from CodaFi July 6, 2021 02:28
@nkcsgexi

nkcsgexi commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

@ApolloZhu thanks for adding the support for submodules! Though we usually don't have submodule-specific module versions, can we use the top-level module version in the case of sub-modules?

@ApolloZhu

ApolloZhu commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

I'm pretty sure https://github.com/apple/swift/pull/34094/files#diff-0062ad3fe872d09412bd3e7c72610b5ad0bd39667ebc00695351ef93c7c6815f already does this. Please let me know if that's not the case and I'd appreciated some help getting starting on fixing that.

@nkcsgexi

nkcsgexi commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

@ApolloZhu You are right. Thanks for clarifying!

@ApolloZhu

ApolloZhu commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

I see this PR is expecting some tests to pass. Who (and when) can initiate those tests?

@xwu

xwu commented Aug 16, 2021

Copy link
Copy Markdown
Collaborator

@swift-ci test

@ApolloZhu
ApolloZhu force-pushed the fix-canImport-submodule branch from ac56a4e to 683d469 Compare December 29, 2021 08:35
@ApolloZhu

Copy link
Copy Markdown
Contributor Author
  1. Fixed merge conflict so this can be merged/tested/reviewed again
  2. It would be great if we can remove the swift-evolution-pending-discussion label to reflect the confirmation in the earlier comment Implement canImport test for submodule #34094 (comment)
  3. Please let me know if there are additional things I should do to get this merged
    • We did briefly discuss the potential of implementing code completion for canImport submodule during WWDC, but as there exists no current implementation for canImport top level module completion, I believe that can/should be done in a separate PR

@xwu xwu removed the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Dec 29, 2021
@xwu

xwu commented Dec 29, 2021

Copy link
Copy Markdown
Collaborator

Pinging @CodaFi as they requested. Tag removed.

@xwu

xwu commented Dec 29, 2021

Copy link
Copy Markdown
Collaborator

@swift-ci smoke test

@CodaFi

CodaFi commented Dec 29, 2021

Copy link
Copy Markdown
Contributor

Brilliant - still LGTM. Will merge once the bots come back blue.

@CodaFi

CodaFi commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

@swift-ci smoke test

@ApolloZhu

Copy link
Copy Markdown
Contributor Author

Great, all checks have passed

@CodaFi

CodaFi commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

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.

5 participants