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.

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

Use didChangedWatchedFilesNotifications to trigger compilation #41

Open
tricktron opened this issue Aug 11, 2022 · 2 comments
Open

Use didChangedWatchedFilesNotifications to trigger compilation #41

tricktron opened this issue Aug 11, 2022 · 2 comments

Comments

@tricktron
Copy link
Owner

Currently, I trigger the compilation in the didSave notification. This leads to the following problem:

  1. Module B is importing module A.
  2. Make a change in module A (e.g. make an error leading to error diagnostics)
  3. Switch to module B.

Now module B does not know anything about the changes from module A and you need to manually save the module B again to pull in the changes, which is annoying.

@tricktron
Copy link
Owner Author

The didChangedWatchedFilesNotification does not help here because there is no notification to recognise the switch to module B in step 3.

I tried it out on the branch did-changed-watched-files-notification:

@Override
public void didChangeWatchedFiles(DidChangeWatchedFilesParams params)
{
// TODO: extract function to functional consumer, give it good name and call it in forEach
params.getChanges().forEach
(
change ->
{
URI uri = URI.create(change.getUri());
compileService.compileAndUpdateGlobals
(
uri,
fregeServer.getProjectService().getProjectGlobal()
);
DiagnosticService.publishCompilerDiagnostics
(
fregeServer.getClient(),
compileService.getGlobal(uri),
uri.toString()
);
}
);
}

After some thinking the problem can be solved in two ways either in step 2 or in step 3:

  • Step 2/all dependent diagnostics: Compute all dependent classes in step 2 when a change is made, and trigger recompilation of all dependent classes and publish the errors. Recompilation of all dependent classes is not really necessary, we could also directly publish to all dependent class a generic message like module A not built because of compilation errors, however then we get a UI - global delta which is also not nice.
  • Step 3/active window diagnostics: Use the new pull diagnostics mechanism introduced in lsp version 3.17. This allows to get a notification when a switch to module B happens. As a result, errors in dependent modules are shown or cleared when switching. For this example that means: if step 2 led to an error, then switching module B will show the error, if step 2 fixed the error, nothing happens because the errors of dependent modules are as of now not yet published but even if they were the error would be cleared.

Step 3 provides only a solution for the active window diagnostics. If you refactor some code, then you normally want to see if it breaks anything in the whole project. This may be achievable with pull diagnostics because it also supports a relatedDocuments field. It still needs the compute all dependent classes step so we are back to solution in step 2. Besides step 3/active window diagnostics can already be triggered manually by saving the file after switching.

Then there is actually also the new pull workspaceDiagnostics notification which allows to pull all workspace diagnostics.

Implementation Considerations from official lsp docs:

Generally the language server specification doesn’t enforce any specific client implementation since those usually depend on how the client UI behaves. However since diagnostics can be provided on a document and workspace level here are some tips:

  • a client should pull actively for the document the users types in.
  • if the server signals inter file dependencies a client should also pull for visible documents to ensure accurate diagnostics. However the pull should happen less frequently.
  • if the server signals workspace pull support a client should also pull for workspace diagnostics. It is recommended for clients to implement partial result progress for the workspace pull to allow servers to keep the request open for a long time. If a server closes a workspace diagnostic pull request the client should re-trigger the request.

@tricktron
Copy link
Owner Author

Best Case Scenario

  1. First compile the Module A, which was changed.
  2. Compile all files which depend on module A. That means we need to keep track of module dependencies in-memory.

Good Case Scenario

  1. First compile the Module A, which was changed.
  2. Compile all files in the project and rely on the caching ability of the Frege compiler, which only compiles the dependent modules anyway.

The Good Case Scenario has only one disadvantage: It needs to read every Frege file from disk. On the other side, it has the advantage that we don't need to keep track of module dependencies in-memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant