Add Show Objective-C Selector code action #2399
Conversation
iMostfa
requested review from
ahoppen,
hamishknight and
rintaro
as code owners
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for opening the PR. One thing I’m not entirely convinced about is whether this should be a refactoring actions that modifies the current source file. I would probably have implemented this as a code action that shows a message with the Objective-C selector name to the user via the window/showMessageRequest request if the client supports that request. What do you think about that approach?
iMostfa
force-pushed
the
push-tpunonyzorpx
branch
4 times, most recently
from
44a8a3c to
67d3137
Compare
|
@ahoppen |
iMostfa
force-pushed
the
push-tpunonyzorpx
branch
2 times, most recently
from
711ea33 to
e37ec29
Compare
Screen.Recording.2025-12-23.at.1.10.07.AM.mov@ahoppen I tried to use After giving it more thought, I think it’s more useful to add the Objective-C selector as a comment rather than only showing it in a dialog. The most common use case for this action is likely copying the selector to use it in an Objective-C file. While it’s possible to move the mouse and copy it from the dialog, it’s more convenient to stay on the keyboard and simply navigate to and select the comment. Do you think it makes sense to prefer adding a comment in this case? |
My thoughts are that the Objective-C selector name is a transient bit of information that you are very unlikely to want to keep around in your source file. While I agree that copying may be easier if the selector gets added as a comment, I see two downsides (both not big, but still): We give a small indication that it may be a good pattern to keep the Objective-C selector of these functions around in your source file, which I don’t want to give. Furthermore, you need to remember to remove the comment again after copying the selector name. And doing so may counteract any workflow advantages from putting the selector in the source file. |
|
fair points, so, if we decide to continue with just showMessage request, maybe we can rename the code action to be "Show Objective-c selector" instead of "Copy Objective-c selector" ? |
Absolutely! |
iMostfa
force-pushed
the
push-tpunonyzorpx
branch
4 times, most recently
from
fb9b7a1 to
fab55a4
Compare
iMostfa
changed the title
iMostfa
force-pushed
the
push-tpunonyzorpx
branch
from
fab55a4 to
9ad93db
Compare
This updates the implementation to use `source.request.objc.selector` directly instead of intercepting a refactoring action. Changes: - ShowObjCSelector.swift: Call `source.request.objc.selector` and read the selector from `key.text` in the response - ShowObjCSelectorCommand.swift: Remove actionString field since we're not using refactoring - SwiftLanguageService.swift: Show "Show Objective-C Selector" action for methods/functions/constructors instead of checking for a non-existent refactoring action - sourcekitd_uids.swift: Add `objcSelector` UID for the new request This aligns with swiftlang/swift#86173 which implements the feature as a standalone request rather than a refactoring action.
iMostfa
force-pushed
the
push-tpunonyzorpx
branch
from
9ad93db to
da99589
Compare
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Nice, this looks good. Could you also add a test case for the new refactoring action? Both for the case where it shows and also a case checking that we don’t show it if the function doesn’t have an Objective-C selector.
| guard case .dictionary(let documentDict)? = dictionary[CodingKeys.textDocument.stringValue], | ||
| case .string(let title)? = dictionary[CodingKeys.title.stringValue], | ||
| case .dictionary(let rangeDict)? = dictionary[CodingKeys.positionRange.stringValue] | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| guard let positionRange = Range<Position>(fromLSPDictionary: rangeDict), | ||
| let textDocument = TextDocumentIdentifier(fromLSPDictionary: documentDict) | ||
| else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I think this can be simplified slightly by using init(fromLSPAny:)
| guard case .dictionary(let documentDict)? = dictionary[CodingKeys.textDocument.stringValue], | |
| case .string(let title)? = dictionary[CodingKeys.title.stringValue], | |
| case .dictionary(let rangeDict)? = dictionary[CodingKeys.positionRange.stringValue] | |
| else { | |
| return nil | |
| } | |
| guard let positionRange = Range<Position>(fromLSPDictionary: rangeDict), | |
| let textDocument = TextDocumentIdentifier(fromLSPDictionary: documentDict) | |
| else { | |
| return nil | |
| } | |
| guard let textDocument = TextDocumentIdentifier(fromLSPAny: dictionary[CodingKeys.textDocument.stringValue]), | |
| case .string(let title)? = dictionary[CodingKeys.title.stringValue], | |
| let positionRange = Range<Position>(fromLSPAny: dictionary[CodingKeys.positionRange.stringValue]) | |
| else { | |
| return nil | |
| } |
There was a problem hiding this comment.
Could you add the new files to CMakeLists.txt for the Windows build?
| return methodKinds.contains(kind) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Does this mean that we show the refactoring action on every Swift method, even if it isn’t backed by an Objective-C method and thus the Show Objective-C Selector would just fail? Do we have some indication in the cursor info response already to see whether this method is defined in Objective-C / that we have an Objective-C selector? One ugly idea I have is checking whether the USR starts with s: but there might be nicer implementations as well.
2 participants
Resolves: #2145
along with: swiftlang/swift#86173
example.mov
Add Copy Objective-C Selector code action
Implements a new refactor action that allows users to copy Objective-C
selectors from Swift code. Adds the objcSelector SourceKitD request and
corresponding command handling in the language service.
If the approach is okay, i can go forward with adding/updating tests, lint..etc
Thanks