pony-lsp: Pass AST after every pass to lsp#5227
Conversation
For now only finer detailed compilation progress reporting, but also as a preparation for investigating the AST in earlier stages, e.g. right after parsing for replacing text based analysis.
ponylang-main
added
the
discuss during sync
|
So what is this building towards? |
|
It makes the Stuff like this, where all the passes messed up the actual AST, that makes it hard to analyze it. |
|
@mfelsche ack |
|
There is a lot of direct source code scanning in the inlay hints feature, see #5222. I'd love to replace that with AST analysis. So I'm very happy to see this. |
orien
left a comment
orien
left a comment
There was a problem hiding this comment.
Thanks for pursuing this 🙇
I'm not super familiar with the compilation process. So I have some questions.
| elseif this._client.supports_workspace_diagnostic_refresh() then | ||
| // we only need to issue a refresh if | ||
| // the client doesn't support publish | ||
| // diagnostics | ||
|
|
||
| // tell the client to refresh all | ||
| // diagnostics for us | ||
| this._request_sender.send_request( | ||
| Methods.workspace().diagnostic().refresh(), | ||
| None) |
There was a problem hiding this comment.
Do we still want to support this class of client?
| // group errors by file | ||
| let errors_by_file = Map[String, pc.Vec[JsonValue]].create() |
There was a problem hiding this comment.
To prevent unnecessary allocations, can we move this into the if done_compiling then block below?
There was a problem hiding this comment.
I will add a prealloc of 0 to create() in order to have no additional allocation when compilation is not done yet.
| // notify client about progress | ||
| let notification = this._progress_report_nofitication(program_dir, pass) | ||
| if this._client.supports_window_work_done_progress() then | ||
| this._channel.send(notification) | ||
| end |
There was a problem hiding this comment.
Can we move the notification creation into the conditional block?
| paths: Array[String val] val, | ||
| notify: CompilerNotify tag) | ||
| notify: CompilerNotify tag, | ||
| notify_passes: Array[PassId] val = [PassFinaliser]) |
There was a problem hiding this comment.
The trait definition of this behavior doesn't include the default value. Is that a problem?
| | (let program: Program val, PassParse) => | ||
| // parsing was successful | ||
| // TODO: use parse pass data already | ||
| // to pre-fill some package and module states | ||
| None |
There was a problem hiding this comment.
You mentioned that some of this data is mutable. Should we be worried about accessing it?
There was a problem hiding this comment.
yeah, you are absolutely right, the PonyCompiler actor is mutating the AST in parralel, while we are accessing it here. The compilation process needs to happen within the WorkspaceManager. I will change this, pls do not merge.
There was a problem hiding this comment.
Added a "do not merge" label as requested in the comment above
Co-authored-by: Orien Madgwick <497874+orien@users.noreply.github.com>
Co-authored-by: Orien Madgwick <497874+orien@users.noreply.github.com>
SeanTAllen
removed
the
discuss during sync
5 participants
For now only finer detailed compilation progress reporting, but also as a preparation for investigating the AST in earlier stages, e.g. right after parsing for replacing text based analysis.