Skip to content

Fix issue where jump to definition would go to the wrong place when there are aliased identifiers in submodules #653

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 4 commits into from
Dec 15, 2022
Merged

Fix issue where jump to definition would go to the wrong place when there are aliased identifiers in submodules #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e220d26
Add an example of wrong reference with inner modules, .resi, and shad…
cristianoc Dec 15, 2022
d32dfa4
Use module path instead of simply the id to find references across re…
cristianoc Dec 15, 2022
dc067e5
refactor
cristianoc Dec 15, 2022
786b6c8
Refactor env module path construction.
cristianoc Dec 15, 2022
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: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@

- Fix issue with references from implementation files which also happen to have interface files https://github.com/rescript-lang/rescript-vscode/issues/645

- Fix issue where jump to definition would go to the wrong place when there are aliased identifiers in submodules https://github.com/rescript-lang/rescript-vscode/pull/653

## v1.8.2

#### :rocket: New Feature
Expand Down
32 changes: 15 additions & 17 deletions analysis/src/Hover.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ let hoverWithExpandedTypes ~docstring ~file ~package ~supportsMarkdownLinks typ
Markdown.goToDefinitionText ~env ~pos:loc.Warnings.loc_start
else ""
in
Markdown.divider ^ (if supportsMarkdownLinks then Markdown.spacing else "")
Markdown.divider
^ (if supportsMarkdownLinks then Markdown.spacing else "")
^ Markdown.codeBlock
(decl
|> Shared.declToString ~printNameAsIs:true
Expand Down Expand Up @@ -183,24 +184,21 @@ let newHover ~full:{file; package} ~supportsMarkdownLinks locItem =
| None -> None
| Some file -> (
let env = QueryEnv.fromFile file in
match ResolvePath.resolvePath ~env ~path ~package with
match References.exportedForTip ~env ~path ~package ~tip with
| None -> None
| Some (env, name) -> (
match References.exportedForTip ~env name tip with
| Some (_env, _name, stamp) -> (
match Stamps.findModule file.stamps stamp with
| None -> None
| Some stamp -> (
match Stamps.findModule file.stamps stamp with
| Some md -> (
match References.resolveModuleReference ~file ~package md with
| None -> None
| Some md -> (
match References.resolveModuleReference ~file ~package md with
| None -> None
| Some (file, declared) ->
let name, docstring =
match declared with
| Some d -> (d.name.txt, d.docstring)
| None -> (file.moduleName, file.structure.docstring)
in
showModule ~docstring ~name ~file declared)))))
| Some (file, declared) ->
let name, docstring =
match declared with
| Some d -> (d.name.txt, d.docstring)
| None -> (file.moduleName, file.structure.docstring)
in
showModule ~docstring ~name ~file declared))))
| LModule NotFound -> None
| TopLevelModule name -> (
match ProcessCmt.fileForModule ~package name with
Expand Down Expand Up @@ -250,4 +248,4 @@ let newHover ~full:{file; package} ~supportsMarkdownLinks locItem =
let typeString, docstring = t |> fromType ~docstring in
typeString :: docstring)
in
Some (String.concat "\n\n" parts)
Some (String.concat "\n\n" parts)
38 changes: 5 additions & 33 deletions analysis/src/ProcessCmt.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ let rec forSignatureItem ~env ~(exported : Exported.t)
decl |> forTypeDeclaration ~env ~exported ~recStatus)
| Tsig_module
{md_id; md_attributes; md_loc; md_name = name; md_type = {mty_type}} ->
let item = forTypeModule env mty_type in
let item = forTypeModule (env |> Env.addModule ~name:name.txt) mty_type in
let declared =
addDeclared ~item ~name ~extent:md_loc ~stamp:(Ident.binding_time md_id)
~env md_attributes
Expand Down Expand Up @@ -374,14 +374,7 @@ let rec forStructureItem ~env ~(exported : Exported.t) item =
mtd_type = Some {mty_type = modType};
mtd_loc;
} ->
let env =
{
env with
modulePath =
ExportedModule
{name = name.txt; modulePath = env.modulePath; isType = true};
}
in
let env = env |> Env.addModuleType ~name:name.txt in
let modTypeItem = forTypeModule env modType in
let declared =
addDeclared ~item:modTypeItem ~name ~extent:mtd_loc
Expand Down Expand Up @@ -429,14 +422,7 @@ and forModule env mod_desc moduleName =
match mod_desc with
| Tmod_ident (path, _lident) -> Ident path
| Tmod_structure structure ->
let env =
{
env with
modulePath =
ExportedModule
{name = moduleName; modulePath = env.modulePath; isType = false};
}
in
let env = env |> Env.addModule ~name:moduleName in
let contents = forStructure ~env structure.str_items in
Structure contents
| Tmod_functor (ident, argName, maybeType, resultExpr) ->
Expand All @@ -456,26 +442,12 @@ and forModule env mod_desc moduleName =
| Tmod_apply (functor_, _arg, _coercion) ->
forModule env functor_.mod_desc moduleName
| Tmod_unpack (_expr, moduleType) ->
let env =
{
env with
modulePath =
ExportedModule
{name = moduleName; modulePath = env.modulePath; isType = false};
}
in
let env = env |> Env.addModule ~name:moduleName in
forTypeModule env moduleType
| Tmod_constraint (expr, typ, _constraint, _coercion) ->
(* TODO do this better I think *)
let modKind = forModule env expr.mod_desc moduleName in
let env =
{
env with
modulePath =
ExportedModule
{name = moduleName; modulePath = env.modulePath; isType = false};
}
in
let env = env |> Env.addModule ~name:moduleName in
let modTypeKind = forTypeModule env typ in
Constraint (modKind, modTypeKind)

Expand Down
186 changes: 80 additions & 106 deletions analysis/src/References.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,23 @@ let getConstructor (file : File.t) stamp name =
| Some const -> Some const)
| _ -> None)

let exportedForTip ~(env : QueryEnv.t) name (tip : Tip.t) =
match tip with
| Value -> Exported.find env.exported Exported.Value name
| Field _ | Constructor _ | Type ->
Exported.find env.exported Exported.Type name
| Module -> Exported.find env.exported Exported.Module name
let exportedForTip ~env ~path ~package ~(tip : Tip.t) =
match ResolvePath.resolvePath ~env ~path ~package with
| None ->
Log.log ("Cannot resolve path " ^ pathToString path);
None
| Some (env, name) -> (
let kind =
match tip with
| Value -> Exported.Value
| Field _ | Constructor _ | Type -> Exported.Type
| Module -> Exported.Module
in
match Exported.find env.exported kind name with
| None ->
Log.log ("Exported not found for tip " ^ name ^ " > " ^ Tip.toString tip);
None
| Some stamp -> Some (env, name, stamp))

let definedForLoc ~file ~package locKind =
let inner ~file stamp (tip : Tip.t) =
Expand Down Expand Up @@ -180,42 +191,17 @@ let definedForLoc ~file ~package locKind =
None
| Some file -> (
let env = QueryEnv.fromFile file in
match ResolvePath.resolvePath ~env ~path ~package with
| None ->
Log.log ("Cannot resolve path " ^ pathToString path);
None
| Some (env, name) -> (
match exportedForTip ~env name tip with
match exportedForTip ~env ~path ~package ~tip with
| None -> None
| Some (env, name, stamp) -> (
maybeLog ("Getting for " ^ string_of_int stamp ^ " in " ^ name);
match inner ~file:env.file stamp tip with
| None ->
Log.log
("Exported not found for tip " ^ name ^ " > " ^ Tip.toString tip);
Log.log "could not get defined";
None
| Some stamp -> (
maybeLog ("Getting for " ^ string_of_int stamp ^ " in " ^ name);
match inner ~file:env.file stamp tip with
| None ->
Log.log "could not get defined";
None
| Some res ->
maybeLog "Yes!! got it";
Some res))))

let declaredForExportedTip ~(stamps : Stamps.t) ~(exported : Exported.t) name
(tip : Tip.t) =
let bind f x = Option.bind x f in
match tip with
| Value ->
Exported.find exported Exported.Value name
|> bind (fun stamp -> Stamps.findValue stamps stamp)
|> Option.map (fun x -> {x with Declared.item = ()})
| Field _ | Constructor _ | Type ->
Exported.find exported Exported.Type name
|> bind (fun stamp -> Stamps.findType stamps stamp)
|> Option.map (fun x -> {x with Declared.item = ()})
| Module ->
Exported.find exported Exported.Module name
|> bind (fun stamp -> Stamps.findModule stamps stamp)
|> Option.map (fun x -> {x with Declared.item = ()})
| Some res ->
maybeLog "Yes!! got it";
Some res)))

(** Find alternative declaration: from res in case of interface, or from resi in case of implementation *)
let alternateDeclared ~(file : File.t) ~package (declared : _ Declared.t) tip =
Expand All @@ -230,10 +216,16 @@ let alternateDeclared ~(file : File.t) ~package (declared : _ Declared.t) tip =
match Cmt.fullFromUri ~uri:(Uri.fromPath alternateUri) with
| None -> None
| Some {file; extra} -> (
match
declaredForExportedTip ~stamps:file.stamps
~exported:file.structure.exported declared.name.txt tip
with
let env = QueryEnv.fromFile file in
let path = ModulePath.toPath declared.modulePath declared.name.txt in
maybeLog ("find declared for path " ^ pathToString path);
let declaredOpt =
match exportedForTip ~env ~path ~package ~tip with
| None -> None
| Some (_env, _name, stamp) ->
declaredForTip ~stamps:file.stamps stamp tip
in
match declaredOpt with
| None -> None
| Some declared -> Some (file, extra, declared)))
| _ ->
Expand Down Expand Up @@ -382,16 +374,12 @@ let definitionForLocItem ~full:{file; package} locItem =
| None -> None
| Some file -> (
let env = QueryEnv.fromFile file in
match ResolvePath.resolvePath ~env ~path ~package with
match exportedForTip ~env ~path ~package ~tip with
| None -> None
| Some (env, name) -> (
maybeLog ("resolved path:" ^ name);
match exportedForTip ~env name tip with
| None -> None
| Some stamp ->
(* oooh wht do I do if the stamp is inside a pseudo-file? *)
maybeLog ("Got stamp " ^ string_of_int stamp);
definition ~file:env.file ~package stamp tip)))
| Some (env, _name, stamp) ->
(* oooh wht do I do if the stamp is inside a pseudo-file? *)
maybeLog ("Got stamp " ^ string_of_int stamp);
definition ~file:env.file ~package stamp tip))

let digConstructor ~env ~package path =
match ResolvePath.resolveFromCompilerPath ~env ~package path with
Expand Down Expand Up @@ -425,7 +413,7 @@ let typeDefinitionForLocItem ~full:{file; package} locItem =
let isVisible (declared : _ Declared.t) =
declared.isExported
&&
let rec loop v =
let rec loop (v : ModulePath.t) =
match v with
| File _ -> true
| NotVisible -> false
Expand All @@ -434,17 +422,6 @@ let isVisible (declared : _ Declared.t) =
in
loop declared.modulePath

let rec pathFromVisibility visibilityPath current =
match visibilityPath with
| File _ -> Some current
| IncludedModule (_, inner) -> pathFromVisibility inner current
| ExportedModule {name; modulePath = inner} ->
pathFromVisibility inner (name :: current)
| NotVisible -> None

let pathFromVisibility visibilityPath tipName =
pathFromVisibility visibilityPath [tipName]

type references = {
uri: Uri.t;
locOpt: Location.t option; (* None: reference to a toplevel module *)
Expand Down Expand Up @@ -497,35 +474,35 @@ let forLocalStamp ~full:{file; extra; package} stamp (tip : Tip.t) =
(* if this file has a corresponding interface or implementation file
also find the references in that file *)
in
match pathFromVisibility declared.modulePath declared.name.txt with
| None -> []
| Some path ->
maybeLog ("Now checking path " ^ pathToString path);
let thisModuleName = file.moduleName in
let externals =
package.projectFiles |> FileSet.elements
|> List.filter (fun name -> name <> file.moduleName)
|> List.map (fun moduleName ->
Cmt.fullsFromModule ~package ~moduleName
|> List.map (fun {file; extra} ->
match
Hashtbl.find_opt extra.externalReferences
thisModuleName
with
| None -> []
| Some refs ->
let locs =
refs
|> Utils.filterMap (fun (p, t, locs) ->
if p = path && t = tip then Some locs
else None)
in
locs
|> List.map (fun loc ->
{uri = file.uri; locOpt = Some loc})))
|> List.concat |> List.concat
in
alternativeReferences @ externals)
let path =
ModulePath.toPath declared.modulePath declared.name.txt
in
maybeLog ("Now checking path " ^ pathToString path);
let thisModuleName = file.moduleName in
let externals =
package.projectFiles |> FileSet.elements
|> List.filter (fun name -> name <> file.moduleName)
|> List.map (fun moduleName ->
Cmt.fullsFromModule ~package ~moduleName
|> List.map (fun {file; extra} ->
match
Hashtbl.find_opt extra.externalReferences
thisModuleName
with
| None -> []
| Some refs ->
let locs =
refs
|> Utils.filterMap (fun (p, t, locs) ->
if p = path && t = tip then Some locs
else None)
in
locs
|> List.map (fun loc ->
{uri = file.uri; locOpt = Some loc})))
|> List.concat |> List.concat
in
alternativeReferences @ externals)
else (
maybeLog "Not visible";
[])
Expand Down Expand Up @@ -577,17 +554,14 @@ let allReferencesForLocItem ~full:({file; package} as full) locItem =
| None -> []
| Some file -> (
let env = QueryEnv.fromFile file in
match ResolvePath.resolvePath ~env ~path ~package with
match exportedForTip ~env ~path ~package ~tip with
| None -> []
| Some (env, name) -> (
match exportedForTip ~env name tip with
| Some (env, _name, stamp) -> (
match Cmt.fullFromUri ~uri:env.file.uri with
| None -> []
| Some stamp -> (
match Cmt.fullFromUri ~uri:env.file.uri with
| None -> []
| Some full ->
maybeLog
("Finding references for (global) " ^ Uri.toString env.file.uri
^ " and stamp " ^ string_of_int stamp ^ " and tip "
^ Tip.toString tip);
forLocalStamp ~full stamp tip))))
| Some full ->
maybeLog
("Finding references for (global) " ^ Uri.toString env.file.uri
^ " and stamp " ^ string_of_int stamp ^ " and tip "
^ Tip.toString tip);
forLocalStamp ~full stamp tip)))
2 changes: 1 addition & 1 deletion analysis/src/ResolvePath.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ let resolveFromCompilerPath ~env ~package path =
| NotFound -> NotFound
| Exported (env, name) -> Exported (env, name)

let rec getSourceUri ~(env : QueryEnv.t) ~package path =
let rec getSourceUri ~(env : QueryEnv.t) ~package (path : ModulePath.t) =
match path with
| File (uri, _moduleName) -> uri
| NotVisible -> env.file.uri
Expand Down
Loading