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

Show file tree
Hide file tree
Changes from 3 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
72 changes: 41 additions & 31 deletions analysis/src/Commands.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,16 @@ let dump files =

let completion ~path ~line ~col ~currentFile =
let uri = Uri2.fromPath path in
let pos = (line, col) in
let result =
let textOpt = Files.readFile currentFile in
NewCompletions.computeCompletions ~uri ~textOpt ~pos:(line, col)
let completionItems =
match NewCompletions.getCompletable ~textOpt ~pos with
| None -> []
| Some (completable, rawOpens) ->
NewCompletions.computeCompletions ~completable ~pos ~rawOpens ~uri
in
completionItems
|> List.map Protocol.stringifyCompletionItem
|> Protocol.array
in
Expand Down Expand Up @@ -143,38 +150,41 @@ let references ~path ~line ~col =

let documentSymbol ~path =
let uri = Uri2.fromPath path in
match ProcessCmt.getFullFromCmt ~uri with
| None -> print_endline Protocol.null
| Some {file} ->
let open SharedTypes in
let rec getItems {topLevel} =
let rec getItem = function
| MValue v -> (v |> SharedTypes.variableKind, [])
| MType (t, _) -> (t.decl |> SharedTypes.declarationKind, [])
| Module (Structure contents) -> (Module, getItems contents)
| Module (Constraint (_, modTypeItem)) -> getItem (Module modTypeItem)
| Module (Ident _) -> (Module, [])
let result =
match ProcessCmt.getFullFromCmt ~uri with
| None -> Protocol.null
| Some {file} ->
let open SharedTypes in
let rec getItems {topLevel} =
let rec getItem = function
| MValue v -> (v |> SharedTypes.variableKind, [])
| MType (t, _) -> (t.decl |> SharedTypes.declarationKind, [])
| Module (Structure contents) -> (Module, getItems contents)
| Module (Constraint (_, modTypeItem)) -> getItem (Module modTypeItem)
| Module (Ident _) -> (Module, [])
in
let fn {name = {txt}; extentLoc; item} =
let item, siblings = getItem item in
if extentLoc.loc_ghost then siblings
else (txt, extentLoc, item) :: siblings
in
let x = topLevel |> List.map fn |> List.concat in
x
in
let fn {name = {txt}; extentLoc; item} =
let item, siblings = getItem item in
if extentLoc.loc_ghost then siblings
else (txt, extentLoc, item) :: siblings
let allSymbols =
getItems file.contents
|> List.map (fun (name, loc, kind) ->
Protocol.stringifyDocumentSymbolItem
{
name;
location =
{uri = Uri2.toString uri; range = Utils.cmtLocToRange loc};
kind = SharedTypes.symbolKind kind;
})
in
let x = topLevel |> List.map fn |> List.concat in
x
in
let allSymbols =
getItems file.contents
|> List.map (fun (name, loc, kind) ->
Protocol.stringifyDocumentSymbolItem
{
name;
location =
{uri = Uri2.toString uri; range = Utils.cmtLocToRange loc};
kind = SharedTypes.symbolKind kind;
})
in
print_endline ("[\n" ^ (allSymbols |> String.concat ",\n") ^ "\n]")
"[\n" ^ (allSymbols |> String.concat ",\n") ^ "\n]"
in
print_endline result

let rename ~path ~line ~col ~newName =
let uri = Uri2.fromPath path in
Expand Down
49 changes: 24 additions & 25 deletions analysis/src/NewCompletions.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,33 +1133,32 @@ let processCompletable ~findItems ~full ~package ~rawOpens
Utils.startsWith name prefix && not (List.mem name identsSeen))
|> List.map mkLabel

let computeCompletions ~uri ~textOpt ~pos =
let getCompletable ~textOpt ~pos =
match textOpt with
| None -> []
| None -> None
| Some text -> (
match PartialParser.positionToOffset text pos with
| None -> []
| None -> None
| Some offset -> (
match PartialParser.findCompletable text offset with
| None -> []
| Some completable -> (
match ProcessCmt.getFullFromCmt ~uri with
| None -> []
| Some full ->
let rawOpens = PartialParser.findOpens text offset in
let package = full.package in
let allFiles =
FileSet.union package.projectFiles package.dependenciesFiles
in
let findItems ~exact parts =
let items =
getItems ~full ~package ~rawOpens ~allFiles ~pos ~parts
in
match parts |> List.rev with
| last :: _ when exact ->
items
|> List.filter (fun {SharedTypes.name = {txt}} -> txt = last)
| _ -> items
in
completable |> processCompletable ~findItems ~full ~package ~rawOpens)
))
| None -> None
| Some completable ->
let rawOpens = PartialParser.findOpens text offset in
Some (completable, rawOpens)))

let computeCompletions ~completable ~pos ~rawOpens ~uri =
match ProcessCmt.getFullFromCmt ~uri with
| None -> []
| Some full ->
let package = full.package in
let allFiles =
FileSet.union package.projectFiles package.dependenciesFiles
in
let findItems ~exact parts =
let items = getItems ~full ~package ~rawOpens ~allFiles ~pos ~parts in
match parts |> List.rev with
| last :: _ when exact ->
items |> List.filter (fun {SharedTypes.name = {txt}} -> txt = last)
| _ -> items
in
completable |> processCompletable ~findItems ~full ~package ~rawOpens
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