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

Client update to connect to ViperServer's language server #113

Merged
merged 86 commits into from
Oct 3, 2022

Conversation

WissenIstNacht
Copy link

@WissenIstNacht WissenIstNacht commented Oct 12, 2020

closes #114
closes #115, i.e. same settings are still used and some of them are passed to ViperServer to configure verification. However, some settings values are unused
closes #116, i.e. we decided to simply remove debugging functionality

npm-debug.log
languageServerExample.log
server/
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the last line (as the server extension is obsolete)

Copy link
Author

Choose a reason for hiding this comment

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

This is not "server" as in the directory that holds the code for the legacy typescript server. That's the folder that held javascript files that bloated this PR.

@@ -43,7 +45,9 @@ export class State {

public static unitTest: UnitTestCallback;

public static autoVerify: boolean = true;
// OG
Copy link
Member

Choose a reason for hiding this comment

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

Unclear comments

if(!this.checkedSettings){
Log.error("Error getting timeout, there are no checked Settings available, default to no timeout.");
return 0;
if (!this.checkedSettings) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is a temporary stub, please add a comment that says so.

@@ -10,12 +10,22 @@
// The module 'vscode' contains the VS Code extensibility API
// Import the module and reference it with the alias vscode in your code below



//============================================================================//
Copy link
Member

Choose a reason for hiding this comment

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

This very useful comment should be duplicated in README.

State.client.onNotification(Commands.Log, (msg: { data: string, logLevel: LogLevel }) => {
Log.log((Log.logLevel >= LogLevel.Debug ? "S: " : "") + msg.data, msg.logLevel);
State.client.onNotification(Commands.Log, (data, logLevel) => {
// OG
Copy link
Member

Choose a reason for hiding this comment

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

What does OG stand for?

Copy link
Author

Choose a reason for hiding this comment

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

original

@@ -584,8 +591,7 @@ function considerStartingBackend(backendName: string) {
backend: backendName,
manuallyTriggered: true,
forceRestart: false,
isViperServerEngine: false //TODO: how to set that correctly

isViperServerEngine: true
Copy link
Member

Choose a reason for hiding this comment

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

This option is not necessary anymore as the concept of ViperServerEngine is obsolete. If it's easy to carve out the code that mentioned this concept, please do so; otherwise, add an explaination in the comments.

State.client.onNotification(Commands.Progress, (msg: { data: Progress, logLevel: LogLevel }) => {
Log.progress(msg.data, msg.logLevel);
});
State.client.onNotification(Commands.ToLogFile, (msg: { data: string, logLevel: LogLevel }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why are some of the notifications removed?

Copy link
Author

Choose a reason for hiding this comment

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

Because the Command.Error notification is currently not used by the server.

@@ -47,6 +52,7 @@ export class Commands {
static BackendReady = "BackendReady";//BackendReadyParams
Copy link
Member

Choose a reason for hiding this comment

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

What do these comments mean? e.g. //BackendReadyParams

Copy link
Author

Choose a reason for hiding this comment

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

I guess the author wrote them to indicate the type of object the corresponding method takes as input.

@@ -119,7 +119,9 @@ export class Log {
private static doLog(message: string, logLevel: LogLevel) {
let timing = this.logTiming ? this.prettyUptime() + ' ' : '';
message = this.prefix(logLevel) + message;
if (!Log.logLevel || Log.logLevel >= logLevel) {
// OG
Copy link
Member

Choose a reason for hiding this comment

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

Unclear comment

// The server is implemented in node
let serverModule = State.context.asAbsolutePath(path.join('server', 'server.js'));
// let serverModule = State.context.asAbsolutePath(path.join('server', 'server.js'));
Copy link
Member

Choose a reason for hiding this comment

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

FIXME

run: { module: serverModule, transport: TransportKind.ipc },
debug: { module: serverModule, transport: TransportKind.ipc, options: debugOptions }
}
// let serverOptions: ServerOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

FIXME

resolve();
});
Log.log("Initiating language server shutdown.", LogLevel.Info);
State.client.stop()
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not necessary anymore to shut down the server?

Copy link
Author

Choose a reason for hiding this comment

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

This is implicitly / automatically taken care of by taken care of by the LSP. The call to stop() initiates a "termination handshake" between client and server. The server is shut down during the course of that handshake.

let backendName = State.activeBackend;
let backendSettings = State.checkedSettings.verificationBackends.find(config => config.name == backendName)
return backendSettings.stoppingTimeout;
return 10000;
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment that this value should be taken from the settings (I assume).

let diag = new vscode.Diagnostic(range, item.message, item.severity-1);
diagnostics.push( diag );
})
let diagnostics: vscode.Diagnostic[] = params.diagnostics;
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

let diagnostics: vscode.Diagnostic[] = [];
JSON.parse(params.diagnostics).forEach( item => {
let range = new vscode.Range(item.range.start.line, item.range.start.character, item.range.end.line, item.range.end.character);
let diag = new vscode.Diagnostic(range, item.message, item.severity-1);
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that the yellow squiggly line issue comes from the fact that the severity level is not set properly.
If you want to use only one severity level, at least I would recommend to use the one that corresponds to "red" (i.e. error but not warning).

Copy link
Member

@aterga aterga left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I added some comments.

@WissenIstNacht
Copy link
Author

This PR is part of the ongoing refactoring process to outsource the extension's language server to ViperServer codebase (see the corresponding PR in ViperServer).

So far, Viper IDE extension comprised a client, a server, and a verification engine. With the refactoring of ViperServer, the server and the engine have been merged into a single codebase. I.e., most of the functionality that was previously part of the extension's server has been moved to ViperServer. Accordingly, the server's code has been deleted from this branch.

The extension's client has mostly been unaffected by these changes. Some adaptations have been made to allow the client to directly connect and communicate with ViperServer instead of connecting to the extension's server.

Some of the extension's features have been refactored yet. Refactoring steps that are yet to be done are documented by corresponding issues (#114, #115, #116)

Copy link
Member

@aterga aterga left a comment

Choose a reason for hiding this comment

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

Thank you for the revision.

Before merging the PR, the issues labeled "PR 113" should be resolved.

Additionally, we should add the compilation and debugging instructions to the README.

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