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

Import lightweight objc generics #1816

Merged
merged 16 commits into from Mar 28, 2016

Conversation

Projects
None yet
7 participants
@jckarter
Member

jckarter commented Mar 23, 2016

Support for importing ObjC generics as generic classes in Swift.

cc @cwillmor

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 23, 2016

Member

@swift-ci Please test

Member

jckarter commented Mar 23, 2016

@swift-ci Please test

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 24, 2016

Collaborator

Great to see this progress Joe!

Collaborator

lattner commented Mar 24, 2016

Great to see this progress Joe!

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 24, 2016

Member

@cwillmor and @jrose-apple did all the hard work.

Member

jckarter commented Mar 24, 2016

@cwillmor and @jrose-apple did all the hard work.

@slavapestov

This comment has been minimized.

Show comment
Hide comment
@slavapestov

slavapestov Mar 24, 2016

Member

@jckarter Can you make the patch history linear or were there tricky merge conflicts etc?

Member

slavapestov commented Mar 24, 2016

@jckarter Can you make the patch history linear or were there tricky merge conflicts etc?

ClangImporter::Implementation::shouldSuppressGenericParamsImport(
const clang::ObjCInterfaceDecl *decl) {
while (decl) {
StringRef name = decl->getName();

This comment has been minimized.

@slavapestov

slavapestov Mar 24, 2016

Member

Better to compare identifiers instead?

@slavapestov

slavapestov Mar 24, 2016

Member

Better to compare identifiers instead?

bodyParams.push_back(ParameterList::createWithoutLoc(selfVar));
Type selfInterfaceType;
if (dc->getAsProtocolOrProtocolExtensionContext()) {

This comment has been minimized.

@slavapestov

slavapestov Mar 24, 2016

Member

These four lines appear in many places -- it would be awesome if you factored it out

@slavapestov

slavapestov Mar 24, 2016

Member

These four lines appear in many places -- it would be awesome if you factored it out

Show outdated Hide outdated lib/ClangImporter/ImportDecl.cpp
if (auto proto = dyn_cast<ProtocolDecl>(dc)) {
std::tie(type, interfaceType)
= getProtocolMethodType(proto, type->castTo<AnyFunctionType>());
} else if (dc->getGenericParamsOfContext()) {

This comment has been minimized.

@slavapestov

slavapestov Mar 24, 2016

Member

This is going away, use isGenericContext()

@slavapestov

slavapestov Mar 24, 2016

Member

This is going away, use isGenericContext()

Show outdated Hide outdated lib/ClangImporter/ImportDecl.cpp
@@ -3479,6 +3493,17 @@ namespace {
result->setInitializerInterfaceType(interfaceInitType);
result->setInterfaceType(interfaceAllocType);
} else if (dc->getGenericParamsOfContext()) {

This comment has been minimized.

@slavapestov

slavapestov Mar 24, 2016

Member

isGenericContext()

@slavapestov

slavapestov Mar 24, 2016

Member

isGenericContext()

Show outdated Hide outdated lib/SILGen/SILGenBridging.cpp
case ParameterConvention::Indirect_In_Guaranteed:
llvm_unreachable("indirect args in foreign thunked method not implemented");
{
GenericContextScope genericScope(SGM.Types,

This comment has been minimized.

@slavapestov

slavapestov Mar 24, 2016

Member

Is this necessary? You're using mapTypeIntoContext() already below

@slavapestov

slavapestov Mar 24, 2016

Member

Is this necessary? You're using mapTypeIntoContext() already below

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 24, 2016

Collaborator

Awesome, great to see this progress from everyone!

Collaborator

lattner commented Mar 24, 2016

Awesome, great to see this progress from everyone!

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 24, 2016

Member

@slavapestov It was like that when I got here, so I don't know the background, sorry. @cwillmor or @jrose-apple , are you able to linearize the history here?

Member

jckarter commented Mar 24, 2016

@slavapestov It was like that when I got here, so I don't know the background, sorry. @cwillmor or @jrose-apple , are you able to linearize the history here?

@slavapestov

This comment has been minimized.

Show comment
Hide comment
@slavapestov

slavapestov Mar 24, 2016

Member

Just try doing 'git rebase origin/master' and see if each patch along the way compiles? I could give it a shot if you want...

Member

slavapestov commented Mar 24, 2016

Just try doing 'git rebase origin/master' and see if each patch along the way compiles? I could give it a shot if you want...

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 24, 2016

Member

@swift-ci Please smoke test

Member

jckarter commented Mar 24, 2016

@swift-ci Please smoke test

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 25, 2016

Member

@swift-ci Please smoke test

Member

jckarter commented Mar 25, 2016

@swift-ci Please smoke test

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 25, 2016

Member

@parkera Is this failure in Foundation expected on Linux?

Foundation/NSOperation.swift:549:24: error: use of unresolved identifier 'dispatch_queue_get_specific'
        let specific = dispatch_queue_get_specific(dispatch_get_main_queue(), NSOperationQueue.OperationQueueKey)
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Foundation/NSOperation.swift:549:52: error: use of unresolved identifier 'dispatch_get_main_queue'
        let specific = dispatch_queue_get_specific(dispatch_get_main_queue(), NSOperationQueue.OperationQueueKey)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~
Member

jckarter commented Mar 25, 2016

@parkera Is this failure in Foundation expected on Linux?

Foundation/NSOperation.swift:549:24: error: use of unresolved identifier 'dispatch_queue_get_specific'
        let specific = dispatch_queue_get_specific(dispatch_get_main_queue(), NSOperationQueue.OperationQueueKey)
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Foundation/NSOperation.swift:549:52: error: use of unresolved identifier 'dispatch_get_main_queue'
        let specific = dispatch_queue_get_specific(dispatch_get_main_queue(), NSOperationQueue.OperationQueueKey)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~
@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 25, 2016

Member

@swift-ci Please smoke test

Member

jckarter commented Mar 25, 2016

@swift-ci Please smoke test

@parkera

This comment has been minimized.

Show comment
Hide comment
@parkera

parkera Mar 25, 2016

Member

@phausler This stuff should be disabled on Linux right?

Member

parkera commented Mar 25, 2016

@phausler This stuff should be disabled on Linux right?

@phausler

This comment has been minimized.

Show comment
Hide comment
@phausler

phausler Mar 25, 2016

Member

yea I disabled it via expanding the guards

Member

phausler commented Mar 25, 2016

yea I disabled it via expanding the guards

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 25, 2016

Member

@parkera @phausler Thanks for confirming!

Member

jckarter commented Mar 25, 2016

@parkera @phausler Thanks for confirming!

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 26, 2016

Member

@swift-ci Please test

Member

jckarter commented Mar 26, 2016

@swift-ci Please test

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 28, 2016

Member

@slavapestov Are these IRGen test failures known?

https://ci.swift.org/job/swift-PR-osx/542/#showFailuresLink

Test Result (6 failures / ±0)
Swift.IRGen.class_resilience.swift
Swift.IRGen.enum_resilience.swift
Swift.IRGen.struct_resilience.swift
Swift.IRGen.foreign_types.sil
Swift.IRGen.concrete_inherits_generic_base.swift
Swift.IRGen.nested_types.sil
Member

jckarter commented Mar 28, 2016

@slavapestov Are these IRGen test failures known?

https://ci.swift.org/job/swift-PR-osx/542/#showFailuresLink

Test Result (6 failures / ±0)
Swift.IRGen.class_resilience.swift
Swift.IRGen.enum_resilience.swift
Swift.IRGen.struct_resilience.swift
Swift.IRGen.foreign_types.sil
Swift.IRGen.concrete_inherits_generic_base.swift
Swift.IRGen.nested_types.sil

cwillmor and others added some commits Feb 22, 2016

[test] Convert imported_objc_generics to StdlibUnittest.
...and clean up a few things, and fill out a few more tests.
[IRGen] Functions with the ObjCMethod convention are not polymorphic.
...even if the 'self' type is generic. Additionally, Objective-C generic
types cannot be used as a source of type metadata, because Objective-C
generics are erased at runtime by default. (This may need to change.)

With these two changes, we now pass type metadata explicitly when we need
to, and /don't/ try to pass it to Objective-C methods that would have
needed it if they were Swift methods.
[IRGen] Don't assume Objective-C classes won't be generic.
This showed up as a test failure under swift_test_mode=optimized.
We probably have more of these we should be worrying about.
IRGen: Erase ObjC generics before forming metadata references.
The generic parameters to an ObjC class don't exist at runtime, so when forming a metadata reference, lower them away.
@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Mar 28, 2016

Member

@swift-ci Please test

Member

jckarter commented Mar 28, 2016

@swift-ci Please test

@jckarter jckarter merged commit 4e06de4 into apple:master Mar 28, 2016

2 checks passed

Swift Test Linux Platform Build finished. 8050 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform Build finished. 32364 tests run, 0 skipped, 0 failed.
Details

@jckarter jckarter deleted the jckarter:import-lightweight-objc-generics branch Mar 28, 2016

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 28, 2016

Collaborator

Awesome, please update the changelog to mention this!

Collaborator

lattner commented on 3bcb169 Mar 28, 2016

Awesome, please update the changelog to mention this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment