Skip to content

TypeApp, type parameter completions #12933

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 6 commits into from
Apr 29, 2022
Merged

TypeApp, type parameter completions #12933

merged 6 commits into from
Apr 29, 2022

Conversation

kerams
Copy link
Contributor

@kerams kerams commented Apr 3, 2022

The first part of this PR deals with completions inside of a type application (SynExpr.TypeApp). Given

let arr = Array.empty<str>

pressing Ctrl-space on str will now only show type-like completions. Currently the list also contains keywords and functions.

Secondly, type variables (parameters) in scope are now included in the list of completions. Given

let emptyMap<'keyType, 'valueType> () =
    Map.empty<'keyType, va>

type Option<'typ> =
    | None
    | Some of ty

pressing Ctrl-space on va or ty will show type-like completions, including valueType and typ, respectively. ' (or ^ for SRTPs) is added automatically when the completion is used. (I have a vague memory this was thing in the past, and that you'd often end up with 2 apostrophes. Maybe in Power Tools pre-Roslyn?)

79xHfj1Th0

Thirdly, type variables now have (currently not really useful) tooltips and are clickable (F12 navigation works already).

UEFrK2xna9

cZuvaGAQK3

Thoughts/open issues (for future PRs?)

  1. In C# tooltips for type parameters show where the latter is declared.
    devenv_AxBCfELVBp
    Typar doesn't seem to directly contain such information. Is there a way I can quickly find out the record/union/type definition or method/function as the point of declaration?

  2. Because denv.showConstraintTyparAnnotations is false, SRTP tooltips show up with an apostrophe instead of a caret, which looks a bit weird.
    auEj4REkgb

  3. I would like to see only type-like completions pop up in all of these cases

    let abc<'typ> = Array.empty<
    let abc<'typ> = Array.empty<'
    let abc<'typ> = Array.empty<'>
    let abc<'typ> = Array.empty<>
    let abc<'typ> = Array.empty<'t

    The parser detects an error and reports SynExpr.App, and the presence of a less than/inequality operator, so the TypeApp completion context cannot be immediately established. Unfortunately, because of type inference, we cannot really rule out the possiblity that the user wants to make a comparison, however unlikely it would be in these 5 functions.

  4. When type variables are in scope, I feel they should have a high priority on the completion list, because are likely to be used. When func<' > is typed, they should certainly be the only items on the list.

  5. typeparam XmlDoc does not appear to be collected on a function. We could at least show that in the tooltip.

Comment on lines 963 to 965
// Unchecked.defaultof<str$>
| SynExpr.TypeApp (typeArgsRange = range) when rangeContainsPos range pos ->
Some CompletionContext.TypeApp
Copy link
Contributor Author

@kerams kerams Apr 3, 2022

Choose a reason for hiding this comment

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

In light of points 3 and 4, I'd rather remove CompletionContext.TypeApp and abuse CompletionContext.PatternType. The reason is that we might like to have the TypeApp case carry further information in the future, and we'd be making a breaking change then.

|> Seq.map (fun kvp -> Item.TypeVar (kvp.Key, kvp.Value))
|> Seq.toList

unqualifiedItems @ activePatternItems @ moduleAndNamespaceItems @ tycons @ constructors @ typeVars
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure ResolvePartialLongIdentPrim is the place to insert these type variables as it's outside of the FCS.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure exactly what you mean by 'outside of FCS' here. This seems like a reasonable change, and sometimes lower level changes need to be made in order to surface better behaviors in the user-facing APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's outside of the service folder, so I thought this file was part of the compiler proper.

@kerams
Copy link
Contributor Author

kerams commented Apr 11, 2022

Let me know how you want to proceed. If the PR is acceptable as it is, I'll just update the surface area.

@vzarytovskii
Copy link
Member

Let me know how you want to proceed. If the PR is acceptable as it is, I'll just update the surface area.

Is it possible to add some tests for type-parameters completions? Just couple of simple ones?

Overall, I think, this is great, @KevinRansom @cartermp thoughs?

@kerams
Copy link
Contributor Author

kerams commented Apr 11, 2022

Good idea, that slipped my mind.

@kerams
Copy link
Contributor Author

kerams commented Apr 11, 2022

Two more points.

nenv.eTypars is empty when collecting completions in type A<'lType> = { Field: l }. This is in contrast with type A<'lType> = Case of l, where 'lType is present in the dictionary.

For let blah<'foo1> (x: 'foo2) = Unchecked.defaultof<foo> nenv.eTypars contains only 'foo1.

@kerams kerams marked this pull request as ready for review April 20, 2022 19:51
@vzarytovskii vzarytovskii merged commit 7abc88b into dotnet:main Apr 29, 2022
@vzarytovskii
Copy link
Member

Thanks!

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.

3 participants