LS integration for diagnostics & resource-reference #1150

nighca · 2024-12-17T09:55:59Z

qiniu-prow · 2024-12-17T09:56:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copilot reviewed 13 out of 28 changed files in this pull request and generated no comments.

Files not reviewed (15)

spx-gui/src/components/editor/code-editor/ui/diagnostics/DiagnosticsUI.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/resource/ResourceItem.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/resource-reference/selector/ResourceSelector.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/resource-reference/ResourceReferenceUI.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/markdown/ResourcePreview.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/CodeEditorUI.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/markdown/MarkdownView.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/CodeEditor.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/markdown/DiagnosticItem.vue: Evaluated as low risk
spx-gui/src/components/editor/code-editor/lsp/index.ts: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/common.ts: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/completion/index.ts: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/index.ts: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/hover/index.ts: Evaluated as low risk
spx-gui/src/components/editor/code-editor/ui/resource-reference/index.ts: Evaluated as low risk

Comments suppressed due to low confidence (4)

spx-gui/src/components/editor/code-editor/common.ts:451

[nitpick] The function name positionEq should be renamed to arePositionsEqual for clarity.

export function positionEq(a: Position | null, b: Position | null) {

spx-gui/src/components/editor/code-editor/ui/resource-reference/selector/index.ts:198

[nitpick] The variable name 'parsed' is used multiple times within the same function. Consider using more descriptive names for each instance.

const parsed = parseResourceURI(rr.resource.uri)

spx-gui/src/components/editor/code-editor/ui/resource-reference/selector/index.ts:213

[nitpick] The error message 'Unexpected sub-resource type: ${parsed[1].type}' could be more descriptive to provide more context about why the sub-resource type is unexpected.

throw new Error(`Unexpected sub-resource type: ${parsed[1].type}`)

spx-gui/src/components/editor/code-editor/ui/resource-reference/selector/index.ts:221

[nitpick] The error message 'Unexpected resource type: ${parsed[0].type}' could be more descriptive to provide more context about why the resource type is unexpected.

throw new Error(`Unexpected resource type: ${parsed[0].type}`)

nighca · 2024-12-18T02:34:20Z

tools/spxls/internal/server/compile.go

@@ -90,7 +90,10 @@ func (r *compileResult) innermostScopeAt(pos goptoken.Pos) *types.Scope {
 	// The `MainEntry` scope is the last child of the main package and covers all positions in the workspace.
 	// We need to skip `MainEntry` when searching for the innermost scope, as the file scope will be more precise.
 	// Especially for positions in trailing empty lines of a file, the file scope does not contain them in its position range.
-	mainEntryScope := mainPkgScope.Child(mainPkgScope.Len() - 1)
+	var mainEntryScope *types.Scope
+	if mainPkgScope.Len() > 0 {


这里是遇到过 mainPkgScope 一个 child 也没有的情况，所以处理下

现在感觉直接认最后一个 child 就是 MainEntry 也未必准确，不过暂时没有想到更好的识别方式，就先没动

nighca · 2024-12-18T02:35:51Z

tools/spxls/internal/server/diagnostic.go

@@ -10,7 +10,7 @@ func (s *Server) textDocumentDiagnostic(params *DocumentDiagnosticParams) (*Docu
 	return &DocumentDiagnosticReport{Value: &RelatedFullDocumentDiagnosticReport{
 		FullDocumentDiagnosticReport: FullDocumentDiagnosticReport{
 			Kind:  string(DiagnosticFull),
-			Items: result.diagnostics[params.TextDocument.URI],
+			Items: append([]Diagnostic{}, result.diagnostics[params.TextDocument.URI]...),


确保 items 为空时 JSON 序列化后是 [] 而不是 null，跟 LSP 这里的要求保持一致

如果这是 LSP 要求的，那感觉在

builder/tools/spxls/internal/server/compile.go

Line 321 in 57de383

result.diagnostics[documentURI] = nil

这里改成初始化空数组可能更合适些

是的，LSP 中 items 的类型是 Diagnostic[]；我来调整下

done https://github.com/goplus/builder/compare/57de383cb871474b358ec98186ee16259dda5012..6718f654348389aba32db2a816e3f1485490cdd9

nighca · 2024-12-18T02:36:43Z

spx-gui/src/components/editor/code-editor/ui/resource-reference/index.ts

  return [
-    ResourceReferenceKind.AutoBindingReference,
+    // ResourceReferenceKind.AutoBindingReference, // consider cases like `Foo.say "xxx"`, modifying `Foo` is complicated


情况比我预计的复杂，暂时先不支持 AutoBindingReference 的 modify..

nighca · 2024-12-18T02:38:01Z

spx-gui/src/components/editor/code-editor/lsp/index.ts

+  async textDocumentDiagnostic(params: lsp.DocumentDiagnosticParams): Promise<lsp.DocumentDiagnosticReport> {
+    const spxlc = await this.prepareRquest()
+    const report = await spxlc.request<any>(lsp.DocumentDiagnosticRequest.method, params)
+    return report.value // TODO: this should be fixed in `spxls`, see type `DocumentDiagnosticReport` there for more details


这里需要瞅一眼，我没想好 ls 那里怎么改合适，所以先没改那里

指的是返回的结果不应该是：

{ "value": { "kind": "full", "items": [] } }

而应该是：

{ "kind": "full", "items": [] }

是这个意思吗？

对的，不应该有 value 这一层

嗯我看到了，我从 gopls 那里拷过来的时候漏了点东西，应该把 https://github.com/golang/tools/blob/74dea82592ad27cc0a4954831ee4bca305ed5ebd/gopls/internal/protocol/tsjson.go 这个文件的内容也拷进 protocol.go 才对，这里面有对 Value 做处理：

https://github.com/golang/tools/blob/74dea82592ad27cc0a4954831ee4bca305ed5ebd/gopls/internal/protocol/tsjson.go#L386-L396

hhh 要不这个你来处理吧？你改完之后帮我把这边改成

return spxlc.request<lsp.DocumentDiagnosticReport>(lsp.DocumentDiagnosticRequest.method, params)

就好

嗯可以的，那先合吧

nighca · 2024-12-18T02:40:04Z

spx-gui/src/components/editor/code-editor/CodeEditor.vue

+  }
+  private adaptDiagnosticRange({ start, end }: Range, textDocument: ITextDocument) {
+    // make sure the range is not empty, so that the diagnostic info can be displayed as inline decorations
+    // TODO: it's a workaround, should be fixed in the server side


对于空 range 的 diagnostic，先处理成向后或向前找到第一个非换行字符，使用这个字符作为 range，这样确保至少有一个字符会被高亮（波浪线 & hover）

qiniu-prow · 2024-12-18T02:41:34Z

@nighca: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
goplus-builder-pr-review-environment-test	`57de383`	link	true	`/test goplus-builder-pr-review-environment-test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vercel bot deployed to Preview December 17, 2024 10:01 View deployment

nighca requested a review from Copilot December 18, 2024 00:38

Copilot AI reviewed Dec 18, 2024

View reviewed changes

nighca force-pushed the issue-1149 branch from 6fa1f0b to 3671e6b Compare December 18, 2024 01:00

vercel bot deployed to Preview December 18, 2024 01:05 View deployment

nighca force-pushed the issue-1149 branch from 3671e6b to 57de383 Compare December 18, 2024 01:09

vercel bot deployed to Preview December 18, 2024 01:15 View deployment

nighca commented Dec 18, 2024

View reviewed changes

nighca marked this pull request as ready for review December 18, 2024 02:40

LS integration for diagnostics & resource-reference

6718f65

nighca force-pushed the issue-1149 branch from 57de383 to 6718f65 Compare December 19, 2024 01:39

aofei approved these changes Dec 19, 2024

View reviewed changes

aofei merged commit fb662b5 into goplus:dev Dec 19, 2024
5 of 7 checks passed

vercel bot deployed to Preview December 19, 2024 01:45 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LS integration for diagnostics & resource-reference #1150

LS integration for diagnostics & resource-reference #1150

nighca commented Dec 17, 2024

qiniu-prow bot commented Dec 17, 2024

nighca Dec 18, 2024

nighca Dec 18, 2024 •

edited

Loading

aofei Dec 19, 2024

nighca Dec 19, 2024

nighca Dec 19, 2024

nighca Dec 18, 2024

nighca Dec 18, 2024

aofei Dec 19, 2024

nighca Dec 19, 2024

aofei Dec 19, 2024

nighca Dec 19, 2024

aofei Dec 19, 2024

nighca Dec 18, 2024

qiniu-prow bot commented Dec 18, 2024

LS integration for diagnostics & resource-reference #1150

LS integration for diagnostics & resource-reference #1150

Conversation

nighca commented Dec 17, 2024

qiniu-prow bot commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca Dec 18, 2024 • 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

qiniu-prow bot commented Dec 18, 2024

nighca Dec 18, 2024 •

edited

Loading