Fix references to file modules #254

amiralies · 2021-05-26T01:07:50Z

No description provided.

amiralies · 2021-05-26T01:08:33Z

This pr adds support for global references to a module.
if you have these structure.

// A.res
let a = 2

// B.res
let x = A.a

// C.res
let x = A.a

requesting refrence on A in either B or C results in references to all these three files.
note that reference to A itself is a special (0,0), (0,0) loc.

cristianoc · 2021-05-26T02:11:36Z

See how that interacts with rename:

cristianoc · 2021-05-26T02:13:47Z

There's also this confusing thing that the first line

let x = 12

is considered as a reference to References.res.
That would leave me puzzled as a user.

So basically 2 things to be solved in user experience.

amiralies · 2021-05-26T11:00:13Z

See how that interacts with rename:

Will fix that

is considered as a reference to References.res.

Do you think that's bad experience? for me it's like referencing to let binding when i get references on the usage site
for example

let x = 2

let b = x

if i get refrence on second x, it includes the first let binding which declares x.

cristianoc · 2021-05-26T11:16:34Z

This module reference is different. We want a reference to a module, we get the first line of a file. The user will look at the first line, and might think: how did I get here? That first line has nothing to do with the original query, except it happens to be in the file that the query is about.
It's not a terrible experience, but not a great experience either. That said, I'm not sure it's possible to do better.

amiralies · 2021-05-26T12:44:48Z

Yeah your argument is valid.
and it's happening becaus module decleration for file-modules is implicit, we don't have a exact location to refer to that so we refer to top of file.

amiralies · 2021-05-26T19:31:59Z

File rename demo

recording.mp4

cristianoc · 2021-05-26T21:13:43Z

server/src/server.ts

-        changes[uri].push(textEdit);
+      if (utils.isRangeTopOfFile(range)) {
+        let filePath = fileURLToPath(uri);
+        let newFilePath = `${path.dirname(filePath)}/${params.newName}${path.extname(filePath)}`;


what about Windows paths?

cristianoc · 2021-05-26T21:38:17Z

analysis/src/References.ml

+      | Some paths -> (
+        let moduleSrcToRef src = (Uri2.fromPath src, [Utils.topLoc src]) in
+        match paths with
+        | Impl (_, None) -> []


How about move this extraction of source files to SharedTypes alongside getSrc.
So that it returns a list of source files.

cristianoc

This is great, thanks a lot.
I've added a few comments.

In addition, how about adapting the test files so they cover all cases, including implementation and interface files, as well as references from another file than the current one.

amiralies · 2021-05-27T00:47:27Z

thank you i will address them.
I've a question, should we move rename stuff into analysis cli in order to test them separately or testing references is enough?

cristianoc · 2021-05-27T01:02:33Z

I think eventually it will have to move, as e.g. the isRangeTopOfFile check really belongs there, where one can make it less indirect.

amiralies · 2021-05-27T01:10:30Z

I will move if it's okay.

another thing have to be fixed. looks like there's a problem when the project has namespace: true in bsconfig.

cristianoc · 2021-05-27T01:23:29Z

Great go ahead with moving.

cristianoc · 2021-05-27T14:23:02Z

@amiralies meanwhile I've done some refactoring relevant to a couple of lines of this diff:
#256

Specifically, type paths in SharedTypes.ml.

I can merge and this gets rebased, or it can be done later.

amiralies · 2021-05-27T18:24:03Z

Maybe you can merge your branch then I will update my branch/PR

cristianoc · 2021-05-27T18:58:11Z

Merged

cristianoc · 2021-05-30T03:01:16Z

@amiralies a couple of next steps that fix build and clean up after rebase:
#261

Feel free to incorporate and continue. Or I'm also happy to continue if you prefer.

amiralies · 2021-05-30T07:05:31Z

I will merge your branch on mine, and continue the work here if it's okay.

cristianoc · 2021-05-30T07:06:07Z

Perfect

cristianoc · 2021-06-04T23:16:44Z

Windows tests are failing, I think because the order in which files are processed is nondeterministic. And can become platform-dependent.

cristianoc · 2021-06-04T23:20:23Z

analysis/src/Commands.ml

+      match References.locItemForPos ~full pos with
+      | None -> Protocol.null
+      | Some locItem ->
+        let allReferences = References.allReferencesForLocItem ~full locItem in


This list I think can be non-deterministic.
Some fixed order should be chosen (e.g. by sorting the results on Uri then loc).

It might be enough to make fileReferences inside extra in SharedTypes.ml store a set of locations instead of a list.

I will check what's going wrong on a windows setup.

Seems like there was an issue checking for interface file on windows

Great thanks. I've also looked at removing some nondeterminism:
#263

Which I might add later on. To make debugging across platforms easier.

amiralies · 2021-06-04T23:47:32Z

Works to be done aside from issue on windows:

Rename tests
Use rename command in server

amiralies · 2021-06-05T04:42:55Z

I think it's ready for review !

cristianoc · 2021-06-05T04:53:10Z

analysis/src/Commands.ml

+                 let dir = Filename.dirname path in
+                 let ext = Filename.extension path in
+                 let sep = Filename.dir_sep in
+                 let newPath = dir ^ sep ^ newName ^ ext in


Better use Filename.concat instead of string concat with sep.

cristianoc · 2021-06-05T04:55:10Z

analysis/src/Commands.ml

+                 let newUri = Uri2.fromPath newPath in
+                 Protocol.
+                   {
+                     kind = `rename;


Nit: I think it's easier to expose a constructor function in the protocol that only takes 2 parameters: oldUri and newUri so the type of kind does not leak here.

Actually, I would just remove kind and use in the printing function only.

cristianoc · 2021-06-05T04:56:51Z

analysis/src/Protocol.ml

@@ -18,6 +18,20 @@ type location = {uri : string; range : range}

 type documentSymbolItem = {name : string; kind : int; location : location}

+type renameFile = {kind : [`rename]; oldUri : string; newUri : string}


Suggestion: remove kind.

cristianoc · 2021-06-05T04:58:12Z

analysis/src/Protocol.ml

+  "oldUri": "%s",
+  "newUri": "%s"
+}|}
+    (match rf.kind with `rename -> "rename")


this can go once field kind is removed, but the printing of "rename" stays.

cristianoc · 2021-06-05T04:59:20Z

analysis/src/Utils.ml

@@ -7,6 +7,12 @@ let topLoc fname =
    loc_ghost = false;
  }

+let isTopLoc (loc : Warnings.loc) =


It might be possible to clean this up further, but looks OK for now.

cristianoc · 2021-06-05T05:00:25Z

Indeed, it looks great!
Left some minor comments as review.

amiralies · 2021-06-05T14:17:39Z

Thanks, I was going to bed. I see you already fixed them

cristianoc reviewed May 26, 2021

View reviewed changes

amiralies added 3 commits May 28, 2021 21:17

Fix references to file modules

6ddf611

Fix reference to interfaces

a328945

Add support for file renames

f15a838

amiralies force-pushed the fix-file-module-refs branch from ed9748c to f15a838 Compare May 28, 2021 16:47

amiralies mentioned this pull request May 29, 2021

Better file renaming experience #258

Closed

cristianoc added 2 commits May 30, 2021 04:54

Fix build after rebase.

13a09b3

Refactor getSrc

f8802ba

amiralies added 3 commits June 5, 2021 03:25

Add rename command

96ef240

Make dce analyzer happy

e31cf55

Add tests

53344ce

cristianoc reviewed Jun 4, 2021

View reviewed changes

amiralies added 3 commits June 5, 2021 08:26

Fix checking for interface file on windows

db7f838

Add rename tests

c025498

Update server to use rename command

b1dc77d

cristianoc reviewed Jun 5, 2021

View reviewed changes

cristianoc merged commit c9aab8a into rescript-lang:master Jun 5, 2021

		@@ -18,6 +18,20 @@ type location = {uri : string; range : range}

		type documentSymbolItem = {name : string; kind : int; location : location}

		type renameFile = {kind : [`rename]; oldUri : string; newUri : string}

Fix references to file modules #254

Fix references to file modules #254

Conversation

amiralies commented May 26, 2021

amiralies commented May 26, 2021

cristianoc commented May 26, 2021

cristianoc commented May 26, 2021

amiralies commented May 26, 2021

cristianoc commented May 26, 2021

amiralies commented May 26, 2021

amiralies commented May 26, 2021

Choose a reason for hiding this comment

cristianoc May 26, 2021 • edited Loading

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

amiralies commented May 27, 2021

cristianoc commented May 27, 2021

amiralies commented May 27, 2021

cristianoc commented May 27, 2021

cristianoc commented May 27, 2021

amiralies commented May 27, 2021

cristianoc commented May 27, 2021

cristianoc commented May 30, 2021

amiralies commented May 30, 2021

cristianoc commented May 30, 2021

cristianoc commented Jun 4, 2021

Choose a reason for hiding this comment

cristianoc Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amiralies commented Jun 4, 2021

amiralies commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jun 5, 2021 • edited Loading

amiralies commented Jun 5, 2021

cristianoc May 26, 2021 •

edited

Loading

cristianoc Jun 4, 2021 •

edited

Loading

cristianoc commented Jun 5, 2021 •

edited

Loading