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.

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

Complete pipeable functions from dot completion on record #1054

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

zth
Copy link
Collaborator

@zth zth commented Nov 5, 2024

Complete for pipeable functions from the parent module for a record when that record is the type t of the parent module.

Example:

module SomeModule = {
  type t = {name: string}

  @get external getName: t => string = "name"
}

let n = {SomeModule.name: "hello"}

// n.
//   ^com

This should complete for both name (record field) and getName (function from parent module that takes this record as the first argument).

Experimental @editor.completeFrom annotation

This PR also introduces an experimental @editor.completeFrom annotation, that lets you provide more modules the tooling can draw additional completions from, without that type needing to be defined as the type t of that module. This is useful because there are situations where you can't/don't want to define a type as type t inside of a module, but you still want to get completions from that module. Examples include recursive type chains, where the type definition might need to live outside of the module, and similar.

Annotating with @editor.completeFrom will make the editor tooling complete for pipes from the provided module too. Example:

// We point out `SomeOtherModule` as the module this type is the main type for
@editor.completeFrom(SomeOtherModule)
type typeOutsideModule = {nname: string}

module SomeOtherModule = {
  type t = typeOutsideModule
  @get external getNName: t => string = "nname"
  @get external getNName2: typeOutsideModule => string = "nname"
}

let nn = {nname: "hello"}

// nn.
//    ^com

This will complete for:

  • nname (record field)
  • getNName (pipe from module)
  • getNName2 (pipe from module)

The annotation has no other effect than providing a hint for the editor tooling.

Follow ups

  • The @editor.completeFrom annotation can be applied to the regular pipe completion mechanism as well in a separate PR.
  • We can set up completion for modules for the payload of @editor.completeFrom as well.
  • Right now it's enforced that the type we're trying to complete is a record. But, I guess we don't have to assume that, and dot-completion on something that's not a record, but still the main type of a module, could get pipe completions.

TODO ✅

This is an almost working PoC. A few things remain:

  • Set an explicit loc for the completion edit, so the existing dot in the text being completed is removed in favor of the ->. Currently the completion will end up being n.->SomeModule.getName where we obviously want n->SomeModule.getName
  • Don't enforce that the record must be called t (essentially - allow aliases to type t too)
  • Related to the point above - ponder on some sort of @mainModule directive so that we can trigger completions for types that aren't necessary a real type t for various reasons, but that de facto is the main type of a module

Comment on lines 16 to 29
"label": "->SomeModule.getName",
"kind": 12,
"tags": [],
"detail": "t => string",
"documentation": null,
"sortText": "getName",
"insertText": "->SomeModule.getName"
}, {
"label": "name",
"kind": 5,
"tags": [],
"detail": "string",
"documentation": {"kind": "markdown", "value": "```rescript\nname: string\n```\n\n```rescript\ntype t = {name: string}\n```"}
}]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the relevant test output.

@zth zth changed the title PoC complete pipeable functions from dot completion on record Complete pipeable functions from dot completion on record Nov 6, 2024
@zth zth marked this pull request as ready for review November 6, 2024 21:27
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Any chances this affects library design? Core?

Path n
Path SomeModule.
[{
"label": "->SomeModule.getName",
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious about the function coming before the field
also, any thoughts about making . a valid source language construct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you expand a bit?

Copy link
Collaborator

@cristianoc cristianoc Nov 7, 2024

Choose a reason for hiding this comment

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

1 If I write x. I see x->getName() before x.field, which can be surprising to people working with records. Just wondering.

2 Could x.getName() be actually part of the language, or will it always need to be expressed as x->getName? Autocompletion is right now suggestive of something that does not exist in the language. Not necessarily a bad thing, just thinking aloud about possible pros and cons here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Ahh right! That's a good point. It's currently sorted on the field name and function name. We could sort it whatever way we think works best. I'd say either this (sort it together with the regular fields) or put it at the bottom. I'd favor the current way, I think.
  2. Oh, that's.... a very interesting idea! That would solve quite a few of our issues. Let's explore that!

Copy link
Member

Choose a reason for hiding this comment

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

It seems that is about what called "uniform function call syntax"

And there was a similar discussion in ReasonML reasonml/reason#1638 early day

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the context. Will read through.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if we could figure out a good way to do that, pointing out the module to use with the call (someRecord.SomeModule.someFn()), but with a small enough twist that it's still more convenient than pipe, but not ambigious wrt the current behavior of SomeModule there being a hint.

Anyway, just completing the actual pipe, but via dot, is a good start for DX imo.

@zth
Copy link
Collaborator Author

zth commented Nov 7, 2024

Any chances this affects library design? Core?

I don't think so. It should mostly just simplify things, and I also think with this we could get rid of some the special casing for pipe completions we have, and just add the annotation to types shipped with the compiler.

@zth
Copy link
Collaborator Author

zth commented Nov 7, 2024

Ok, so this PR explicitly handles (resolvable) record types only, because this was originally intended to cater to a potential use case in https://github.com/rescript-lang/experimental-rescript-webapi. But, this could easily be useful in more general cases. Examples:

  • We could apply the same logic for pipe completions in general. Ie if you have a type where @mainTypeForModule is defined, then complete also from that module when doing pipe completion.
  • Given that whatever. is such an established concept in JS/TS, we really don't need to restrict this to records only. We could just generalize this pattern to work on any dot-completion, as long as the type that dot completion resolves to is a type t of a module, or annotated with a @mainTypeForModule

The latter part opens up some pretty good optimizations for people used to JS/TS:

"some string".
//            ^com

JS/TS users would expect the above to complete for length, since string.length is how you get the length of a string in JS. In ReScript, you'd do "some string"->String.length. Currently that's difficult to discover though unless you know about it, because you need to explicitly do pipe completion.

Expanding dot completion to also complete for relevant pipe functions here would make that much more discoverable. I would also think it would mean there's a good chance people will just learn the convention of "main type, pipe functions from module of that main type" automatically because they'll see what their expected dot completion is resolved to. This would be a massive win because that means that the tooling could help teach one of the larger differences between how things are done in ReScript vs JS.

// ^com
// ^dv-

@mainTypeForModule(SomeOtherModule)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this also work when SomeOtherModule is another file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's effectively just a path to a module, so it'll take that exact module path and try to complete it from whatever point you're completing. So it's simulating doing whateverToComplete-><pathFromMainTypeForModuleHere>.<com>.

type typeOutsideModule = {nname: string}

module SomeOtherModule = {
type t = typeOutsideModule
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this alias as t a hard requirement to make this work?
Being newish to ReScript type t is a little strange and it feels a bit like an artificial limitation right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants