Offer 'in' as completion in operator #24089
Conversation
| namespace Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery | ||
| { | ||
| internal static class SyntaxNodeExtensions | ||
| { | ||
| public static bool IsDelegateOrConstructorOrLocalFunctionOrMethodParameterList(this SyntaxNode node) | ||
| public static bool IsDelegateOrConstructorOrLocalFunctionOrMethodOrOperatorParameterList(this SyntaxNode node, bool includeOperators) |
There was a problem hiding this comment.
Do we really need the extra parameter? What's the consequence of just returning true for all of the operators as well all of the time?
There was a problem hiding this comment.
Basically, it's not terrible if we offer keywords even when not always applicable. That's still much better than not offering it. Then, we could just make this cod really simple. Basically: "offer these things if it's in a parameter list", even if that allows some things when not applicable.
There was a problem hiding this comment.
The impact is offering out and ref in places where they are not applicable.
Sure. I'll remove the parameter.
There was a problem hiding this comment.
Yeah... at this point i think it's fine to offer in all argument cases. It's unlikely anyone would notice. And the worst impact is they type something that the compiler then tells you is not valid.
|
@dotnet/roslyn-ide for review. Thanks |
| { | ||
| await VerifyAbsenceAsync( | ||
| await VerifyKeywordAsync( |
There was a problem hiding this comment.
❗️ params is not valid in this context (CS1670).
| public async Task TestNotAfterOperator() | ||
| { | ||
| await VerifyAbsenceAsync( | ||
| await VerifyKeywordAsync( |
There was a problem hiding this comment.
❗️ out is not valid in this context (CS0631).
| { | ||
| await VerifyAbsenceAsync( | ||
| await VerifyKeywordAsync( |
There was a problem hiding this comment.
❗️ ref is not valid in this context (CS0631).
|
@jcouv Considering we were not working around a performance problem, I'd rather implement this correctly instead of approximately (i.e. revert the last commit which caused regressions for |
|
I'll let @CyrusNajmabadi comment. I don't know what is the general rule for recommenders in terms of simplicity vs. tightness trade-offs. |
|
The general rule was: whatever i felt like when i originally implemented it :D The next most general was: the more parameters i needed to add, and the more special cases, the less i wanted to do it. This started out simply a long time ago: If method/constructor do X. Now it's a lot of different cases and stuff to try to continually maintain as the language becomes more permissive. I think it's reasonable to just say "when typing arguments, let people write this". Importantly, the completion list doesn't serve to tell you exactly what's legal, but to help speed up typing. That's still accomplished even when extra items are shown. -- Then again. I don't really care. if @sharwell feels even slightly strong about this, that's fine with me :) |
|
@sharwell Removed the second commit. Please review. Thanks |
|
Retest windows_debug_vs-integration_prtest please |
|
Retest ubuntu_14_debug_prtest please |
|
@Pilchie for approval. Thanks |
|
Approved - but needs to be retargeted to the 15.6 branch if you want to ship in that release. |
|
@Pilchie This PR was already retargeted to |
Customer scenario
Type a custom operator. When you reach the parameters,
inshould be offered as a completion.Bugs this fixes
Fixes #24079
Workarounds, if any
Type
indespite lack of completion.Risk
Performance impact
Low. Adding a syntax kind check to a method that is used for recommending
in. That method is only used by similar recommenders (outand such) and those skip the new logic.Is this a regression from a previous update?
No.
How was the bug found?
Reported by customer.