Report codeActions for fixIts produced by swift syntax #941

joehsieh · 2023-10-27T15:45:13Z

~~still WIP, i'm trying to figure out how to write tests for this change.~~

Implemented an initializer of CodeAction with the FixIt from swift-syntax and added a test for it.

ahoppen · 2023-10-27T18:14:50Z

Having an idea of how to test this is definitely not trivial, so here’s a snippet. You should then be able to write assertions to test diagnostics.

// Create a workspace with compile commands that don't contain the file we are getting diagnostics for.
// That way we have a workspace that has a build system (and thus doesn't promote settings from the fallback build
// system to be non-fallback settings) but doesn't have build settings for the file being tested. Thus, we generate
// diagnostics from the built-in SwiftParser.
let ws = try await MultiFileTestWorkspace(files: [
  "test.swift": "let var = 2",
  "compile_commands.json": "[]"
])

let (uri, _) = try ws.openDocument("test.swift")

let diagnostics = try await ws.testClient.send(DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(uri)))

ahoppen · 2023-10-27T18:26:33Z

And thanks a lot for working on this @joehsieh!

joehsieh · 2023-10-30T15:00:00Z

Package.swift

@@ -337,7 +337,8 @@ if useLocalDependencies {
    .package(url: "https://github.com/apple/swift-package-manager.git", branch: relatedDependenciesBranch),
    .package(url: "https://github.com/apple/swift-tools-support-core.git", branch: relatedDependenciesBranch),
    .package(url: "https://github.com/apple/swift-argument-parser.git", from: "1.2.2"),
-    .package(url: "https://github.com/apple/swift-syntax.git", branch: relatedDependenciesBranch),
+    // .package(url: "https://github.com/apple/swift-syntax.git", branch: relatedDependenciesBranch),
+    .package(url: "https://github.com/joehsieh/swift-syntax", branch: "add-fix-it-extesion"),


note: this change will be reverted once this PR is merged.

ahoppen · 2023-10-30T22:50:02Z

Sources/SourceKitLSP/Swift/Diagnostic.swift

+    )
+  }
+
+  private static func generateTitle(with edits: [TextEdit], snapshot: DocumentSnapshot) -> String? {


Nitpick, I would name this without generate. Since this is side-effect free, I don’t think the function name needs a result and generate basically just says that the function is returning something, which is obvious from the result type.

Suggested change

private static func generateTitle(with edits: [TextEdit], snapshot: DocumentSnapshot) -> String? {

private static func title(for edits: [TextEdit], in snapshot: DocumentSnapshot) -> String? {

you're right. let me update it.

ahoppen · 2023-10-30T22:52:18Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+        edit: WorkspaceEdit(
+          changes: [
+            uri : [
+              TextEdit(range: Position(line: 0, utf16index: 9)..<Position(line: 0, utf16index: 15), newText: "Multiident "),


Could you use location markers here instead of hardcoded line-column offsets. We aren’t very good about it in the codebase yet as I only introduced the concept of location markers in the last couple of weeks but it’s generally proffered because of the same reasons that I mentioned in your swift-syntax PR.

Here’s an example of how to use the location markers.

https://github.com/apple/sourcekit-lsp/blob/9f7ff0891a115f1ae056348744abffd0452806e5/Tests/SourceKitLSPTests/LocalClangTests.swift#L246-L264

got it, it makes sense to use location markers instead. i'll update the code.

ahoppen · 2023-10-30T22:52:33Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+
+    let expectedCodeActions = [
+      CodeAction(
+        title: "Replace \'Multi \' with \'Multiident \'...", 


You don’t need to escape the ' in Swift string literals.

ahoppen · 2023-10-30T22:53:08Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+    guard let codeActions = diagnostic.codeActions else {
+      XCTFail("Expected non-empty codeActions")
+      return
+    }


You could just use XCTUnwrap

Suggested change

guard let codeActions = diagnostic.codeActions else {

XCTFail("Expected non-empty codeActions")

return

}

let codeActions = try XCTUnwrap(diagnostic.codeAction)

ahoppen · 2023-10-31T16:48:51Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+
+  func testCodeActionForFixItsProducedBySwiftSyntax() async throws {
+    let ws = try await MultiFileTestWorkspace(files: [
+      "test.swift": "protocol 0️⃣Multi 1️⃣ident 2️⃣{}",


Nitpick similar to the PR in swift-syntax: We usually start counting the location markers at 0. Would be nice to be consistent here.

ahoppen · 2023-10-31T16:50:19Z

Sources/SourceKitLSP/Swift/Diagnostic.swift

+    guard let generatedTitle = Self.title(for: textEdits, in: snapshot) else {
+      return nil
+    }


Fix-Its from swift-syntax always have a message (fixIt.message.message I think) and I think we should be using that instead of synthesizing a title here. I think the extraction of title(for:in:) makes sense regardless though.

i tried to use fixIt.message.message instead of title(for:in:), but it failed the test, so we might not be able to do it.

Expected: "Replace 'Multi ' with 'Multiident '..." "Replace 'Multi ' with 'MultiIdent '..." Actual: "join the identifiers together", "join the identifiers together with camel-case"

Can’t you just update the test case to expect join the identifiers together?

Also, it would be good to uppercase the first character of the diagnostic, similar to what I did here: #784

you're right, i put the code protocol Multi ident {} in a swift playground.
the error message was

Join the identifiers together Join the identifiers together with camel-case

let me correct it.

ahoppen

Looks good to me. We just need to wait to get swiftlang/swift-syntax#2314 in and revert the change to the package manifest.

When you do that, could you squash your commits? It makes for a nicer Git history.

joehsieh · 2023-11-02T23:21:05Z

Got it, i'll do it.
btw, for sourcekit-lsp, do we have a similar way to format the code like what we did for swift-syntax?

in the meantime, i'm trying to refine below command and put it in a git hook

find . -type f -name "*.swift" -exec swift-format format -i {} \;

ahoppen · 2023-11-06T16:47:36Z

I was thinking about integrating the swift-format command plugin into sourcekit-lsp. That way you should be able to run swift-format e.g. from Xcode. But I haven’t had time to look into it yet. If you want, you could try that.

joehsieh · 2023-11-07T14:27:46Z

thanks for the reminder. i found a vscode plugin for swift-format.
https://marketplace.visualstudio.com/items?itemName=vknabel.vscode-apple-swift-format&ssr=false#overview
even though i need to click a button to format the code by myself...

for the plugin of Xcode, it looks like there is a good one. https://github.com/nicklockwood/SwiftFormat#what-is-this
but I haven't tried it.

Finetune the code Finetune the code Finetune the code Update the code per comments Use location marker instead of hard coded indexes Update the test to align the code convention Rename a variable Correct unit tests Format the code Update Package.swift

ahoppen

Let’s get this in 🎉

ahoppen · 2023-11-08T15:25:50Z

@swift-ci Please test

ahoppen · 2023-11-08T15:26:02Z

@swift-ci Please test Windows

ahoppen · 2023-11-08T19:49:34Z

Windows failure is unrelated to your PR.

shahmishal · 2023-11-08T21:11:51Z

@swift-ci Please test Windows

ahoppen · 2023-11-09T17:42:00Z

@swift-ci Please test Windows

joehsieh force-pushed the report-codeActions-for-fix-its-produced-by-swift-syntax branch 2 times, most recently from 15158e3 to 8a4bc54 Compare October 30, 2023 14:56

joehsieh commented Oct 30, 2023

View reviewed changes

joehsieh marked this pull request as ready for review October 30, 2023 15:00

joehsieh requested review from benlangmuir and ahoppen as code owners October 30, 2023 15:00

ahoppen reviewed Oct 30, 2023

View reviewed changes

joehsieh requested a review from ahoppen October 31, 2023 13:05

ahoppen reviewed Oct 31, 2023

View reviewed changes

joehsieh force-pushed the report-codeActions-for-fix-its-produced-by-swift-syntax branch from 5b5e295 to 33b4950 Compare November 2, 2023 01:10

ahoppen reviewed Nov 2, 2023

View reviewed changes

joehsieh force-pushed the report-codeActions-for-fix-its-produced-by-swift-syntax branch from 13feeb5 to a095810 Compare November 8, 2023 09:12

joehsieh requested a review from ahoppen November 8, 2023 09:14

ahoppen approved these changes Nov 8, 2023

View reviewed changes

ahoppen enabled auto-merge November 8, 2023 15:25

ahoppen merged commit 113219a into swiftlang:main Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report codeActions for fixIts produced by swift syntax #941

Report codeActions for fixIts produced by swift syntax #941

joehsieh commented Oct 27, 2023 •

edited

Loading

ahoppen commented Oct 27, 2023

ahoppen commented Oct 27, 2023

joehsieh Oct 30, 2023 •

edited

Loading

ahoppen Oct 30, 2023

joehsieh Oct 31, 2023

ahoppen Oct 30, 2023

joehsieh Oct 31, 2023

ahoppen Oct 30, 2023

ahoppen Oct 30, 2023

ahoppen Oct 31, 2023

ahoppen Oct 31, 2023

joehsieh Nov 1, 2023

ahoppen Nov 1, 2023

joehsieh Nov 2, 2023

ahoppen left a comment

joehsieh commented Nov 2, 2023 •

edited

Loading

ahoppen commented Nov 6, 2023

joehsieh commented Nov 7, 2023

ahoppen left a comment

ahoppen commented Nov 8, 2023

ahoppen commented Nov 8, 2023

ahoppen commented Nov 8, 2023

shahmishal commented Nov 8, 2023

ahoppen commented Nov 9, 2023

	private static func generateTitle(with edits: [TextEdit], snapshot: DocumentSnapshot) -> String? {
	private static func title(for edits: [TextEdit], in snapshot: DocumentSnapshot) -> String? {

Report codeActions for fixIts produced by swift syntax #941

Report codeActions for fixIts produced by swift syntax #941

Conversation

joehsieh commented Oct 27, 2023 • edited Loading

ahoppen commented Oct 27, 2023

ahoppen commented Oct 27, 2023

joehsieh Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

joehsieh commented Nov 2, 2023 • edited Loading

ahoppen commented Nov 6, 2023

joehsieh commented Nov 7, 2023

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented Nov 8, 2023

ahoppen commented Nov 8, 2023

ahoppen commented Nov 8, 2023

shahmishal commented Nov 8, 2023

ahoppen commented Nov 9, 2023

joehsieh commented Oct 27, 2023 •

edited

Loading

joehsieh Oct 30, 2023 •

edited

Loading

joehsieh commented Nov 2, 2023 •

edited

Loading