Skip to content

Add prepare rename provider #268

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

Merged
merged 5 commits into from
Jun 7, 2021
Merged

Add prepare rename provider #268

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d5c9db
Add prepare rename provider
amiralies Jun 6, 2021
30da51e
return false explicitly
amiralies Jun 6, 2021
d163d29
refactor completion
cristianoc Jun 7, 2021
f412eb7
Revert "refactor completion"
cristianoc Jun 7, 2021
08682f5
Don't return a boolean directly, but use result instead.
cristianoc Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,50 @@ function references(msg: p.RequestMessage) {
return response;
}

function prepareRename(msg: p.RequestMessage) {
// https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_prepareRename
let params = msg.params as p.PrepareRenameParams;
let filePath = fileURLToPath(params.textDocument.uri);
let locations: null | p.Location[] = utils.getReferencesForPosition(
filePath,
params.position
);

let result: p.Range | null = null;

if (locations !== null && locations.length > 0) {
let targetLoc = locations.find(loc => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering about the logic (and duplication of logic) here.
It seems intuitively, this should fire exactly when rename returns a non-empty set of changes.

Looking at the commands in Commands.ml, rename returns results always when locations is a non-empty array. So it seems this logic here is unnecessary. Or if necessary, how comes the same logic is not in the rename command?

Should this just call rename instead and check if the result is empty? Not sure why the protocol splits the logic in this way.

Also, just noticed utils.getReferencesForPosition was created when there was more than one use. Before this PR, there was a single use, so it should probably have been removed and inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the split is because of checking rename availability without actually analyzing refs. (I.e check the cursor position is an ident)

Ill look at it tomorrow to see if we can do it better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One difference w.r.t. rename is that the new name is not supplied. So technically, the rename command cannot be called.

I think this prepare operation makes most sense in an architecture when one has direct access to parsing the document, in which case prepareRename in some cases could be resolved by just parsing.

In our case, we make use of the function that finds references. Note this is more expensive, but it should not matter much as rename is interactive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see the command should find a range for the symbol. OK then the logic is necessary.

The only remaining comment I have is about returning a boolean directly, which seems to work on vscode but can't see supported in the spec.
Will make a suggestion for a change.

if (
path.normalize(fileURLToPath(loc.uri)) ===
path.normalize(fileURLToPath(params.textDocument.uri))
) {
let { start, end } = loc.range;
let pos = params.position;

return (
start.character <= pos.character &&
start.line <= pos.line &&
end.character >= pos.character &&
end.line >= pos.line
);
}

return false;
});

if (targetLoc != null) {
result = targetLoc.range;
}
}

let response: m.ResponseMessage = {
jsonrpc: c.jsonrpcVersion,
id: msg.id,
result
};
return response;
}

function rename(msg: p.RequestMessage) {
// https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_rename
let params = msg.params as p.RenameParams;
Expand Down Expand Up @@ -591,7 +635,7 @@ function onMessage(msg: m.Message) {
hoverProvider: true,
definitionProvider: true,
referencesProvider: true,
renameProvider: true,
renameProvider: { prepareProvider: true },
documentSymbolProvider: false,
completionProvider: { triggerCharacters: [".", ">", "@", "~"] },
},
Expand Down Expand Up @@ -642,6 +686,8 @@ function onMessage(msg: m.Message) {
send(definition(msg));
} else if (msg.method === p.ReferencesRequest.method) {
send(references(msg));
} else if (msg.method === p.PrepareRenameRequest.method) {
send(prepareRename(msg));
} else if (msg.method === p.RenameRequest.method) {
send(rename(msg));
} else if (msg.method === p.DocumentSymbolRequest.method) {
Expand Down