-
Notifications
You must be signed in to change notification settings - Fork 689
[move-analyzer] Support document symbol PSL #218
Changes from 58 commits
79d4d64
166bd9c
1c16bcc
4f940bc
ce9ec8a
9cd389b
c9f13e7
6efd3bf
254cd9c
c74bfff
5809291
a8f6997
3bb4356
b008c1a
e74bceb
000b9a3
f53f2cc
35476e9
09ef64d
6f87de5
ea8761b
43fea39
179708e
a479ee7
bfbe898
c863d2f
945ead0
71e3900
b682cd0
3558e5d
b817b02
081f93e
0be56e1
72c3cb6
07c9b97
b006e8f
1a19071
be3090a
33c45fd
5ffa26e
a3ab0bd
b33ffce
023b6da
96cbf9e
021c713
9ce1851
b6897a4
09d482f
a05eeac
ecebbc6
d623b21
00ee707
998e630
edd291d
73d4c73
44ac909
a1458e8
2242869
9016aa3
1cc7245
b8ea9ac
21697b0
b149aee
1e62ec3
8d4e3d6
f56bb60
2ad1510
66d7d5a
23c4da1
78f15cb
0660448
33caa51
b564f72
c4f76b9
4332a4f
761a77b
df1a545
bc250ea
4753dd8
7c55136
1f2dce5
2b3f95f
808e8c7
b40fe1e
8e534ed
feffb26
317618b
4ec7364
4695277
49246f6
ceefd18
0781331
078e528
bfcac5e
253e656
95c0545
6d1b88f
2af17df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,8 @@ jobs: | |
- prepare | ||
steps: | ||
- uses: actions/[email protected] | ||
- name: install rust toolchain | ||
uses: actions-rs/toolchain@v1 | ||
- name: Use Node.js 14 | ||
uses: actions/[email protected] | ||
with: | ||
|
@@ -257,7 +259,7 @@ jobs: | |
gem install awesome_bot | ||
# Don't look in git or target dirs. Don't check png, bib, tex, js, or shell files | ||
# We allow links to be redirects, allow duplicates, and we also allow Too Many Requests (429) errors | ||
find . -not \( -path "./.git*" -prune \) -not \( -path "./target" -prune \) -type f -not -name "*.png" -not -name "*.sh" -not -name "*.bib" -not -name "*.tex" -not -name "*.js" | while read arg; do awesome_bot --allow-redirect --allow-dupe --allow 429 --skip-save-results $arg; done | ||
find . -not \( -path "./.git*" -prune \) -not \( -path "./target" -prune \) -type f -not -name "*.png" -not -name "*.sh" -not -name "*.bib" -not -name "*.tex" -not -name "*.js" -not -name "package-lock.json" | while read arg; do awesome_bot --allow-redirect --allow-dupe --allow 429 --skip-save-results $arg; done | ||
|
||
build-move-cli-docker-image: | ||
name: Build Docker image for the Move CLI | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,7 @@ | |
"max-len": [ | ||
"warn", | ||
{ | ||
"code": 100, | ||
"code": 120, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this changes and other eslint changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a conflict between lint and fmt checks before, making it impossible to pass the check when setting 100, and the others were similar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I tested your new PR and stack overflow problem solved |
||
"ignoreUrls": true | ||
} | ||
], | ||
|
@@ -206,10 +206,15 @@ | |
], | ||
"@typescript-eslint/strict-boolean-expressions": "warn", | ||
"@typescript-eslint/switch-exhaustiveness-check": "warn", | ||
"@typescript-eslint/type-annotation-spacing": "warn", | ||
"@typescript-eslint/unified-signatures": "warn", | ||
|
||
// The following are eslint-plugin-tsdoc rules: | ||
"tsdoc/syntax": "warn" | ||
"tsdoc/syntax": "warn", | ||
|
||
"@typescript-eslint/no-unsafe-return": "off", | ||
"@typescript-eslint/type-annotation-spacing": "off", | ||
"@typescript-eslint/no-explicit-any": "off", | ||
"@typescript-eslint/no-unsafe-assignment": "off", | ||
"@typescript-eslint/restrict-template-expressions": "off" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,5 @@ out | |
.vscode-test | ||
# VS Code extension package (VSIX) archive | ||
*.vsix | ||
# Yarn cache directory | ||
yarn.lock |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './lsp_command'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stella3d, I was. hoping that you could take a look at the TS code in this PR, mostly at how idiomatic it is (I am not at TS expert I am afraid...). |
||
import * as lc from 'vscode-languageclient'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type declarations of import { DocumentSymbolParams, SymbolInformation, DocumentSymbol } from vscode-languageclient
...
export async function textDocumentDocumentSymbol(context: Readonly<Context>, params: DocumentSymbolParams) |
||
import type { Context } from '../context'; | ||
|
||
/** | ||
* An LSP command textDocument/documentSymbol | ||
*/ | ||
export async function textDocumentDocumentSymbol(context: Readonly<Context>, params: lc.DocumentSymbolParams) | ||
: Promise<Array<lc.SymbolInformation> | Array<lc.DocumentSymbol> | null> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const client = context.getClient(); | ||
if (client === undefined) { | ||
return Promise.reject(new Error('No language client connected.')); | ||
} | ||
|
||
// Send the request to the language client. | ||
return client.sendRequest(lc.DocumentSymbolRequest.type, params); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,15 @@ import { sync as commandExistsSync } from 'command-exists'; | |
|
||
/** Information passed along to each VS Code command defined by this extension. */ | ||
export class Context { | ||
private _client: lc.LanguageClient | undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless required by the style guide in this repo, it's more idiomatic to rely on the |
||
|
||
private constructor( | ||
private readonly extensionContext: Readonly<vscode.ExtensionContext>, | ||
readonly configuration: Readonly<Configuration>, | ||
) { } | ||
client: lc.LanguageClient | undefined = undefined, | ||
) { | ||
this._client = client; | ||
} | ||
|
||
static create( | ||
extensionContext: Readonly<vscode.ExtensionContext>, | ||
|
@@ -39,12 +44,14 @@ export class Context { | |
*/ | ||
registerCommand( | ||
name: Readonly<string>, | ||
command: (context: Readonly<Context>) => Promise<void>, | ||
command: (context: Readonly<Context>, ...args: Array<any>) => any, | ||
): void { | ||
const disposable = vscode.commands.registerCommand(`move-analyzer.${name}`, async () => { | ||
const com = await command(this); | ||
return com; | ||
const disposable = vscode.commands.registerCommand(`move-analyzer.${name}`, async (...args: Array<any>) | ||
: Promise<any> => { | ||
const ret = await command(this, ...args); | ||
return ret; | ||
}); | ||
|
||
this.extensionContext.subscriptions.push(disposable); | ||
} | ||
|
||
|
@@ -60,7 +67,7 @@ export class Context { | |
* To read more about the messages sent and responses received by this client, such as | ||
* "initialize," read [the Language Server Protocol specification](https://microsoft.github.io/language-server-protocol/specifications/specification-current/#initialize). | ||
**/ | ||
startClient(): void { | ||
async startClient(): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this changes? In particular, why does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or because ci-test needs to wait for the symbol table parsing to be completed synchronously, which requires that the activation of the plugin is also synchronous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, if "the activation of the plugin is also synchronous" then why do we need More generally, I am not a TS expert, and that's why I asked one of colleagues to look at the TS part of this PR, but still - a few more comments in code explaining what the additions to the TS code do would help (also with future maintenance). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any additional comment on this? I still do not understand why we need asynchrony. You yourself write that the symbol table parsing happens synchronously so why we need async code here? |
||
const executable: lc.Executable = { | ||
command: this.configuration.serverPath, | ||
}; | ||
|
@@ -92,5 +99,16 @@ export class Context { | |
log.info('Starting client...'); | ||
const disposable = client.start(); | ||
this.extensionContext.subscriptions.push(disposable); | ||
this._client = client; | ||
await this._client.onReady(); | ||
} | ||
|
||
/** | ||
* Returns the client that this extension interacts with. | ||
* | ||
* @returns lc.LanguageClient | ||
*/ | ||
getClient(): lc.LanguageClient | undefined { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless replacing the client after constructing is required, this could be replaced with a readonly property: |
||
return this._client; | ||
} | ||
} | ||
} // Context |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,14 @@ import * as path from 'path'; | |
/* eslint-disable */ | ||
// deno-lint-ignore require-await | ||
export async function run(): Promise<void> { | ||
// dev mode | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My confusion about the changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will merge this new PR |
||
const mode = process.env['mode'] || 'test'; | ||
if (mode === 'dev') { | ||
return new Promise((resolve) => { | ||
setTimeout(resolve, 1000 * 60 * 15); // 15分钟休息一下 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change this comment to English. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
}); | ||
} | ||
|
||
/* eslint-disable */ | ||
const suite = new Mocha({ | ||
ui: 'tdd', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
[package] | ||
name = "Symbols" | ||
version = "0.0.1" | ||
|
||
[dependencies] | ||
MoveStdlib = { local = "../../../../../../move-stdlib/", addr_subst = { "std" = "0x1" } } | ||
|
||
[addresses] | ||
Symbols = "0xCAFE" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"folders": [ | ||
{ | ||
"path": "." | ||
} | ||
], | ||
"settings": { | ||
"move-analyzer.server.path": "../../../../target/debug/move-analyzer" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
module Symbols::M1 { | ||
|
||
const SOME_CONST: u64 = 42; | ||
|
||
struct SomeOtherStruct has drop { | ||
some_field: u64, | ||
} | ||
|
||
public fun some_other_struct(v: u64): SomeOtherStruct { | ||
SomeOtherStruct { some_field: v } | ||
} | ||
|
||
#[test] | ||
#[expected_failure] | ||
fun this_is_a_test() { | ||
1/0; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
import * as assert from 'assert'; | ||
import * as Mocha from 'mocha'; | ||
import * as path from 'path'; | ||
import * as vscode from 'vscode'; | ||
import * as lc from 'vscode-languageclient'; | ||
|
||
Mocha.suite('LSP', () => { | ||
Mocha.test('textDocument/documentSymbol', async () => { | ||
const ext = vscode.extensions.getExtension('move.move-analyzer'); | ||
assert.ok(ext); | ||
|
||
await ext.activate(); | ||
|
||
// 1. get workdir | ||
const workDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? ''; | ||
|
||
// 2. open doc | ||
const docs = await vscode.workspace.openTextDocument(path.join(workDir, 'sources/M1.move')); | ||
await vscode.window.showTextDocument(docs); | ||
|
||
// 3. execute command | ||
const params: lc.DocumentSymbolParams = { | ||
textDocument: { | ||
uri: docs.uri.toString(), | ||
}, | ||
}; | ||
|
||
const syms: Array<lc.DocumentSymbol> | undefined = await | ||
vscode.commands.executeCommand( | ||
'move-analyzer.textDocumentDocumentSymbol', params, | ||
); | ||
|
||
assert.ok(syms); | ||
assert.deepStrictEqual(syms[0]?.kind, lc.SymbolKind.Module); | ||
assert.deepStrictEqual(syms[0].name, 'M1'); | ||
|
||
assert.ok(syms[0].children); | ||
assert.deepStrictEqual(syms[0]?.children[0]?.kind, lc.SymbolKind.Constant); | ||
assert.deepStrictEqual(syms[0]?.children[0].name, 'SOME_CONST'); | ||
assert.deepStrictEqual(syms[0]?.children[1]?.kind, lc.SymbolKind.Struct); | ||
assert.deepStrictEqual(syms[0]?.children[1].name, 'SomeOtherStruct'); | ||
assert.ok(syms[0].children[1].children); | ||
assert.deepStrictEqual(syms[0]?.children[1]?.children[0]?.kind, lc.SymbolKind.Field); | ||
assert.deepStrictEqual(syms[0]?.children[1]?.children[0]?.name, 'some_field'); | ||
assert.deepStrictEqual(syms[0]?.children[1].name, 'SomeOtherStruct'); | ||
assert.deepStrictEqual(syms[0]?.children[2]?.kind, lc.SymbolKind.Function); | ||
assert.deepStrictEqual(syms[0]?.children[2].name, 'some_other_struct'); | ||
assert.deepStrictEqual(syms[0]?.children[3]?.kind, lc.SymbolKind.Function); | ||
assert.deepStrictEqual(syms[0]?.children[3].name, 'this_is_a_test'); | ||
assert.deepStrictEqual(syms[0]?.children[3]?.detail, '["test", "expected_failure"]'); | ||
}); | ||
}); |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmwill, I was hoping that you could take a look at the changes in this file...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not obvious to me why the rust toolchain needs to get installed to be able to build a JS package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can at least try to respond to this one. The extended testing introduced in this PR involves the plugin (VSCode side) talking to the language server (which is written in Rust and presumably we need Rust tool chain to build its binary).