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

[DSLX:LS] Diagnostics updates for dependent modules. #1666

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cdleary
Copy link
Collaborator

@cdleary cdleary commented Oct 21, 2024

"Fixes" inconsistency previously observed in the handling across files in the language server.

Previously if you made a change to leaf.x that unbroke a module that depended on leaf.x, you wouldn't naturally get an update to the "problems" pane in VSCode that indicated the problem had been resolved.

Introduces a very lightweight virtualized filesystem layer so we can see updated text in the editor (language server) workspace reflected in the language server results.

Screencast.from.10-20-2024.02.19.10.PM.webm

Previously if you made a change to `leaf.x` that unbroke a module that
depended on `leaf.x`, you wouldn't naturally get an update to the
"problems" pane in VSCode that indicated the problem had been resolved.

Introduces a very lightweight virtualized filesystem layer so we can see
updated text in the editor (language server) workspace reflected in the
language server results.

Update before review.
Copy link
Member

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not reviewed in-depth yet, but I think there might be something needed to take care of files that are removed, i.e. closed in the editor as that would change where files are coming from.

As an FYI, there is a last_global_version() value in the EditTextBuffer that tells you when it was last updated (just a number that increases with each global edit). It might or might not be useful in cases where you want to decide if it is necessary to re-parse a subset of file.

// being an import-time error to having no import-time error).
std::vector<std::string> sensitive_uris =
adapter.import_sensitivity().GatherAllSensitiveToChangeIn(file_uri);
for (std::string sensitive_uri : sensitive_uris) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a const reference.

.diagnostics = adapter.GenerateParseDiagnostics(sensitive_uri),
};
dispatcher.SendNotification("textDocument/publishDiagnostics", params);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't add comment below in github UI, but around line 190 below you get a callback whenever a file is removed from the editor (i.e. file got closed in the editor). In that case you probably need to do something as you then need to fallback to the regular filesystem in that context, i.e. need to remove it from your virtual filesystem.

      [&](const std::string& uri, const EditTextBuffer* buffer) {
        if (buffer == nullptr) {
          return;  // <- remove from virtual filesystem ?
        }
        TextChangeHandler(uri, *buffer, dispatcher, language_server_adapter);
      });

@cdleary
Copy link
Collaborator Author

cdleary commented Oct 21, 2024

I mentioned that in a TODO in the change around workspace commands, this is still a strict improvement

@hzeller
Copy link
Member

hzeller commented Oct 21, 2024

Sounds good, just wanted to make sure that you're not running into dangling references of non-existent files, but sounds like worst thing that can happen is that the language server just stays at the last in-memory version it has seen in the editor. Other than that, this is indeed strictly better.

Can you run the clang-tidy on the files ? There seem to be some missing includes; maybe you can minimize the number of files to process to change the start directory to xls/dslx/lsp around here.
(Sorry, no clang-tidy commenter in the github CI yet...)

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

Successfully merging this pull request may close these issues.

2 participants