Fix cref's IntelliSense ordering for generic types (#8623)

[dimalypai]

Comparing on symbol text resulted in the opposite order, because of the ordering between > or } and , (comma). More discussion was happening in the issue #8623.

[dnfclas]

Hi @dmytrolypai, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[davkean]

tag @dotnet/roslyn-ide
@dotnet-bot test eta please

[davkean]

tag @rchande

[dnfclas]

@dmytrolypai, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[davkean]

@dotnet/roslyn-ide Can we please take a look at this?

[DustinCampbell]

Add <WorkItem(8623, "https://github.com/dotnet/roslyn/issues/8623")> attribute so we know what issue this tests.

[dimalypai]

Ah, sorry. For some reason I thought it was only for internal issues. Fixed now.

[tamlin-mike]

If the goal is for versions with more template arg's to come later, couldn't it simply be solved by letting sortText be symbol (base) name + " " + count_of_commas_plus_1 (or simply template args count, since that probably must be available somewhere as an O(1) operation) in a 2-digit form with leading zero?
(if the language supports more than 99 template arguments, make it three digits :-) ).

[dimalypai]

@tamlin-mike I think this sounds similar to one of the ideas in the issue discussion (#8623), please correct me if I am wrong. See comments by @rchande and @davkean regarding the usage of metadata type names.

[davkean]

@dotnet/roslyn-ide Can we take another look?

[tamlin-mike]

@dmytrolypai I don't see see it. davkean noted that "10" would come before "2" when sorted, and would therefore not work for template argument count. My idea was using "%02d", nTemplateArgs (C format syntax), which seems to solve that problem.

[dimalypai]

@tamlin-mike Aha, right, now I understand. Yes, 10 vs 2 problem is gone. The only downside I see is what you pointed out earlier about the number of template arguments and picking the correct width for the number part (I don't know if there is a limit for the number of type parameters in C#). Will you implement this idea? The test part from this pull requests can be used, I guess. I don't how to do it the best way with git/github magic, though :)

[tamlin-mike]

@dmytrolypai Afraid not. This was more of a drive-by problem solving.

[dimalypai]

@tamlin-mike I see)
I would like to hear an opinion from @dotnet/roslyn-ide about the solution idea described by @tamlin-mike. I found this also: http://stackoverflow.com/questions/2524293/limit-on-amount-of-generic-parameters-in-net (It looks like the spec does not restrict the number of type parameters, which becomes a bit of a problem here).

[rchande]

I wonder what the rest of @dotnet/roslyn-ide thinks of this issue I mentioned on the bug:

We also can't use "whitespace and identifier character strings with type parameter names" either:

genericType ztypeparameter would be sorted after generictype atypeparameter btypeparameter.

If we can't use the metadata name, can we take the type name and append the number of type parameters with some leading 0 padding?

[sharwell]

💭 Seems like the sort text should use the metadata name (with arity, e.g. Dictionary`2) and just ignore all the type parameters. Not sure if that would break other scenarios though.

[Pilchie]

I'm actually okay with this the way it is.

It should order things by number of parameters if they all use the same type parameter names, and lexically if they don't, which seems right to me.

👍

[davkean]

@Pilchie Won't that order Func after Func<T1, T2, TResult>?

[Pilchie]

@davkean Can you try that again with some escaping?

[davkean]

I meant Func<TResult> after Func<T1, T2, TResult>?

[tamlin-mike]

Can't the sort order hold its own info, separate from the display strings?

I mean, if we have
struct/class { string GenericName; /* NOTE: Intentionally not TYPE name */
int nGenericArgs ];

it would be obvious how to sort them with a predicate.

I'm starting to think I'm either missing something obvious, or we're all just fumbling in the dark. :-)

[rchande]

@dmytrolypai This slipped off our radar and I'm very sorry about that. Are you still interested in getting this merged? If so, can you resolve the conflicts and push a new set of changes? Thanks!

[davkean]

Think Dictionary`2 vs Dictionary`10. From: Sam Harwell<mailto:notifications@github.com> Sent: Saturday, September 9, 2017 1:31 AM To: dotnet/roslyn<mailto:roslyn@noreply.github.com> Cc: David Kean<mailto:David.Kean@microsoft.com>; Mention<mailto:mention@noreply.github.com> Subject: Re: [dotnet/roslyn] Fix cref's IntelliSense ordering for generic types (#8623) (#9356) @sharwell commented on this pull request.

________________________________ In src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Froslyn%2Fpull%2F9356%23discussion_r137820280&data=02%7C01%7CDavid.Kean%40microsoft.com%7C89fe4079b3b7456d28c108d4f6ceb5d1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636404815048926025&sdata=Dt2hLa6MBlWnJKmoRQYgUEQxb7dN32DdianOQVehsNc%3D&reserved=0>:

@@ -291,6 +291,13 @@ private static TextSpan GetTextChangeSpan(SourceText text, int position)

.Replace('>', '}') .ToString(); + // Sorting on symbolText or insertionText would have an annoying result for types like Action or Tuple. + // See issue: #8623 + var sortText = builder + .Replace('{', ' ') + .Replace('}', ' ') + .ToString(); 💭 Seems like the sort text should use the metadata name (with arity, e.g. Dictionary`2) and just ignore all the type parameters. Not sure if that would break other scenarios though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Froslyn%2Fpull%2F9356%23discussion_r137820280&data=02%7C01%7CDavid.Kean%40microsoft.com%7C89fe4079b3b7456d28c108d4f6ceb5d1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636404815048926025&sdata=Dt2hLa6MBlWnJKmoRQYgUEQxb7dN32DdianOQVehsNc%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABDYIkFV9e2Jy_lCWg5GuKIk-_05odSeks5sgV3egaJpZM4HmyID&data=02%7C01%7CDavid.Kean%40microsoft.com%7C89fe4079b3b7456d28c108d4f6ceb5d1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636404815048926025&sdata=UTlf0kZ5q0en2gQBAe%2Bpg3skVSauA5FG7l3%2BGrekxMU%3D&reserved=0>.

[dimalypai]

@rchande No worries! Yeah, it's been a while, but I will try to get this back on track.

[sharwell]

Think Dictionary`2 vs Dictionary`10.

That's easy enough to handle with a logical comparer for the sort, e.g. dotnet/corefx#395.

[CyrusNajmabadi]

We do have LogicalStringComparer at the VS layer. It could be properly lower down for this purpose.

[jaredpar]

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!