Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Review for draft v2 #2

Closed
tanishiking opened this issue Sep 26, 2023 · 1 comment
Closed

Review for draft v2 #2

tanishiking opened this issue Sep 26, 2023 · 1 comment

Comments

@tanishiking
Copy link

https://mtshiba.github.io/documents/lsp_book_draft_v2.pdf に対するレビューです
https://prog-lang-sys-ja.zulipchat.com/#narrow/stream/343500-.E9.9B.91.E8.AB.87/topic/LSP.E3.81.AE.E6.9C.AC.E3.82.92.E6.9B.B8.E3.81.8D.E3.81.BE.E3.81.97.E3.81.9F/near/388313721

Review

全体

  • 例えば ServerCapabilities など、LSPで定義されているデータ型をまるまるコピペして書いているけれど、解説する一部のデータ型だけに絞って書いて、詳細は specification へのリンクを貼ってそっちを参照してねってするのでもいいかもしれません。
interface ClientCapabilities {
	...
	/**
   	 * Text document specific client capabilities.
   	 */
   	textDocument?: TextDocumentClientCapabilities;
   
   	/**
   	 * Capabilities specific to the notebook document support.
   	 *
   	 * @since 3.17.0
   	 */
   	notebookDocument?: NotebookDocumentClientCapabilities;
   	...
   }
  • リクエストや通知など、LSPの用語はそのまま Request Notifiation と書いたほうがわかりやすいんじゃないかなと思いますがどうでしょう?
  • 機能をサポートしていることを表現するのに "対応する" という言葉を使っているようですが、実装している・サポートする などの言葉のほうが分かりやすいのではないでしょうか? (対応するって何に対応している?リクエストに対応できるというつもり?)

細かいフィードバック

P13 通信は標準入出力を介して行われます。なので、両者のプロセスは完全に独立している訳
ではなく、サーバーはクライアントの子プロセスとして起動されます。

P14 ペイロードは JSON-RPCとなります。

JSON-RPC は RPC protocol なので、ペイロードが JSON-RPC っていう言い方は違和感があります。
"ペイロードは JSON-RPC message です" はどうでしょう?

P15 id と method 両方があるとそのメッセージはリクエストになります。id がない場合は notification(通知) と呼ばれる形式になります。

  • request には id と method が含まれ、notification には id が含まれません。という言い方のほうが 分かりやすい気がします。
    • (確かに TypeScript では id が無い struct は notification の型を conform するので、idがない場合は notification の形式になるという言い方は分かるのですが、これは実装の視点が強すぎる気がする)

P15 これはどちら かが一方的に送り応答を要求しません。

"これ" は notification だと思うのですが、先の文章で立て続けに request と notification の説明をしているので、何を指しているのかがわかりにくい気がします。"notification はサーバーかクライアントが一方的にメッセージを送り、応答は要求しません。" などでどうでしょう?

P15 params
各メッセージのパラメータを表します。具体的な「型」はメソッドによって異なります。

「型」-> データ形式 もしくは ペイロード?

P15 (error)

なんで括弧で囲われているんでしょう? typo?

P15 JSON-RPC の基礎的な部分としては以上となります。

ここまでは JSON-RPC の話ではなくて、LSP での (JSON-RPCをベースとしている) base protocol の話な気がします。JSON-RPC の話だったら mime type の話とかは関係ないはず
"LSPの基本的にな通信形式の解説は以上となります。"とか?

P16
capabilities
クライアントの能力を表します。クライアントとサーバーは、最初のメッセージで互いに
対応している機能を表す Client Capabilities と Server Capabilities を交換します。

  • "クライアント・サーバーがサポートしているLSPの機能のセットを表します。クライアントとサーバーは、initialize request/response で互いに、サポートしている機能を表す Client Capabilities と Server Capabilities を交換します。" などでどうでしょう?
  • 能力って何?って思いました、後ろでは対応している機能を表す...という表現を使っているので、それをそのまま書けばよいかと思います
  • 対応している機能 - 全体的なフィードバックにも書きましたが、対応しているっていう言葉少しわかりにくいなと思いました。サポートしている、などで良いのではないでしょうか
  • 最初のメッセージ - initialize リクエストがクライアントからサーバーに最初に送信されるべきメッセージだということが前提に書かれている気がします。わかっている人にしか伝われなそう。最初のメッセージ ではなく initialize リクエストで...という書き方の方が分かりやすい?
  • また"メッセージ"という書き方だと、クライアントからサーバーに送られるリクエストしか指しておらず、それではサーバー側が Server Capabilities をクライアントに response back することができない。ので、 initialize request/response で....としたほうが正しいかなと思いました。

P16 Server Capabilities で指定しなかった機能は、クライアントがサポートしていてもリクエストを送らないようになります。多くのリクエストはクライアント起点で送信されるので、クライアントが対応していない機能に関してサーバー側で何か対処したりする必要はありません。

これ、１つ目の文は Server Capabilities の説明で、2つめの文は Client Capabilities の説明ってことであってますか?

2文目
"Client Capabilities をサーバー側に伝えることにより、サーバー側はクライアントが対応していない機能に関するデータの処理を省略することができます。"

などどうでしょう?多くのリクエストはクライアント起点で送信される、とその次の文章を順接でつなげるのよく分かりませんでした。

P17 workspaceFolders
「ワークスペース」を指定するものです。多くのエディタでは、あるディレクトリ内に複数のルートを設定することが出来ます。この単位を LSP ではワークスペースと呼んでいます。これによって、例えば巨大なモノレポの中に複数のプロジェクトが存在し、それぞれ異なる依存関係を持つことをサーバーが認識できます。

この単位をLSPではワークスペースと呼んでいます。

  • (それぞれの workspace root のことなはずですが)どの単位? 複数のルート? もしくはそれぞれのルート? と曖昧な気がしました。
  • "それぞれの プロジェクトのルートフォルダーを ワークスペースと呼んでいます" とかどうでしょう?
  • 依存関係->ビルド設定?

P17 ファイルの URI は、file:/// で始まる絶対パスで表されま
す。例えば、/home/user/project/src/main.rs は file:///home/user/project/src/main.rs となります。

  • file とは限らないはず。LSP では URI schema に対する言及はない
  • 例えばサードパーティライブラリに含まれるファイルのuriを https://... で言及することもあります。

P18 Positionの定義

code:typescript
 /**
  * Defines an unsigned integer number in the range of 0 to 2^31 - 1.
  */
 export type uinteger = number;

も書いておくと親切?

P18 主に文字列の範囲を表しますが、カーソルの位置を表すのにも使われます。

カーソル範囲選択しているときは~ みたいなのも言及しても良いかも(どっちでもいい)

P22　JSON-RPC としてパ

JSON-RPC message として...

P23
2.. 呼び出し方式
3. 一体化方式
4. 独自実装方式

コメント:

  • 呼び出し方式: ScalaのIDEであるMetalsやIntelliJの一部製品は BuildServerProtocolというプロトコルを定義してたりします。 https://build-server-protocol.github.io/
  • Scala compiler も一部の quickfix や code navigation に組み込んでいるので、Scala の Metals は 2 + 3 って感じですね (コメント)
  • Eclipse JDT や、ほとんどの IntelliJ 製品、Visual C++ の cl.exe なども独自実装方式ですね、rust-analyzerよりそれらのほうが歴史はありそうですが、言及はしないのでしょうか。(eclipse.jtd.ls 以外は LS じゃないけど...)

P25 リクエスト (通知)

LSP において Request と Notification は別物なのでどっちやねんと思いました。
"通知" だけでよいのではないでしょうか? (全体フィードバックにも書きましたが、Notification のほうが個人的には良いと思います)

P26 解析の度に再送信して更新するか、エラーが全て除去されたな
らば空のレスポンスを送る必要があります。

何が空であるべき? かが曖昧だと思いました。
"Diagnostics が空リストな notification を送る必要があります" でどうでしょう?

P32 この際に情報の不足があるようならば、diagnostic を publish する段階で事前にサーバーがこのフィールドに必要なデータを入れておくのです。

"この際にサーバーが codeAction 生成に必要となるデータを、diagnostic の data プロパティを通じてクライアントに渡しておくのです。"

などはどうでしょう?

P34 モジュールシステムを持つ言語の場合、Location だけでは足りません。ファイル
パス (より一般にはコードの所在を特定できる識別子) の情報も持たせる必要があります

モジュールシステムの定義がよくわかってないのですが、プログラムを複数ファイルに分割できる(これはモジュールシステムではない?)ならパスは必要になると思います。
"モジュールシステムを持つ言語の場合、Location だけでは足りません。" この一文はなくても良いかも

P40 completionProvider を設定すると、指定された入力の際にクライアントから completion リクエストが送られてきます。

  • 指定された入力って何でしょうか?
  • "identifier の入力時と指定したtriggerCharactersが入力された際にクライアントからcompletionリクエストが送られてきます。" とか?
    • 実は "identifier" の入力をクライアントがどう検知しているのかよくわかっていない、どっかに正規表現かなにかあるのかな?

P49 グローバル変数は多くの言語で変更不能ですが、そのような場合、一度生成した補完項目は次回以降も使い回すことができます。

  • グローバル変数って何のことを指しているのでしょうか? トップレベルスコープにユーザーが定義する変数をグローバル変数だと思っているのですが、変更不能ってことはないので別のものを指している?
  • プログラミング言語がスコープに追加する pre-defined な値のことを指している?

P50 構文要素

構文要素というよりかは AST node のことですよね?
構文要素って token とか、tokenないのcharacterも含まれる気がする?

P51 パラメータとしては該当の不完全な補完候補が送られてきますので、欠けている部分を埋めて送り返します。

"不完全な" -> "documentation や details フィールドが欠けた補完候補が" でどうでしょう?

P53 ローカル変数の補完で、(まだインポートしていない) モジュールを補完候補に入れて、選択時に自動で import 宣言を加えるようにしたくなります。こ

  • これはローカル変数とは言えない気がします?
    • "現在のスコープから見えない変数をコード補完し、completion commit に自動で import 宣言をを加えるようにしたくなります。" でどうでしょう? と思ったけど、次のページでの "よりファジーな補完" との違いがよくわからなくなった?
    • ローカル変数補完でインポートしてないモジュールを補完候補に入れるっていうののイメージがつかないんですが、どういう状況ですか?
  • あと"選択"という言葉を補完候補にカーソルを合わせることと completion を確定すること両方に使っているように見えます。紛らわしいので違う言葉を使えると良いですね

P54 よりファジーな補完として、その名前空間にない変数も補完の候補に含め、accept と同時に自動で import してしまうというアイデアもあります。

  • これは先のまだインポートしてないモジュールを...と何が違うのでしょうか?
  • accept というのは completion commit のことでしょうか? ~accept という言葉は広く使われている? -> LSP specification でも使われてた。

P54 更新されうることをクライアントに伝えることができます。ある程度文字数が入力されたら、未インポートのシンボルも探索し始めるといった実装ができます。

"これを利用して、最初の1文字入力したときのcompletionでは、未インポートのシンボルは探索せず、ある程度文字数が入力されたら、それらのシンボルも探索し、CompletionListを更新するといった実装も出来ます。" とかだとどうでしょうか?

P57 Inlay hints は、コード上に省略された情報を表示する機能です。バージョン 3.17 で追加された新しめの機能で、それまでは VSCode の独自仕様でした。

  • 確かに Inlay Hints を表示するための VSCode API である decorator は VSCode 独自の機能(ほんまか? 別に似たような機能は別のエディタにもあると思う?)
  • Inlay Hint という形式で LSP に定めていたのは rust-analyzer の独自仕様で、LSP 3.17 で rust-analyzer から inlay hint が逆輸入された Feature: Inlay Hints · Issue #956 · microsoft/language-server-protocol
  • なので、個人的には "それまでは rust-analyzer の独自仕様" でしたという方が正しく感じます。

P66 変数情報の中には定義箇所の情報も含まれているので、これを返せばよいことになります。

  • "変数情報の中には定義箇所の情報も含まれている"
    • これは erg compiler の話ですか? 主語がないので任意のコンパイラ/型チェッカーの変数情報の中には...と読めてしまう。
    • だいたいのコンパイラの "変数情報" にはだいたい定義箇所の情報、もしくはそれを引くために十分な情報(シンボルなど)があるはずではあるが...
  • "変数情報を利用して定義箇所の情報を取得し、それを返せば良い" などでどうでしょうか?

P80 only アクションの種類を指定します。

"サーバーから取得する CodeAction の種類を指定します。何もしないしない場合はすべてのCodeActionを取得します。" でどうでしょう?
@mtshiba
Copy link
Owner

mtshiba commented Sep 29, 2023

ご指摘の箇所について修正・加筆しました。

レビューありがとうございました！

@mtshiba mtshiba closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tanishiking @mtshiba