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.

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

Fix renaming of labelled arguments #872

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix renaming of labelled arguments #872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion analysis/src/ProcessExtra.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,16 @@ let typ ~env ~extra (iter : Tast_iterator.iterator) (item : Typedtree.core_type)

let pat ~(file : File.t) ~env ~extra (iter : Tast_iterator.iterator)
(pattern : Typedtree.pattern) =
let addForPattern stamp name =
let addForPattern stamp (name : string Location.loc) =
if Stamps.findValue file.stamps stamp = None then (
let name =
match
pattern.pat_attributes
|> List.find_opt (fun ({Asttypes.txt}, _) -> txt = "res.namedArgLoc")
with
| None -> name
| Some ({loc}, _) -> {name with loc}
in
let declared =
ProcessAttributes.newDeclared ~name ~stamp ~modulePath:NotVisible
~extent:pattern.pat_loc ~item:pattern.pat_type false
Expand Down
3 changes: 3 additions & 0 deletions analysis/tests/src/Rename.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@ let c = x

let foo = (~xx) => xx + 1
// ^ren yy

let foo2 = (~xx) => xx + 1
// ^ren yy
Copy link
Collaborator

Choose a reason for hiding this comment

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

So foo was working and foo2 wasn't?
Not sure I get what's going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They both work, but when for foo the tilde is dropped from ~xx so it becomes (yy) => ..., and for foo2 the tilde was added to the body (~yy) => ~yy + 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps hacking renaming itself to special-case tilde does not break other things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my first idea but it might be a little bit of extra work, because we'll need to read the text from the loc and have a look at that. There's no information carried on that can help us when producing the rename instructions. Might be the best idea anyway though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my first idea but it might be a little bit of extra work, because we'll need to read the text from the loc and have a look at that. There's no information carried on that can help us when producing the rename instructions. Might be the best idea anyway though.

Not sure it makes sense, but perhaps when the length does not match. E.g. if the loc says 3 and it's supposed to be 2. Though that's risky.

12 changes: 8 additions & 4 deletions analysis/tests/src/expected/Hover.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ Hover src/Hover.res 33:4
{"contents": {"kind": "markdown", "value": "```rescript\nunit => int\n```\n\nDoc comment for functionWithTypeAnnotation"}}

Hover src/Hover.res 37:13
getLocItem #5: heuristic for JSX and compiler combined:
~x becomes Props#x
heuristic for: [Props, x], give loc of `x`
getLocItem #7: heuristic for JSX on type-annotated labeled (~arg:t):
(~arg:t) becomes Props#arg
Props has the location range of arg:t
arg has the location range of arg
heuristic for: [Props, arg], give loc of `arg`
n1:Props n2:name
{"contents": {"kind": "markdown", "value": "```rescript\nstring\n```"}}

Expand Down Expand Up @@ -74,7 +76,9 @@ Hover src/Hover.res 103:25
{"contents": {"kind": "markdown", "value": "```rescript\nfloat\n```"}}

Hover src/Hover.res 106:21
{"contents": {"kind": "markdown", "value": "```rescript\nint\n```"}}
Nothing at that position. Now trying to use completion.
posCursor:[106:21] posNoWhite:[106:19] Found expr:[106:13->106:36]
null

Hover src/Hover.res 116:16
{"contents": {"kind": "markdown", "value": "```rescript\nAA.cond<[< #str(string)]> => AA.cond<[< #str(string)]>\n```\n\n---\n\n```\n \n```\n```rescript\ntype AA.cond<'a> = 'a\n constraint 'a = [< #str(string)]\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Hover.res%22%2C110%2C2%5D)\n"}}
Expand Down
2 changes: 1 addition & 1 deletion analysis/tests/src/expected/References.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ References src/References.res 0:4

References src/References.res 9:19
[
{"uri": "References.res", "range": {"start": {"line": 9, "character": 11}, "end": {"line": 9, "character": 14}}},
{"uri": "References.res", "range": {"start": {"line": 9, "character": 12}, "end": {"line": 9, "character": 14}}},
{"uri": "References.res", "range": {"start": {"line": 9, "character": 19}, "end": {"line": 9, "character": 21}}}
]

Expand Down
19 changes: 18 additions & 1 deletion analysis/tests/src/expected/Rename.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Rename src/Rename.res 9:19 yy
"uri": "Rename.res"
},
"edits": [{
"range": {"start": {"line": 9, "character": 11}, "end": {"line": 9, "character": 14}},
"range": {"start": {"line": 9, "character": 12}, "end": {"line": 9, "character": 14}},
"newText": "yy"
}, {
"range": {"start": {"line": 9, "character": 19}, "end": {"line": 9, "character": 21}},
Expand All @@ -35,3 +35,20 @@ Rename src/Rename.res 9:19 yy
}
]

Rename src/Rename.res 12:14 yy
[
{
"textDocument": {
"version": null,
"uri": "Rename.res"
},
"edits": [{
"range": {"start": {"line": 12, "character": 13}, "end": {"line": 12, "character": 15}},
"newText": "yy"
}, {
"range": {"start": {"line": 12, "character": 20}, "end": {"line": 12, "character": 22}},
"newText": "yy"
}]
}
]

10 changes: 0 additions & 10 deletions analysis/tests/src/expected/Xform.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,6 @@ let foo = x => {
}

Xform src/Xform.res 34:21
Hit: Add type annotation
{"start": {"line": 34, "character": 24}, "end": {"line": 34, "character": 24}}
newText:
<--here
: int

Xform src/Xform.res 38:5
Hit: Add Documentation template
Expand All @@ -93,11 +88,6 @@ newText:
let make = (~name) => React.string(name)

Xform src/Xform.res 41:9
Hit: Add type annotation
{"start": {"line": 41, "character": 11}, "end": {"line": 41, "character": 11}}
newText:
<--here
: int

Xform src/Xform.res 48:21
posCursor:[48:21] posNoWhite:[48:19] Found expr:[48:15->48:25]
Expand Down
Loading