Fix F1 keywords clashing for !_CSharpKeyword #47516
Conversation
Youssef1313
force-pushed
the
f1-fixes
branch
from
a559cba
to
3db401f
Compare
| { | ||
| text = token.Parent.IsKind(SyntaxKind.LogicalNotExpression) | ||
| ? Keyword("!") | ||
| : Keyword("nullForgiving"); |
There was a problem hiding this comment.
This feels a little like redoing some of the logic the parser is doing for us already, and will fall down if exclamation points are used for something else in future. For example there is a proposal for argument validation that would use it.
To me it makes more sense to simply check if the parent is SuppressNullableWarningExpression and return Keyword("nullForgiving") for that, and let the rest of the code flow as it did before.
There was a problem hiding this comment.
@davidwengier I did that in the latest commit but just realized a problem with that approach.
For null forgiving operator, SuppressNullableWarningExpression won't always be the parent.
The parent could be MemberAccessExpression whose parent is SuppressNullableWarningExpression.
So, that will need traversing the tree up to make sure it'll work correctly.
Does it worth doing so?
There was a problem hiding this comment.
By the way, I believe there are many other cases that aren't handled correctly. For example, the colon in inheritance vs ternary operator.
Do you see a way to refactor the whole thing instead of keeping special-casing more things? probably make it based on semantics instead of syntax?
There was a problem hiding this comment.
The parent could be MemberAccessExpression whose parent is SuppressNullableWarningExpression.
What scenario would have this?
There was a problem hiding this comment.
I looked again in SharpLab and it was just my eyes incorrectly seeing sibling nodes as parent/children 😄 . So this approach should just work.
davidwengier
added
Area-IDE
Community
src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs
Show resolved
Hide resolved
…Service.cs Co-authored-by: Allison Chou <allichou@microsoft.com>
src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs
Outdated
Show resolved
Hide resolved
…Service.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
|
Hello @davidwengier! Because this pull request has the 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 (
|
|
@sharwell @davidwengier Can you re-run the integration tests please? |
|
I've restarted the leg than hung, and will monitor if it passes (and then if GitHub can be convinced that it has) |
ghost
merged commit 
ghost
added this to the 
Fixes #46988
@davidwengier Can you review? Also, we should get the docs PR (dotnet/docs#20341) merged first right?