Update F1 keywords to disambiguate `class` by TheSench · Pull Request #48506 · dotnet/roslyn

TheSench · 2020-10-12T00:41:34Z

This PR makes the f1 keywords for the class keyword be context-specific. If it appears in a generic constraint clause, it will route to the where-generic-type-constraint page, otherwise it will continue to go to the class declaration page.

Fixes part of dotnet/docs#20799

Related docs PR:
dotnet/docs#21037

davidwengier · 2020-10-12T01:16:25Z

src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs

Suggested change

if (token.IsKind(SyntaxKind.ClassKeyword)) {

if (token.Parent is ClassOrStructConstraintSyntax) {

text = Keyword("classconstraint");

return true;

} else {

text = Keyword("class");

return true;

}

}

if (token.IsKind(SyntaxKind.ClassKeyword) && token.Parent is ClassOrStructConstraintSyntax)

{

text = Keyword("classconstraint");

return true;

}

CyrusNajmabadi · 2020-10-12T02:46:17Z

src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs

can we do the same for struct/new as those are also constraints.

@TheSench feel free to include them in this PR, or not. They are called out in dotnet/docs#20799 so we're not going to forget.

They would go to the same file as your current docs PR is changing, so there might be some sense in at least doing that side of things.

Seeing as the build failed due to what appears to be a transient issue, I'll add this and kick off another one. Is there a benefit to creating a new f1_keyword versus just using the existing whereconstraint_CSharpKeyword one for the constraint form of both of these?

I'm wondering if we want to introduce a switch on token.RawKind here now that the number of special cases has grown a bunch. I'll probably be tackling more of these cases in the next week, so that would be something I consider in the next one, but I'd be interested in direction before I get going on it, if possible.

this code is C# specific. So you don't need RawKind (you can use Kind()). I wouldn't really do a switch here. I think the code is clear when it handles each case specifically. There's no need for brevity/perf as this will execute once only on hte rare case of someone hitting f1.

Thanks for the extra context.

TheSench · 2020-10-12T15:48:12Z

It looks like I missed some styling issues that got caught in the build. I'll look at those later tonight and get them cleaned up.

davidwengier · 2020-10-13T00:53:47Z

Thanks for the contribution. I'll wait for dotnet/docs#21037 to be merged before merging this, feel free to ping me here if it happens and I miss the notification.

TheSench · 2020-10-13T17:04:17Z

I'm going to need some help figuring out these build failures. I don't see anything helpful in the logs, aside from the possibility that the tests are timing out:

Running 2 test assemblies in 2 partitions
   1 running,  1 queued,  0 completed
Roslyn Error: test timeout exceeded, dumping remaining processes
Copying ServiceHub logs to F:\workspace\_work\1\s\artifacts\log\Release
##[error](NETCORE_ENGINEERING_TELEMETRY=Test) Tests failed
Command failed to execute with exit code 1: F:\workspace\_work\1\s\artifacts\bin\RunTests\Release\net472\RunTests.exe "C:\Users\vsagent\.nuget\packages\xunit.runner.console\2.4.1-pre.build.4059\tools\net472" "-out:F:\workspace\_work\1\s\artifacts\TestResults\Release" "-logs:F:\workspace\_work\1\s\artifacts\log\Release" "-secondaryLogs:F:\workspace\_work\1\s\artifacts\log2\Release" -tfm:net472 -testVsi -xml -timeout:110 -procdumppath:F:\workspace\_work\1\s\.tools\ProcDump F:\workspace\_work\1\s\artifacts\bin\Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests\Release\net472\Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests.dll F:\workspace\_work\1\s\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\Release\net472\Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll
System.Management.Automation.RuntimeException: Command failed to execute with exit code 1: F:\workspace\_work\1\s\artifacts\bin\RunTests\Release\net472\RunTests.exe "C:\Users\vsagent\.nuget\packages\xunit.runner.console\2.4.1-pre.build.4059\tools\net472" "-out:F:\workspace\_work\1\s\artifacts\TestResults\Release" "-logs:F:\workspace\_work\1\s\artifacts\log\Release" "-secondaryLogs:F:\workspace\_work\1\s\artifacts\log2\Release" -tfm:net472 -testVsi -xml -timeout:110 -procdumppath:F:\workspace\_work\1\s\.tools\ProcDump F:\workspace\_work\1\s\artifacts\bin\Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests\Release\net472\Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests.dll F:\workspace\_work\1\s\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\Release\net472\Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll
at Exec-CommandCore, F:\workspace\_work\1\s\eng\build-utils.ps1: line 95
at Exec-Console, F:\workspace\_work\1\s\eng\build-utils.ps1: line 164
at TestUsingOptimizedRunner, F:\workspace\_work\1\s\eng\build.ps1: line 421
at <ScriptBlock>, F:\workspace\_work\1\s\eng\build.ps1: line 648
at <ScriptBlock>, F:\workspace\_work\_temp\e207cc64-eae2-44fb-a0b9-2d26d42ca06f.ps1: line 3
at <ScriptBlock>, <No file>: line 1
Killing running build processes...
Killing running build processes...
##[error]PowerShell exited with code '1'.

davidwengier · 2020-10-13T20:32:44Z

I'm sure its unrelated. I've kicked the integration tests again and will monitor.

Thanks for adding in the extra struct case too :)

TheSench · 2020-10-13T22:23:34Z

I'm sure its unrelated. I've kicked the integration tests again and will monitor.

Thanks for adding in the extra struct case too :)

Build passed this time!

CyrusNajmabadi · 2020-10-13T22:49:36Z

Thanks!

TheSench requested a review from a team as a code owner October 12, 2020 00:41

TheSench changed the title ~~Feature/disambiguate f1 help for class keyword~~ Update F1 keywords to disambiguate class Oct 12, 2020

TheSench mentioned this pull request Oct 12, 2020

Add f1-keyword for class used as generic type constraint dotnet/docs#21037

Merged

davidwengier reviewed Oct 12, 2020

View reviewed changes

CyrusNajmabadi reviewed Oct 12, 2020

View reviewed changes

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 12, 2020

jinujoseph assigned davidwengier Oct 12, 2020

davidwengier approved these changes Oct 13, 2020

View reviewed changes

TheSench added 8 commits October 13, 2020 06:42

Add tests for class keyword

6dda15e

Check class semantics

75b7f72

Simplify checking for class

d2ac73b

Use Keyword method to make maintenance of suffixes easier

c2166f3

Let normal class usage fall through

6b60abc

Fix curly placement

99eb512

Add tests for disambiguating struct

7c7845b

Disambiguate struct

becfcf8

TheSench force-pushed the feature/disambiguate-f1-help-for-class-keyword branch from 6beaafd to becfcf8 Compare October 13, 2020 11:05

CyrusNajmabadi merged commit 021b82f into dotnet:master Oct 13, 2020

ghost added this to the Next milestone Oct 13, 2020

TheSench deleted the feature/disambiguate-f1-help-for-class-keyword branch October 14, 2020 11:33

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

mikadumont mentioned this pull request Dec 10, 2020

Thank You To Everyone Who Contributed To Roslyn In 16.9 Preview 2 #49892

Closed

-            if (token.IsKind(SyntaxKind.ClassKeyword)) {
-                if (token.Parent is ClassOrStructConstraintSyntax) {
-                    text = Keyword("classconstraint");
-                    return true;
-                } else {
-                    text = Keyword("class");
-                    return true;
-                }
-            }
+            if (token.IsKind(SyntaxKind.ClassKeyword) && token.Parent is ClassOrStructConstraintSyntax)
+            {
+                text = Keyword("classconstraint");
+                return true;
+            }

Conversation

TheSench commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidwengier Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

davidwengier Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

TheSench Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

TheSench Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

TheSench Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

TheSench commented Oct 12, 2020

Uh oh!

davidwengier commented Oct 13, 2020

Uh oh!

TheSench commented Oct 13, 2020

Uh oh!

davidwengier commented Oct 13, 2020

Uh oh!

TheSench commented Oct 13, 2020

Uh oh!

CyrusNajmabadi commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TheSench commented Oct 12, 2020 •

edited

Loading

davidwengier Oct 12, 2020 •

edited

Loading