-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Import Decl] Refactoring ObjCMethod importing #5991
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
/// Retrieve the alternative declaration for the given imported | ||
/// Swift declaration. | ||
ValueDecl *getAlternateDecl(Decl *decl) { | ||
TinyPtrVector<ValueDecl *> getAlternateDecls(Decl *decl) { |
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.
Suggestion: ArrayRef.
// Note: struct name matching; don't drop "With". | ||
// CHECK-FOUNDATION: class func withRange(_: NSRange) -> NSValue | ||
// Note: Init formed, no withRange member | ||
// CHECK-FOUNDATION: /*not inherited*/ init!(range: NSRange) |
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.
I'm pretty sure we still want these test cases in some form…
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.
I agree. What exactly is the rule here that they're using? Is it the return struct name, in which case how can I coerce it into not wanting to be an init?
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.
You could use API notes to mark these things as methods-not-constructors using the old FactoryAsInit annotation, but I think it's probably good enough just to check some methods that don't return the Self type instead, or to use instance methods instead of class methods.
@swift-ci please test |
Build failed |
Build failed |
00deb44
to
988c4f0
Compare
@swift-ci please test |
Build failed |
Build failed |
@swift-ci please test |
Build failed |
Adds assertion that we're not clobbering existing alternative decls, and removes all naked accesses. NFC.
ImportDecl has an alternate decl quasi-escape-hatch where it can create alternative declarations for the same original Clang node. But, it was limited at one, which worked accidentally in the case of VisitObjCMethodDecl's attempts at importing a factory-init-suppressed root-class method that points to the imported-as-init Swift declaration. This commit changes that to a TinyPtrVector, to support the upcoming occasional case where we have more.
Actually, that new test I added isn't ready yet. It requires a real "Raw" mode that will ignore the swift_name attributes. I should drop it. |
Previously, for an Objective-C class method declaration that could be imported as init, we were making 4 decls: 1) The Swift 2 init 2) The Swift 2 class method decl (suppressing init formation) 3) The Swift 3 init (omitting needless words) 4) The Swift 3 class method decl (suppressing init formation and omitting needless words) Decls 1), 2), and 4) exist for diagnostics and redirect the user at 3). But, 4) does not correspond to any actual Swift version name and producing it correctly would require the user to understand how omit-needless-words and other importer magic operates. It provides very limited value and more importantly gets in the way of future Clang importer refactoring. We’d like to turn Decl importing into something that is simpler and language-version parameterized, but there is no real Swift version to correspond to decl 4). Therefore we will be making the following decls: 1) The "raw" decl, the name as it would appear to the user if they copy-pasted Objective-C code 2) The name as it appeared in Swift 2 (which could be an init) 3) The name as it appeared in Swift 3 (which could be an init and omit needless words) This aligns with the language versions we want to import as in the future: raw, swift2, swift3, …, and current. Note that swift-ide-test prunes decls that are unavailable in the current Swift version, so the Swift 2 non-init decls are not printed out, though they are still present. Tests were updated and expanded to ensure this was still the case.
988c4f0
to
d1efc80
Compare
@swift-ci please test |
1 similar comment
@swift-ci please test |
@swift-ci thank you |
Build failed |
Failure is probably un-related: https://ci.swift.org/job/swift-PR-osx/4468/consoleFull
@slavapestov do you know why that is failing like that? |
Awaiting #6020, then testing again |
@swift-ci please test OS X platform |
@swift-ci please test |
[Import Decl] Don’t import as init using omit needless words
Previously, for an Objective-C class method declaration that could be
imported as init, we could make 4 decls, which included a class method decl using omit-needless-words. But, this was never a valid name anyways, nor is it easy for users to divine the magic omit-needless-words transformations that are done. So, we drop and treat the Swift 2 non-init name as a "raw" name, which is the name as it appears from ObjC. This will align nicely with future ImportName refactoring, where ImportDecl becomes parameterized based on language version, or otherwise is passed the name ahead of time.