Skip to content

Fixes for renaming #1924

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
Apr 16, 2025
Merged

Fixes for renaming #1924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
309ba6f
Fix issue with ident filtering
voodoos Apr 15, 2025
144d041
Fix Lid comparison
voodoos Apr 16, 2025
b922789
Add a changelog entry
voodoos Apr 16, 2025
98aa5e9
Promote ordering changes in tests
voodoos Apr 16, 2025
fb84e0a
Fix compat check
voodoos Apr 16, 2025
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
2 changes: 1 addition & 1 deletion .github/workflows/ocaml-lsp-compat.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@ jobs:

- name: Check that Merlin and OCaml-LSP are co-installable
run: |
opam --cli=2.1 pin --with-version=dev --no-action https://github.com/liam923/ocaml-lsp.git#stale-occurrences
opam --cli=2.1 pin --with-version=dev --no-action https://github.com/liam923/ocaml-lsp.git
opam --cli=2.1 pin --with-version=5.4-503 --no-action .
opam install ocaml-lsp-server --ignore-constraints-on=ocamlformat
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ unreleased
(#1882)
- `occurrences` now reports stale files (#1885)
- `inlay-hints` fix inlay hints on function parameters (#1923)
- Fix issues with ident validation and Lid comparison for occurrences (#1924)
+ ocaml-index
- Improve the granularity of index reading by segmenting the marshalization
of the involved data-structures. (#1889)
Expand Down
23 changes: 15 additions & 8 deletions src/analysis/locate.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,14 +620,16 @@ let find_loc_of_comp_unit ~config uid comp_unit =
log ~title "Failed to load the CU's cmt";
`None

let find_loc_of_uid ~config ~local_defs ~ident ?fallback (uid : Shape.Uid.t) =
let find_loc_of_uid ~config ~local_defs ?ident ?fallback (uid : Shape.Uid.t) =
let find_loc_of_item ~comp_unit =
match (find_loc_of_item ~config ~local_defs uid comp_unit, fallback) with
| Some { loc; txt }, _ when String.equal txt ident ->
match find_loc_of_item ~config ~local_defs uid comp_unit, fallback, ident with
| Some { loc; txt }, _, Some ident when String.equal txt ident ->
(* Checking the ident prevent returning nonsensical results when some uid
were swaped but the cmt files were not rebuilt. *)
Some (uid, loc)
| (Some _ | None), Some fallback ->
| Some { loc; _ }, _, None ->
Some (uid, loc)
| (Some _ | None), Some fallback, _ ->
find_loc_of_item ~config ~local_defs fallback comp_unit
|> Option.map ~f:(fun { Location.loc; _ } -> (fallback, loc))
| _ -> None
Expand Down Expand Up @@ -705,7 +707,7 @@ let rec uid_of_result ~traverse_aliases = function
| Approximated _ | Unresolved _ | Internal_error_missing_uid -> (None, true)

(** This is the main function here *)
let from_path ~config ~env ~local_defs ~decl path =
let from_path ~config ~env ~local_defs ~decl ?ident:_ path =
let title = "from_path" in
let unalias (decl : Env_lookup.item) =
if not config.traverse_aliases then (path, decl.uid)
Expand Down Expand Up @@ -752,11 +754,14 @@ let from_path ~config ~env ~local_defs ~decl path =
in
(* Step 2: Uid => Location *)
let loc =
let ident = Path.last path in
let ident =
(* TODO it might not be useful to check the ident without impl_uid *)
Path.last path
in
match impl_uid with
| Some impl_uid ->
find_loc_of_uid ~config ~local_defs ~ident ~fallback:uid impl_uid
| None -> find_loc_of_uid ~config ~local_defs ~ident uid
| None -> find_loc_of_uid ~config ~local_defs uid
in
let loc =
match loc with
Expand Down Expand Up @@ -792,7 +797,9 @@ let from_longident ~config ~env ~local_defs nss ident =
in
match Env_lookup.by_longident nss ident env with
| None -> `Not_in_env str_ident
| Some (path, decl) -> from_path ~config ~env ~local_defs ~decl path
| Some (path, decl) ->
let ident = Longident.last ident in
from_path ~config ~env ~local_defs ~decl ~ident path

let from_path ~config ~env ~local_defs ~namespace path =
File_switching.reset ();
Expand Down
4 changes: 1 addition & 3 deletions src/index-format/lid.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ let pp fmt t =

let compare_pos p1 p2 = Int.compare p1.cnum p2.cnum
let compare_filename t1 t2 =
String.compare
(Filename.basename (G.fetch t1.filename))
(Filename.basename (G.fetch t2.filename))
String.compare (G.fetch t1.filename) (G.fetch t2.filename)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👯


let compare t1 t2 =
match compare_filename t1 t2 with
Expand Down
24 changes: 12 additions & 12 deletions tests/test-dirs/occurrences/project-wide/for-renaming/r-modules-and-types.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,18 @@
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 5,
"col": 25
},
"end": {
"line": 5,
"col": 26
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/lib.ml",
"start": {
Expand Down Expand Up @@ -112,18 +124,6 @@
"col": 7
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 5,
"col": 25
},
"end": {
"line": 5,
"col": 26
},
"stale": false
}
],
"notifications": []
Expand Down
10 changes: 5 additions & 5 deletions tests/test-dirs/occurrences/project-wide/for-renaming/r-with-functors.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ We expect 2 occurrences in func.ml, 1 in func.mli and 2 in main.ml
"line": 1,
"col": 22
}
"$TESTCASE_ROOT/main.ml"
{
"line": 4,
"col": 16
}
"$TESTCASE_ROOT/func.ml"
{
"line": 1,
Expand All @@ -27,8 +32,3 @@ We expect 2 occurrences in func.ml, 1 in func.mli and 2 in main.ml
"line": 1,
"col": 24
}
"$TESTCASE_ROOT/main.ml"
{
"line": 4,
"col": 16
}
40 changes: 20 additions & 20 deletions tests/test-dirs/occurrences/project-wide/mli-vs-ml.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,37 +48,37 @@ the interface and the implementation.
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"file": "$TESTCASE_ROOT/main.mli",
"start": {
"line": 2,
"col": 5
"col": 8
},
"end": {
"line": 2,
"col": 6
"col": 9
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 3,
"col": 8
"line": 2,
"col": 5
},
"end": {
"line": 3,
"col": 9
"line": 2,
"col": 6
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.mli",
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 2,
"line": 3,
"col": 8
},
"end": {
"line": 2,
"line": 3,
"col": 9
},
"stale": false
Expand Down Expand Up @@ -107,37 +107,37 @@ Same when the cursor is at the origin:
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"file": "$TESTCASE_ROOT/main.mli",
"start": {
"line": 2,
"col": 5
"col": 8
},
"end": {
"line": 2,
"col": 6
"col": 9
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 3,
"col": 8
"line": 2,
"col": 5
},
"end": {
"line": 3,
"col": 9
"line": 2,
"col": 6
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.mli",
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 2,
"line": 3,
"col": 8
},
"end": {
"line": 2,
"line": 3,
"col": 9
},
"stale": false
Expand Down
48 changes: 24 additions & 24 deletions tests/test-dirs/occurrences/project-wide/prefix.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,38 +99,38 @@ Merlin successfully finds occurrences outside file when UNIT_NAME directive is u
"stale": false
},
{
"file": "$TESTCASE_ROOT/a.ml",
"file": "$TESTCASE_ROOT/b.ml",
"start": {
"line": 1,
"col": 12
"line": 2,
"col": 8
},
"end": {
"line": 1,
"col": 13
"line": 2,
"col": 9
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/a.ml",
"start": {
"line": 2,
"col": 18
"line": 1,
"col": 12
},
"end": {
"line": 2,
"col": 19
"line": 1,
"col": 13
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/b.ml",
"file": "$TESTCASE_ROOT/a.ml",
"start": {
"line": 2,
"col": 8
"col": 18
},
"end": {
"line": 2,
"col": 9
"col": 19
},
"stale": false
}
Expand Down Expand Up @@ -162,38 +162,38 @@ Merlin successfully finds occurrences outside file when WRAPPING_PREFIX directiv
"stale": false
},
{
"file": "$TESTCASE_ROOT/a.ml",
"file": "$TESTCASE_ROOT/b.ml",
"start": {
"line": 1,
"col": 12
"line": 2,
"col": 8
},
"end": {
"line": 1,
"col": 13
"line": 2,
"col": 9
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/a.ml",
"start": {
"line": 2,
"col": 18
"line": 1,
"col": 12
},
"end": {
"line": 2,
"col": 19
"line": 1,
"col": 13
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/b.ml",
"file": "$TESTCASE_ROOT/a.ml",
"start": {
"line": 2,
"col": 8
"col": 18
},
"end": {
"line": 2,
"col": 9
"col": 19
},
"stale": false
}
Expand Down
24 changes: 12 additions & 12 deletions tests/test-dirs/occurrences/project-wide/pwo-basic.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,38 +29,38 @@
"class": "return",
"value": [
{
"file": "$TESTCASE_ROOT/lib.ml",
"file": "$TESTCASE_ROOT/main.ml",
"start": {
"line": 1,
"col": 4
"col": 26
},
"end": {
"line": 1,
"col": 7
"col": 29
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/lib.ml",
"start": {
"line": 2,
"col": 22
"line": 1,
"col": 4
},
"end": {
"line": 2,
"col": 25
"line": 1,
"col": 7
},
"stale": false
},
{
"file": "$TESTCASE_ROOT/main.ml",
"file": "$TESTCASE_ROOT/lib.ml",
"start": {
"line": 1,
"col": 26
"line": 2,
"col": 22
},
"end": {
"line": 1,
"col": 29
"line": 2,
"col": 25
},
"stale": false
}
Expand Down
Loading
Loading