Roslyn: Signature help for delegate types

[jnm2]

If you're looking at this, ready to type a lambda, how do you know what to type next?

If it's Func<...> or Action<...> it's easy. For two or three others, I have the signature memorized and can translate the generic types in my head to the appropriate locations (e.g. Predicate<X>). For 99% of delegate types, it's a bunch of F12ing or F1ing every time.

Could signature help show something like this?

ParseArgument<SemanticVersion> is (ArgumentResult result) => SemanticVersion

@CyrusNajmabadi mentioned that something similar is already shown for anonymous types:

🔗 Also reported at AB#1244148

[jmarolf]

Design Review conclusions:

• Will take.
• No exclusions (like Func).  Do all delegates.

 

[jnm2]

There is currently the title-cased header Anonymous Types:. I like @CyrusNajmabadi's idea of Anonymous and delegate types: for when both show up, but I really dislike the resulting title-cased Anonymous and Delegate Types:. Any chance these headers could all be sentence-cased?

I'm pretty allergic to title casing and Microsoft guidance is consistent on this too:
https://docs.microsoft.com/en-us/style-guide/scannable-content/headings#formatting-headings
https://docs.microsoft.com/en-us/style-guide/developer-content/formatting-developer-text-elements
https://docs.microsoft.com/en-us/windows/win32/uxguide/text-ui#capitalization

[CyrusNajmabadi]

Feel free to just call it Complex types:

[jnm2]

The other headings should match the sentence casing then.

What does 'complex' explain? Could we get away with no header?

[jnm2]

Do you think the 'Anonymous types' header helps explain what the non-C# 'a syntax is meant to indicate?

[jnm2]

I have a proof of concept for delegate sighelp and a bunch of impl questions at this point, the main one being about specializing for C#/VB while there are signatures I can't touch because of InternalsVisibleTo. Should I ask those questions here or in discord/gitter or start a very early draft PR? Here's the draft: https://github.com/dotnet/roslyn/compare/master...jnm2:delegate_sighelp

Before:

After:

Existing UI for anonymous types:

---

This one sells it even better:

[jnm2]

The => doesn't looks as pretty in a non-monospace font. I liked it because almost all the time I'll want to start with a lambda and it's very easy to grasp.

Not sure ⇒ is better (maybe 🡒?):

Putting the return type before the parens is visually slower to parse, but it has a certain consistency:

Is this a way to make it work better? Hmm:

[davidwengier]

Putting the return type first would be consistent with what @jasonmalinowski is doing for function pointer signature help: https://github.com/dotnet/roslyn/pull/46074

I'm personally not a fan of the 3rd option above though, I like the X is Y pattern

[jnm2]

That's nice to be aware of, thanks! What do you think of this, similar to what you suggested there but without the *?

[davidwengier]

I think adding in delegate in this context is probably unnecessary and adds confusion, particularly if my suggestion in that PR doesn't get accepted. There is no reason to assume that the parameter value will be provided by the user as an anonymous delegate or a lambda, so standard method syntax (int (ArgumentResult result)) is the best compromise in my opinion.

[333fred]

Putting the return type first would be consistent with what @jasonmalinowski is doing for function pointer signature help: #46074

fwiw, don't like how that looks. I don't hate having the full delegate signature, but what happens with nested delegate types? Or nested function pointer types, for that case? It starts to feel like C function pointers, which is absolutely not a complement. I much prefer the => version, and I don't even think the => in a non-monospace font looks bad either.

[Wraith2]

I'd like something to show the end of the text and beginning of the signature. thing is int thingy needs a mental backtrack for me because it's not csharp syntax, it'd be nice to get thing is: int thingy or something along those lines.

[huoyaoyuan]

Upvote for the => version, because it's consistent with the way you will write the lambda.

[jnm2]

but what happens with nested delegate types?

Like this?

[CyrusNajmabadi]

The => doesn't looks as pretty in a non-monospace font. I liked it because almost all the time I'll want to start with a lambda and it's very easy to grasp.

I'm fine with it. I'm also fine with ⇒. I'm not fine with 🡒. I'm not fine with havin this have local-function syntax. Def keep with lambda syntax (also matches TypeScript).

[CyrusNajmabadi]

Is this a way to make it work better? Hmm:

I don't mind showing it in delegate form. but i would actually then write it out as: delegate int ParseArgument<int>(ArgumentResult result).

[CyrusNajmabadi]

I much prefer the => version, and I don't even think the => in a non-monospace font looks bad either.

This applies to me as well. Keep with arrow, monospaced, or ligature form doesn't matter to me. My own code uses ligatures, so i'm used to seeing this:

[CyrusNajmabadi]

thing is int thingy needs a mental backtrack for me because it's not csharp syntax, it'd be nice to get thing is: int thingy or something along those lines.

I wouldn't mind seeing what it looks like like:

Func<a', string>: (a' arg) => string
a': new { string Name }

[jasonmalinowski]

Putting the return type first would be consistent with what @jasonmalinowski is doing for function pointer signature help:

Note that as a far as function pointer signature help, I'm following the existing pattern for _any_ method call which is the return type is first. I don't think I'd say my PR creates any precedent for this feature here.

Do we need to do nested stuff? If the goal here is to let people figure out how to write the lambda, and nested delegates in arguments isn't necessary. In this example above from @jnm2:

If I'm writing a lambda the only thing I need to know is:

1. There's one parameter List<Func<int, string, byte>> but I'm not going to type that in the lambda at all. The actual parameters of the Func<int, string, byte> doesn't matter at all unless I'm say calling list.Add in my lambda (at which point this feature would now trigger when showing the Add, the Func<int, string, byte> is the top level delegate type in that signature help) or I'm invoking a member I got from that list, at which point our existing signature help will help you. Basically, you'll get your help for the nested delegates _when you actually need it_, and there's no reason to also show it here.
2. It returns an Action, so I probably should make sure I end up with one in my body.

[jnm2]

Ah, so it would be best to exclude nested delegate types. That makes a lot of sense. Does that include delegates that are nested in other types that are not delegates, like List<Action<int>> or (Func<int> A, Action<int> B)?

I think it should be fine to wait to show the delegate type until you get here (should be showing here but isn't):

[jnm2]

Would it ever even make sense to show any delegate signature other than the currently highlighted one?

[CyrusNajmabadi]

Idon't mind the recursive nature of this space. in general, you won't have more than a tiny handful of types being shown. So i really don't think we'd need to limit things here.

[jnm2]

@CyrusNajmabadi I thought of something and now I really want it to be different. At the top, I want to see this for the highlighted param only:

Delegate signature:
(System.CommandLine.Parsing.ArgumentResult result) => int

I don't want anything above it, whether delegates or anonymous types. This needs to be very quick to visually seek to because I'm about to type a lambda and the only thing I want to know is the one signature that the lambda will have to match. I don't want there to be even a tiny chance that this is not standing out at the top.

Is this an acceptable starting point, and then we can consider detecting and listing all delegate types if someone asks?

[Wraith2]

Could it eventually be made into a fixer so you can just tab in the args and arrow parts? I'm thinking you type ( then =>\t and get ( (a, b) =>

[CyrusNajmabadi]

This needs to be very quick to visually seek to because I'm about to type a lambda and the only thing I want to know is the one signature that the lambda will have to match

Won't that be the common case? i.e. 99% of the time you won't have an anonymous type (or another delegate). So in nearly all cases, you'll jsut get the single signature you care about. In teh 1% case, there may be a little more info. But that seems fine.

[CyrusNajmabadi]

@Wraith2 Sounds like you're proposing some new feature entirely. This is just about quick-info.

[jnm2]

@CyrusNajmabadi Much less common than that. Anonymous types are listed for all visible parameters of the method, not just the current one. It's common for me to be calling a constructor with a number of delegate parameters to represent UI state. I want the signature for the current parameter I'm on to stand out, just like with the XML docs under it. Showing all delegates in the method signature would be bad unless they were separate visually from the one that shows me the param types, names, and order and return type for the lambda I'm about to type.

Making me look through even two items to compare the listed delegate type with the current parameter is making me jump through hoops I don't like. And if you only ever list the current delegate signature, you cut down on repetition which is needless in the 90% case or whatever the number really is.

(string alias, System.CommandLine.Parsing.ParseArgument parseArgument, [bool isDefault = false], ...)

Delegate signature:
(System.CommandLine.Parsing.ArgumentResult result) => int

Versus:

(string alias, System.CommandLine.Parsing.ParseArgument parseArgument, [bool isDefault = false], ...)

Complex types:
System.CommandLine.Parsing.ParseArgument is (System.CommandLine.Parsing.ArgumentResult result) => int

[CyrusNajmabadi]

How often will tehre even be more than a single delegate? And even 3+ seems liek it would almost never happen.

[jnm2]

@CyrusNajmabadi Like I said, it's common for me to be calling a constructor that has a number of delegate parameters on a class that represents UI state. I'd rather that every parameter in the signature wasn't picked up and sorted in somehow regardless of which one I'm actually on. If it's a really rare case, though, I think we should prefer the version that has less unnecessary repetition for the single case. (See examples above.)

[jnm2]

I would like to go ahead with a separate section above all other sections that appears on delegate-typed parameters and looks like:

Delegate signature:
(System.CommandLine.Parsing.ArgumentResult result) => int

I'm looking for a green light from the team or further engagement on the design. My reasoning is above for why I think it's presented in a superior way compared to both the single delegate and multiple delegate cases. I'm willing to go in a different direction but I'd like the concerns I brought up to be answered then.

If it helps, I can build side-by-side screenshots of the approaches using real-world projects.

[CyrusNajmabadi]

@jnm2 Consider this a green light. :)

[jnm2]

Because https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.language.intellisense.iparameter has no Content property like https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.language.intellisense.isignature has, the content for the currently-selected parameter will have to be placed in the signature content and updated as the current parameter changes. This seems fine because Microsoft.VisualStudio.Language.Intellisense.Implementation.SignatureHelpSessionView.DisplayContent seems to be called every time the current parameter changes.

[jnm2]

Oh, hey. This is already managed for me by Roslyn. Nice.
https://github.com/dotnet/roslyn/blob/f8107de2a94a01e96ac3d7c1f225acbb61e18830/src/EditorFeatures/Core/Implementation/IntelliSense/SignatureHelp/Presentation/Signature.cs#L175-L181

[jnm2]

Can you all live with this?
Too bad there's no way to represent bold display parts in SignatureHelpSymbolParameter.SelectedDisplayParts.

[CyrusNajmabadi]

I think i'd prefer something simpler like:

Delegate types:
  ParseArgument<int> is (ArgumentResult result) => int

(or Complex types: or Type structure:)

[jnm2]

Got 6 votes between these two:

(https://twitter.com/jnm236/status/1333583270643163136)

And a suggestion that we should print out the delegate type declaration instead of either option.

The singular nature of this is the thing I care about most. You shouldn't be scanning among multiple because I think the ones you're skipping are just noise if you're choosing what to type in this position. Multiple was what I started with, and there was a tangible difference in cognitive load for me.