Skip to content

[Clang Importer] Get off of useSwift2Name and onto versions#6278

Merged
milseman merged 4 commits intoswiftlang:masterfrom
milseman:import_name_refactor
Dec 15, 2016
Merged

[Clang Importer] Get off of useSwift2Name and onto versions#6278
milseman merged 4 commits intoswiftlang:masterfrom
milseman:import_name_refactor

Conversation

@milseman
Copy link
Member

Lots of plumbing to scrap all the "useSwift2Name" bools scattered around and instead speak in terms of Swift versions. Some generalizations made that will also help with source compatibility.

Switch all of ImportName over to be version based, including
internally. NFC.
Renames some variables and methods to dictate specific versions of
Swift less, in preparation for deprecating Swift 3 names. NFC
@milseman
Copy link
Member Author

@swift-ci please test

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrose-apple what's a better name here? This, I guess, is not really the compiler version but could be something passed in?

"RequestedVersion"?

Copy link
Contributor

Choose a reason for hiding this comment

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

When in doubt, be explicit. SwiftVersionBeingCompiled vs. SwiftVersionToImport? With "isActive" as the predicate?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrose-apple Should be named markAsVariant instead, since we might be using this on newer names with older requested version?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrose-apple Will this be command-line configurable? That is, should it instead be a function from LangOpts to version? (alternatively, hosted on the Impl)? And, "RequestedVersion" is better name, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a -swift-version flag, and yes, this needs to follow it.

@jrose-apple
Copy link
Contributor

So, this still doesn't get us all the way to importing all three existing versions of a name (raw, Swift2, Swift3), right? Even before I add the Swift-3-vs.-Swift-4 support?

@milseman
Copy link
Member Author

Raw names are still confined to the special case of suppressing init formation. Getting all the raw names is still a todo for me.

@milseman
Copy link
Member Author

Better raw name support is a nice user experience to provide, but otherwise unrelated to source-compatibility, correct? Otherwise I can reprioritize.

@jrose-apple
Copy link
Contributor

Arguably correct, since the raw case will always be fully unavailable (not just deprecated) when it differs from the Swift 3 case. I think all the raw names I care about are covered by the Swift 2 name anyway—things like the top-level CoreGraphics functions.

@milseman milseman force-pushed the import_name_refactor branch from daa65c1 to dc4b268 Compare December 15, 2016 03:53
Remove all occurrences of a "useSwift2Name" bool, and replace it with
version plumbing. This means that ImportDecl is now entirely
version-based, and the importer Impl knows versions. This will be
needed for marking Swift 3 names as deprecated, when there is a new
Swift 4 name.

NFC.
@milseman milseman force-pushed the import_name_refactor branch from dc4b268 to 1e42a16 Compare December 15, 2016 03:55
@milseman
Copy link
Member Author

Right, the raw names were never valid names, but they're nice to recognize if someone copy-pasta-ed some ObjC code. I definitely want to get them soon, of course.

Now updated with better names, and uses the Swift version the user requested us to on the command line.

@milseman
Copy link
Member Author

@swift-ci please test

@slavapestov
Copy link
Contributor

What is a "Swift 2 name" and what are they used for?

@milseman
Copy link
Member Author

Getting tested at https://ci.swift.org/view/Pull%20Request/job/swift-PR-osx/4614/
Don't see the Linux one yet, but may be coming later

@milseman
Copy link
Member Author

It is the name of a foreign decl as it appeared in Swift 2. That means without any omit-needless-words transforms, import-as-member, etc. It does, however, have some of the transforms that were present in Swift 2 and earlier, such as import-as-init.

@jrose-apple
Copy link
Contributor

jrose-apple commented Dec 15, 2016

To elaborate, several weeks ago @milseman and I came up with several different "versions" an API may appear under. For something like the following declaration:

+ (instancetype)fooByWrappingBar:(Bar *)bar NS_SWIFT_NAME("init(containing:)");

where the NS_SWIFT_NAME was added only in Swift 4, we can identify four distinct names the user might try to access this under:

  • Raw: class func fooByWrappingBar(_: Bar) -> Self
  • Swift 1 / Swift 2: init(byWrappingBar: Bar)
  • Swift 3: init(wrapping: Bar)
  • Latest: init(containing: Bar)

and ideally trying to use any of the older names produces a more specific diagnostic than just "no such member". Today we only support the "Swift 2" and "Latest" cases, plus a few hard-coded "Raw" cases; we'd like to instead make this general.

We should probably write this down somewhere other than a PR and my office whiteboard.

@milseman
Copy link
Member Author

And to extend that further, if the user requested to use -swift-version 3, and they typed a Swift 4 name, we'd also like to recognize that and point them at the Swift 3 name (probably through a warning rather than error).

@milseman
Copy link
Member Author

@swift-ci please test

@milseman milseman merged commit 405fa6f into swiftlang:master Dec 15, 2016
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.

3 participants