Update F1 Keywords to differentiate between semantics of default keyword #48500

TheSench · 2020-10-11T11:50:05Z

This PR adds two separate f1_keywords for different semantics of the default keyword. This allows the F1 help to route to the appropriate page based on the context, rather than having to route to a generic default page.

The switch-statement version of default is detected by checking if the token is part of a DefaultSwitchLabelSyntax.

The default-value-expression version of default is detected by checking if the token is part of:

a DefaultExpressionSyntax (default operator)
a LiteralExpressionSyntax (default literal expression)

Fixes part of #48392, dependent on dotnet/docs#20799

Related docs PR:
dotnet/docs#21034

Youssef1313 · 2020-10-11T12:05:12Z

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

@@ -352,6 +352,20 @@ private static bool TryGetTextForKeyword(SyntaxToken token, ISyntaxFactsService
                }
            }

+            if (token.IsKind(SyntaxKind.DefaultKeyword)) {


Suggested change

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

if (token.IsKind(SyntaxKind.DefaultKeyword))

{

Youssef1313 · 2020-10-11T12:06:58Z

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

+                    return true;
+                }
+
+                if (token.Parent is DefaultExpressionSyntax || token.Parent is LiteralExpressionSyntax)


Suggested change

if (token.Parent is DefaultExpressionSyntax || token.Parent is LiteralExpressionSyntax)

if (token.Parent is DefaultExpressionSyntax or LiteralExpressionSyntax)

I believe or is only valid in VB, isn't it?

It's a new C# 9 feature. See the Pattern Combinators section in dotnet/csharplang#2850

Ahh, I need to get up to speed on C# 9. Thanks for pointing that out.

davidwengier · 2020-10-11T21:21:36Z

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

+                if (token.Parent is DefaultExpressionSyntax or LiteralExpressionSyntax)
+                {
+                    text = Keyword("defaultvalue");
+                    return true;
+                }


Is this check adding any value? ~~Why not just let this fall through and use default_CSharpKeyword as it currently does? Seems to me like switch labels are the special case here.~~

Thinking about it more, I'd say just remove the if, set the keyword to defaultvalue and return. I'd probably also put a comment here about why this isn't falling through to default_CSharpKeyword (for older VS versions).

Is this check adding any value? Why not just let this fall through and use default_CSharpKeyword as it currently does? Seems to me like switch labels are the special case here.

This is pending a decision on what happens in the docs PR of course.

Falling back to default_CSharpKeyword will lead to maintaining less f1_keywords in docs. So, +1 for that.

This is also what we have done for !, we only introduced one new f1_keyword, and used the existing one as-is.

I like the idea of falling back to the old default_CSharpKeyword, especially now that it no longer is used for the "choose which context" default page.

Going this route would require moving the default_CSharpKeyword from the /keywords/default.md page to the /operators/default.md page.

The main reason I originally left it in was in case any future usage of default appeared, as it would at least still route to the keyword page. For now though, I'm opting to reuse the existing default_CSharpKeyword and move it to the appropriate page. Does this present any backwards compatibility issues @Youssef1313?

The main reason I originally left it in was in case any future usage of default appeared, as it would at least still route to the keyword page. For now though, I'm opting to reuse the existing default_CSharpKeyword and move it to the appropriate page. Does this present any backwards compatibility issues @Youssef1313?

Changes in F1 area in Roslyn will not introduce any backcompat issues.
Changes in docs may introduce backcompat issues if and only if existing f1_keywords are completely deleted.

If a a future usage of default appear, that's not a problem, we can simply update the f1 service and introduce a new f1_keyword for it.

ghost · 2020-10-13T00:55:57Z

Hello @davidwengier!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

davidwengier · 2020-10-13T00:56:15Z

Thanks for the contribution!

…features/interpolated-string-constants * upstream/master: (295 commits) Update F1 Keywords to differentiate between semantics of default keyword (#48500) Default constructor suggestion between members (#48318) (#48503) Adjust ERR_PartialMisplaced diagnostic message (#48524) Refactor ChangedText.Merge and add fuzz testing (#48420) Apply suggestions from code review Do not crash on unexpected exception (#48367) Reference the contributing doc in 'Analyzer Suggestion' issue template Apply suggestions from code review Hardcode skipped Regex diagnostic ID as it is not available in CodeStyle layer Add using Skip help link for Regex diagnostic analyzer Add contributing doc for IDE code style analyzer documentation Make db lock static to investigate issue. Update dependencies from https://github.com/dotnet/roslyn build 20201012.2 (#48513) Hook up help link even for AbstractCodeQualityDiagnosticAnalyzer Add destructor intellisense test for record (#48297) Remove unused method (#48429) Fix bug Update src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs Add more test ...

* upstream/master: (68 commits) Update F1 Keywords to differentiate between semantics of default keyword (dotnet#48500) Default constructor suggestion between members (dotnet#48318) (dotnet#48503) Adjust ERR_PartialMisplaced diagnostic message (dotnet#48524) Refactor ChangedText.Merge and add fuzz testing (dotnet#48420) Apply suggestions from code review Do not crash on unexpected exception (dotnet#48367) Reference the contributing doc in 'Analyzer Suggestion' issue template Apply suggestions from code review Hardcode skipped Regex diagnostic ID as it is not available in CodeStyle layer Add using Skip help link for Regex diagnostic analyzer Add contributing doc for IDE code style analyzer documentation Make db lock static to investigate issue. Update dependencies from https://github.com/dotnet/roslyn build 20201012.2 (dotnet#48513) Hook up help link even for AbstractCodeQualityDiagnosticAnalyzer Add destructor intellisense test for record (dotnet#48297) Remove unused method (dotnet#48429) Fix bug Update src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs Add more test ...

TheSench requested a review from a team as a code owner October 11, 2020 11:50

Youssef1313 reviewed Oct 11, 2020

View reviewed changes

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

davidwengier reviewed Oct 11, 2020

View reviewed changes

TheSench mentioned this pull request Oct 12, 2020

Add F1 keywords for default token semantics dotnet/docs#21034

Merged

CyrusNajmabadi approved these changes Oct 12, 2020

View reviewed changes

TheSench added 9 commits October 11, 2020 23:59

Add test cases

cbdee66

Add F1_help test cases for default keyword

93e5d20

Identify default switch case

e743fa7

Identify default expression

748ba17

Identify default literal expression

5eba082

Proper C# curly placement

0b0afdc

Simplify with Pattern Combinator

e00dd92

Simplify matching of default cases

e6c49c5

Simply let normal Keyword case fall through

245c6f8

TheSench force-pushed the feature/default-f1-help branch from 622047f to 245c6f8 Compare October 12, 2020 03:59

jinujoseph assigned davidwengier Oct 12, 2020

TheSench added 2 commits October 12, 2020 12:21

Fix test expectations

214b82d

Fix curly placement

eb2e257

davidwengier approved these changes Oct 13, 2020

View reviewed changes

davidwengier added the auto-merge label Oct 13, 2020

ghost approved these changes Oct 13, 2020

View reviewed changes

davidwengier removed the auto-merge label Oct 13, 2020

davidwengier merged commit 8746590 into dotnet:master Oct 13, 2020

ghost added this to the Next milestone Oct 13, 2020

TheSench deleted the feature/default-f1-help branch October 13, 2020 10:32

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update F1 Keywords to differentiate between semantics of default keyword #48500

Update F1 Keywords to differentiate between semantics of default keyword #48500

TheSench commented Oct 11, 2020 •

edited

Loading

Youssef1313 Oct 11, 2020

Youssef1313 Oct 11, 2020

TheSench Oct 11, 2020

MaStr11 Oct 11, 2020

TheSench Oct 11, 2020

davidwengier Oct 11, 2020 •

edited

Loading

Youssef1313 Oct 11, 2020 •

edited

Loading

TheSench Oct 12, 2020

TheSench Oct 12, 2020

TheSench Oct 12, 2020

Youssef1313 Oct 12, 2020

ghost commented Oct 13, 2020

ghost left a comment

davidwengier commented Oct 13, 2020

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

	if (token.Parent is DefaultExpressionSyntax \|\| token.Parent is LiteralExpressionSyntax)
	if (token.Parent is DefaultExpressionSyntax or LiteralExpressionSyntax)

Update F1 Keywords to differentiate between semantics of default keyword #48500

Update F1 Keywords to differentiate between semantics of default keyword #48500

Conversation

TheSench commented Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 13, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost left a comment

Choose a reason for hiding this comment

davidwengier commented Oct 13, 2020

TheSench commented Oct 11, 2020 •

edited

Loading

davidwengier Oct 11, 2020 •

edited

Loading

Youssef1313 Oct 11, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.