Skip to content

[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

Merged
merged 4 commits into from
Dec 2, 2016

Conversation

milseman
Copy link
Member

[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.

/// Retrieve the alternative declaration for the given imported
/// Swift declaration.
ValueDecl *getAlternateDecl(Decl *decl) {
TinyPtrVector<ValueDecl *> getAlternateDecls(Decl *decl) {
Copy link
Contributor

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)
Copy link
Contributor

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…

Copy link
Member Author

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?

Copy link
Contributor

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.

@milseman
Copy link
Member Author

milseman commented Dec 1, 2016

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 1, 2016

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 00deb44ce1d9a247f30a211ebb92e3dd7a19ea60
Test requested by - @milseman

@swift-ci
Copy link
Contributor

swift-ci commented Dec 1, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 00deb44ce1d9a247f30a211ebb92e3dd7a19ea60
Test requested by - @milseman

@milseman milseman force-pushed the import_name_refactor_wip branch from 00deb44 to 988c4f0 Compare December 2, 2016 00:37
@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2016

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 00deb44ce1d9a247f30a211ebb92e3dd7a19ea60
Test requested by - @milseman

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 00deb44ce1d9a247f30a211ebb92e3dd7a19ea60
Test requested by - @milseman

@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 988c4f09f0fc077e3d4245fbe24af4e476c5dba6
Test requested by - @milseman

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.
@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

My branch was out of date, rebasing and fixing now

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.
@milseman milseman force-pushed the import_name_refactor_wip branch from 988c4f0 to d1efc80 Compare December 2, 2016 02:51
@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci please test

1 similar comment
@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci please test

@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci thank you

@swift-ci
Copy link
Contributor

swift-ci commented Dec 2, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - d1efc80
Test requested by - @milseman

@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

Failure is probably un-related: https://ci.swift.org/job/swift-PR-osx/4468/consoleFull

19:12:52 2  swiftc                   0x000000010cdcb9ea swift::DeclContext::lookupQualified(swift::Type, swift::DeclName, swift::NLOptions, swift::LazyResolver*, llvm::SmallVectorImpl<swift::ValueDecl*>&) const + 58

@slavapestov do you know why that is failing like that?

@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

Awaiting #6020, then testing again

@gparker42
Copy link
Contributor

It's a bug in #5990. We're testing a fix in #6020.

@gparker42
Copy link
Contributor

@swift-ci please test OS X platform

@milseman
Copy link
Member Author

milseman commented Dec 2, 2016

@swift-ci please test

@milseman milseman merged commit 82f0a19 into swiftlang:master Dec 2, 2016
@milseman milseman deleted the import_name_refactor_wip branch December 2, 2016 18:29
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.

4 participants