Skip to content

Don't produce duplicate type definitions for recursive types #7524

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
- `rescript-tools doc` no longer includes shadowed bindings in its output. https://github.com/rescript-lang/rescript/pull/7497
- Treat `throw` like `raise` in analysis. https://github.com/rescript-lang/rescript/pull/7521
- Fix `index out of bounds` exception thrown in rare cases by `rescript-editor-analysis.exe codeAction` command. https://github.com/rescript-lang/rescript/pull/7523
- Don't produce duplicate type definitions for recursive types on hover. https://github.com/rescript-lang/rescript/pull/7524

#### :nail_care: Polish

Expand Down
13 changes: 13 additions & 0 deletions analysis/src/Hover.ml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
open SharedTypes

module StringSet = Set.Make (String)

let showModuleTopLevel ~docstring ~isType ~name (topLevel : Module.item list) =
let contents =
topLevel
Expand Down Expand Up @@ -115,7 +117,18 @@ let expandTypes ~file ~package ~supportsMarkdownLinks typ =
],
`InlineType )
| all ->
let typesSeen = ref StringSet.empty in
let typeId ~(env : QueryEnv.t) ~name =
env.file.moduleName :: List.rev (name :: env.pathRev) |> String.concat "."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is based on Debug.debugPrintEnv

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's TypeUtils.findRootTypeId already, I wonder if that could cover this use case? It works on type_expr though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its full type is

TypeUtils.findRootTypeId ~(full : full) ~(env : QueryEnv.t) (t : Types.type_expr)

with type full = {extra: extra; file: File.t; package: package} and

type extra = {
  internalReferences: (int, Location.t list) Hashtbl.t;
  externalReferences:
    (string, (string list * Tip.t * Location.t) list) Hashtbl.t;
  fileReferences: (string, LocationSet.t) Hashtbl.t;
  mutable locItems: locItem list;
}

So my first question is would I need to build an extra value or would a dummy value like the following work?

{internalReferences: Hashtbl.empty, externalReferences: Hashtbl.empty, fileReferences: Hashtbl.empty, locItems: []}

As for getting type_expr values instead of Hover.extractedType values, I would need to modify Hover.findRelevantTypesFromType right? Or do you think I should be able to convert an extractedType to a type_expr?

type extractedType = {
  name: string;
  path: Path.t;
  decl: Types.type_declaration;
  env: SharedTypes.QueryEnv.t;
  loc: Warnings.loc;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's stick with your current approach!

in
( all
(* Don't produce duplicate type definitions for recursive types *)
|> List.filter (fun {env; name} ->
let typeId = typeId ~env ~name in
if StringSet.mem typeId !typesSeen then false
else (
typesSeen := StringSet.add typeId !typesSeen;
true))
|> List.map (fun {decl; env; loc; path} ->
let linkToTypeDefinitionStr =
if
Expand Down
7 changes: 7 additions & 0 deletions tests/analysis_tests/tests/src/Hover.res
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,13 @@ let payloadVariant = InlineRecord({field1: 1, field2: true})
let payloadVariant2 = Args(1, true)
// ^hov

module RecursiveVariants = {
type rec t = Action1(int) | Action2(float) | Batch(array<t>)
}

let recursiveVariant = RecursiveVariants.Action1(1)
// ^hov

// Hover on unsaved
// let fff = "hello"; fff
// ^hov
Expand Down
19 changes: 11 additions & 8 deletions tests/analysis_tests/tests/src/expected/Hover.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,13 @@ Hover src/Hover.res 258:22
Hover src/Hover.res 261:23
{"contents": {"kind": "markdown", "value": "```rescript\npayloadVariants\nArgs(int, bool)\n```\n\n---\n\n```\n \n```\n```rescript\ntype payloadVariants =\n | InlineRecord({field1: int, field2: bool})\n | Args(int, bool)\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Hover.res%22%2C256%2C0%5D)\n"}}

Hover src/Hover.res 265:23
Hover src/Hover.res 268:42
{"contents": {"kind": "markdown", "value": "```rescript\nRecursiveVariants.t\nAction1(int)\n```\n\n---\n\n```\n \n```\n```rescript\ntype RecursiveVariants.t =\n | Action1(int)\n | Action2(float)\n | Batch(array<t>)\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Hover.res%22%2C265%2C2%5D)\n"}}

Hover src/Hover.res 272:23
Nothing at that position. Now trying to use completion.
posCursor:[265:23] posNoWhite:[265:22] Found expr:[265:22->265:25]
Pexp_ident fff:[265:22->265:25]
posCursor:[272:23] posNoWhite:[272:22] Found expr:[272:22->272:25]
Pexp_ident fff:[272:22->272:25]
Completable: Cpath Value[fff]
Package opens Stdlib.place holder Pervasives.JsxModules.place holder
Resolved opens 1 Stdlib
Expand All @@ -323,10 +326,10 @@ Resolved opens 1 Stdlib
ContextPath string
{"contents": {"kind": "markdown", "value": "```rescript\nstring\n```"}}

Hover src/Hover.res 268:33
Hover src/Hover.res 275:33
Nothing at that position. Now trying to use completion.
posCursor:[268:33] posNoWhite:[268:32] Found expr:[268:31->268:40]
Pexp_ident someField:[268:31->268:40]
posCursor:[275:33] posNoWhite:[275:32] Found expr:[275:31->275:40]
Pexp_ident someField:[275:31->275:40]
Completable: Cpath Value[someField]
Package opens Stdlib.place holder Pervasives.JsxModules.place holder
Resolved opens 1 Stdlib
Expand All @@ -339,9 +342,9 @@ ContextPath Value[x]
Path x
{"contents": {"kind": "markdown", "value": "```rescript\nbool\n```"}}

Hover src/Hover.res 271:8
Hover src/Hover.res 278:8
{"contents": {"kind": "markdown", "value": "\n [`Belt.Array`]()\n\n **mutable array**: Utilities functions\n\n```rescript\nmodule Array: {\n module Id\n module Array\n module SortArray\n module MutableQueue\n module MutableStack\n module List\n module Range\n module Set\n module Map\n module MutableSet\n module MutableMap\n module HashSet\n module HashMap\n module Option\n module Result\n module Int\n module Float\n}\n```"}}

Hover src/Hover.res 274:6
Hover src/Hover.res 281:6
{"contents": {"kind": "markdown", "value": "```rescript\ntype aliased = variant\n```\n\n---\n\n```\n \n```\n```rescript\ntype variant = CoolVariant | OtherCoolVariant\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Hover.res%22%2C251%2C0%5D)\n"}}