Complete functions from included module #7515
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
|
Hmm, are |
|
Nope, cmt wise they are there. I checked that with the viewer. |
|
@zth, did some more digging, this really is a scope issue. |
Nice! Yeah something like |
analysis/src/CompletionBackEnd.ml
Outdated
| | `Ok (ident, path) -> ident.name :: path |> String.concat "." | ||
| in | ||
|
|
||
| if String.ends_with ~suffix:includePath source_module_path then |
There was a problem hiding this comment.
I'm not sure how we could entirely match this.
When the scope is created in the CompletionFrontEnd we doesn't know the exact that.
|
@nojaf coming up soon! |
|
I think this looks good, but @cristianoc I think we need your eyes here as well, to check the approach. |
| @@ -1,6 +1,20 @@ | |||
| open SharedTypes | |||
|
|
|||
| let modulePathFromEnv env = env.QueryEnv.file.moduleName :: List.rev env.pathRev | |||
| let modulePathFromEnv env = | |||
There was a problem hiding this comment.
This is an orthogonal change just for namespaces right?
There was a problem hiding this comment.
Yes, exactly, when testing this on my local project (which has a namespace), I needed this.
| // ^com | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
One distinction between open and include is that only include supports the following:
module M = {
let x = 3
}
module N = { include M }
let q = N.xI think it already works (on functors), completing a module N from the outside, so it should just be a matter of adding to the tests.
There was a problem hiding this comment.
Can that be something for a follow up PR? It is outside of my scope, and thus matters less to me. The PR right now would be a huge win to have.
There was a problem hiding this comment.
I think it already works (on functors), just needs to extend the test a bit.
analysis/src/CompletionBackEnd.ml
Outdated
| |> Hashtbl.iter (fun (name, _) (declared : Types.type_expr Declared.t) -> | ||
| (* We check all the values if their origin is the same as the include path. *) | ||
| match declared.modulePath with | ||
| | ModulePath.ExportedModule {name = exportedName} |
There was a problem hiding this comment.
@cristianoc, this part was a bit weird.
When I have only:
module M = {
let lex = (a: int) => "foo"
}
module N = {
include M
let a = 4
// let o = a.l
// ^com
// let _ = l
// ^com
}
in the test file, it it not needed.
Having the entire tests/analysis_tests/tests/src/IncludeModuleCompletion.res made the match ModulePath.ExportedModule case necessary.
I can't explain this.
There was a problem hiding this comment.
It's indeed almost unexplainable.
Turns out that when the value table is populated, each value declared in env is processed, and the pair (name, position) updated in the table.
Included values exist alongside the original values. So there are two values lex: one from M with module path ExportedModule, and one from include M with module path IncludedModule.
The last second one processed overwrites the first one.
And the order seems kind of arbitrary and changes with small changes to the source file.
Interestingly, when including values from other files, since we use the pair (name, position), there could be accidental clash in the position, which is just a pair of numbers.
There was a problem hiding this comment.
@nojaf if included values go to a separate table in LocalTables.ml, is it going to work? Or is there a need for them to be in the value table?
There was a problem hiding this comment.
Wow, that is an interesting find. Would have never figured that one out, thanks!
I've added a check not to override under very specific conditions.
@cristianoc is this the way to go here?
There was a problem hiding this comment.
In fact, they could just be a list, as the location and name are never used, except the name for filtering at the end?
There was a problem hiding this comment.
A separate table could work, so a new one based on ModulePath.IncludedModule _?
There was a problem hiding this comment.
@cristianoc a separate table works fine.
|
Thanks a lot for your help @cristianoc ! |
Thank you! |
I noticed that when using
include Foo({}), you don't receive completions for anything taken fromFoo. This PR addresses that issue.@zth I'm also adding the
cmt viewersince I needed to verify whether the CMT contained all the necessary information. I believe it's great to have this in the master branch already.