[IRGen] Call objc_direct methods correctly #33349

ellishg · 2020-08-07T00:43:43Z

When we encounter an Objective-C method with the objc_direct attribute we don't want to use objc_msgSend(). Instead we call the method directly.

include/swift/AST/Decl.h

test/IRGen/Inputs/objc_direct.h

lib/IRGen/IRGenSIL.cpp

ellishg · 2020-08-17T18:56:53Z

I've resolved most of the comments. Can I get a second review? @CodaFi @rjmccall

slavapestov

This looks great!

lib/SILGen/SILGenApply.cpp

test/IRGen/objc_direct.swift

lib/ClangImporter/ImportDecl.cpp

lib/IRGen/IRGenSIL.cpp

lib/IRGen/GenDecl.cpp

test/IRGen/objc_direct.swift

rjmccall · 2020-09-21T19:49:46Z

lib/AST/NameLookup.cpp

+        if (objCProperty->isDirectProperty())
+          continue;
+      }
+    }


It's an interesting question whether suppressing this in lookup is the right thing to do, as opposed to diagnosing the attempt during type-checking. I've asked @DougGregor and @brentdax to weigh in here. Naively I'd think we should diagnose during type-checking.

This is effectively a choice between emitting a generic and slightly incorrect "no method found"-type diagnostic, and emitting a tailored "direct methods cannot be called through AnyObject dispatch"-type diagnostic.

I think it'd be better to emit a tailored diagnostic, but it could be left out if that would be very difficult, or it could be emitted based on a separate lookup performed while diagnosing the error. Please make sure you include a test case where one class has a direct method and another has a normal method by the same name—we'll want to make sure that the direct method doesn't stop you from using the normal one through AnyObject dispatch.

I do agree that this is a very unhelpful error message for developers who might be surprised to see a "no method found"-type message after adding an objc_direct attribute. I'm not very familiar with this part of the codebase, where would we put the lookup to diagnose the error?

And thanks for the test case, I'll add that.

lib/IRGen/IRGenSIL.cpp

lib/SIL/IR/SILDeclRef.cpp

shahmishal · 2020-10-01T06:53:46Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

rjmccall

LGTM

rjmccall · 2020-10-20T05:09:08Z

@swift-ci Please test

swift-ci · 2020-10-20T06:25:16Z

Build failed
Swift Test Linux Platform
Git Sha - cfc6ed49098b42529e880b2cd926dc6bcfa79941

swift-ci · 2020-10-20T06:58:16Z

Build failed
Swift Test OS X Platform
Git Sha - cfc6ed49098b42529e880b2cd926dc6bcfa79941

ellishg · 2020-10-21T01:52:58Z

Hopefully I have fixed the ci tests

ellishg · 2020-10-21T01:53:06Z

@swift-ci Please test

rjmccall · 2020-10-22T17:03:36Z

@swift-ci Please test

swift-ci · 2020-10-22T18:22:41Z

Build failed
Swift Test Linux Platform
Git Sha - 31d12dc53ebb74b00c454f711cabeeaaf7306295

swift-ci · 2020-10-22T18:47:40Z

Build failed
Swift Test OS X Platform
Git Sha - 31d12dc53ebb74b00c454f711cabeeaaf7306295

rjmccall · 2020-10-23T00:18:14Z

swiftlang/llvm-project#2028

@swift-ci Please test

swift-ci · 2020-10-23T01:18:30Z

Build failed
Swift Test Linux Platform
Git Sha - 31d12dc53ebb74b00c454f711cabeeaaf7306295

ellishg · 2020-10-23T01:33:02Z

Sorry, I think I introduced some errors when I fixed the merge conflict. I’ll take a look tomorrow.

swift-ci · 2020-10-23T02:05:31Z

Build failed
Swift Test OS X Platform
Git Sha - 31d12dc53ebb74b00c454f711cabeeaaf7306295

ellishg · 2020-10-23T17:00:54Z

test/Index/Inputs/cross_language.m

@@ -6,7 +6,7 @@ @interface MyCls1(ext_in_objc)
 // CHECK: [[@LINE-1]]:12 | class/Swift | MyCls1 | [[MyCls1_USR]] |
 // CHECK: [[@LINE-2]]:19 | extension/ObjC | ext_in_objc | c:@M@cross_language@objc(cy)MyCls1@ext_in_objc |
 -(void)someMethFromObjC;
-// CHECK: [[@LINE-1]]:8 | instance-method/ObjC | someMethFromObjC | [[someMethFromObjC_USR:.*]] | -[ext_in_objc someMethFromObjC]
+// CHECK: [[@LINE-1]]:8 | instance-method/ObjC | someMethFromObjC | [[someMethFromObjC_USR:.*]] | -[MyCls1(ext_in_objc) someMethFromObjC]


@rjmccall I want to draw your attention to this test I fixed that breaks when using swiftlang/llvm-project#2028.

ellishg · 2020-10-23T17:01:28Z

I believe I have now fixed all the test failures.

rjmccall · 2020-10-23T18:24:12Z

@swift-ci Please test

swift-ci · 2020-10-23T18:34:39Z

Build failed
Swift Test OS X Platform
Git Sha - 3aa081c

swift-ci · 2020-10-23T19:15:24Z

Build failed
Swift Test Linux Platform
Git Sha - 3aa081c

ellishg · 2020-10-23T22:34:04Z

@rjmccall Oh yeah. You'll need to test with swiftlang/llvm-project#2028.

rjmccall · 2020-10-24T06:09:48Z

swiftlang/llvm-project#2028

@swift-ci Please test

rjmccall · 2020-10-24T06:10:07Z

@rjmccall Oh yeah. You'll need to test with apple/llvm-project#2028.

Argh, yes, I did it right earlier.

swift-ci · 2020-10-24T06:16:19Z

Build failed
Swift Test Linux Platform
Git Sha - 3aa081c

swift-ci · 2020-10-24T06:18:51Z

Build failed
Swift Test OS X Platform
Git Sha - 3aa081c

ellishg · 2020-10-26T19:20:30Z

@rjmccall The CI is failing to checkout llvm-project

fatal: Couldn't find remote ref pull/2028/merge

Shall we try again?

rjmccall · 2020-10-27T20:49:42Z

@rjmccall The CI is failing to checkout llvm-project

fatal: Couldn't find remote ref pull/2028/merge

Shall we try again?

I'm just waiting until it merges into main; it should already be there, but the automerger is held up.

rjmccall · 2020-10-29T21:28:30Z

@swift-ci Please test

rjmccall · 2020-10-30T07:03:50Z

Alright, let's do it. Thanks for seeing this through!

theblixguy requested a review from rjmccall August 7, 2020 00:52

CodaFi reviewed Aug 7, 2020

View reviewed changes

include/swift/AST/Decl.h Outdated Show resolved Hide resolved

CodaFi reviewed Aug 7, 2020

View reviewed changes

test/IRGen/Inputs/objc_direct.h Outdated Show resolved Hide resolved

CodaFi reviewed Aug 7, 2020

View reviewed changes

lib/IRGen/IRGenSIL.cpp Outdated Show resolved Hide resolved

ellishg force-pushed the master branch from 42a770c to 72ac58f Compare August 7, 2020 18:39

slavapestov reviewed Aug 25, 2020

View reviewed changes

lib/SILGen/SILGenApply.cpp Show resolved Hide resolved

test/IRGen/objc_direct.swift Outdated Show resolved Hide resolved

test/IRGen/objc_direct.swift Outdated Show resolved Hide resolved

rjmccall reviewed Aug 25, 2020

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

lib/IRGen/IRGenSIL.cpp Outdated Show resolved Hide resolved

lib/IRGen/GenDecl.cpp Show resolved Hide resolved

test/IRGen/objc_direct.swift Outdated Show resolved Hide resolved

ellishg force-pushed the master branch 3 times, most recently from a601f89 to 02d32ae Compare September 3, 2020 16:14

ellishg force-pushed the master branch 2 times, most recently from 42720e1 to b4727f9 Compare September 10, 2020 18:39

rjmccall reviewed Sep 21, 2020

View reviewed changes

ellishg force-pushed the master branch from b4727f9 to 024ded5 Compare September 24, 2020 22:53

ellishg changed the base branch from master to main October 1, 2020 13:57

ellishg force-pushed the master branch 2 times, most recently from bef9734 to cfc6ed4 Compare October 19, 2020 15:50

rjmccall approved these changes Oct 20, 2020

View reviewed changes

ellishg force-pushed the master branch from cfc6ed4 to 8a035d3 Compare October 21, 2020 01:52

ellishg force-pushed the master branch from 8a035d3 to 31d12dc Compare October 21, 2020 02:47

This was referenced Oct 22, 2020

Cherrypick ObjC method mangling commits for Swift swiftlang/llvm-project#2026

Closed

Cherrypick ObjC method mangling commits for Swift swiftlang/llvm-project#2028

Merged

[IRGen] Call objc_direct methods correctly

3aa081c

ellishg force-pushed the master branch from 31d12dc to 3aa081c Compare October 23, 2020 16:56

ellishg commented Oct 23, 2020

View reviewed changes

rjmccall merged commit e35f077 into swiftlang:main Oct 30, 2020

[IRGen] Call objc_direct methods correctly #33349

[IRGen] Call objc_direct methods correctly #33349

Uh oh!

Conversation

ellishg commented Aug 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellishg commented Aug 17, 2020

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjmccall Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

beccadax Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

ellishg Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shahmishal commented Oct 1, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall commented Oct 20, 2020

Uh oh!

swift-ci commented Oct 20, 2020

Uh oh!

swift-ci commented Oct 20, 2020

Uh oh!

ellishg commented Oct 21, 2020

Uh oh!

ellishg commented Oct 21, 2020

Uh oh!

rjmccall commented Oct 22, 2020

Uh oh!

swift-ci commented Oct 22, 2020

Uh oh!

swift-ci commented Oct 22, 2020

Uh oh!

rjmccall commented Oct 23, 2020

Uh oh!

swift-ci commented Oct 23, 2020

Uh oh!

ellishg commented Oct 23, 2020

Uh oh!

swift-ci commented Oct 23, 2020

Uh oh!

ellishg Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

ellishg commented Oct 23, 2020

Uh oh!

rjmccall commented Oct 23, 2020

Uh oh!

swift-ci commented Oct 23, 2020

Uh oh!

swift-ci commented Oct 23, 2020

Uh oh!

ellishg commented Oct 23, 2020

Uh oh!

rjmccall commented Oct 24, 2020

Uh oh!

rjmccall commented Oct 24, 2020