Adding missing completion for switch when clause by Neme12 · Pull Request #25133 · dotnet/roslyn

Neme12 · 2018-02-28T21:49:41Z

…a declaration pattern

… semantic checks

CyrusNajmabadi · 2018-02-28T22:35:06Z

-@"switch (1) { case int i $$ }"));
+            await VerifyKeywordAsync(AddInsideMethod(@"switch (1) { case int i $$ }"));
+            await VerifyKeywordAsync(AddInsideMethod(@"switch (1) { case int i $$: }"));
+            await VerifyKeywordAsync(AddInsideMethod(@"switch (1) { case int i $$ break; }"));


note: in practice we prefer one test per test method. That way if something fails, you don't have to fix it, only to find the next line then fails.

hmm.. i actually thought about making a helper for this to test the same code all these cases... I think it would clutter up the tests to have 3 versions for each one. And when working on this, it never happened to me that 1 version would actually fail while another one didn't. I just added this as a precaution.

VerifyKeywordAsync itself actually does like 5 different tests for variations such as a partially written keyword, and it's all in 1 method

my point is... I don't think it's very likely that these variations would fail independently (as opposed to the tests hidden inside VerifyKeywordAsync, which often do)

CyrusNajmabadi · 2018-02-28T22:35:36Z

+class C
+{
+    const Color Color = null;
+    void M() { switch (new object()) { case int $$ } }


i don't understnd why there is "const Color Colon" in these tests.

Because I wanted to have the same environment in this set of tests and just do different things inside the switch statement... but you're right maybe it's not a good idea after all. It really doesn't help readability.

CyrusNajmabadi · 2018-02-28T22:38:22Z

        }

-        private static bool IsIdentifierInCasePattern(SyntaxNode node)
+        private static T GetAncestorUntilStatement<T>(SyntaxToken token) where T : SyntaxNode


Don't we already have a GetAncestor or FindAncestor call that could already be used?

Yes but it always goes all the way up to the root, I thought that was unnecessary.. but I guess it wouldn't be a performance right? so maybe this is kinda useless

Usually teh root is no more than 8 items high. So this is a microoptimization that i can't imagine ever being worth it :)

i didn't think of this that much as an optimization as just... I just want this to be as local search as possible, I don't really care what's outside.

Neme12 · 2018-02-28T23:10:14Z

        }

+        [Fact, Trait(Traits.Feature, Traits.Features.Completion)]
+        public async Task ExceptionFilter1_NotBeforeOpenParen()


just a precaution to make sure this never gets mixed up with when inside switch where there's no parentheses required

Neme12 · 2018-02-28T23:16:48Z


            return false;
+
+            bool NotATypeName(SyntaxNode node)


Naming is hard, especially with negatives... After 10 minutes of pondering and constantly changing the name, I settled on this. Even adding 'Is' makes this slightly less readable, not more. My conclusion was that a longer name wouldn't be an improvement here.

Neme12 · 2018-02-28T23:21:30Z

+                return false;
+            }
+
+            // If the last token missing, the expression is incomplete - possibly because of missing parentheses,


note to myself: last token is missing

Neme12 · 2018-03-01T00:20:58Z

+
+        [WorkItem(25084, "https://github.com/dotnet/roslyn/issues/25084")]
+        [Fact, Trait(Traits.Feature, Traits.Features.Completion)]
+        public async Task SwitchPatternCaseWhenClause()


note to myself: they're all pattern cases if they have 'when'... wording doesn't make sense

Neme12 · 2018-03-01T00:21:40Z

+
+            if (lastToken == context.TargetToken)
+            {
+                return NotATypeName(expressionOrPattern);


note: consider TypeSyntax inside a constant pattern & whether that's relevant here

Neme12 · 2018-03-01T00:26:10Z

Question:

case 1 | when

offer when before an existing one? Usually completion ignores this sort of thing and provides completion based on what's to the left, so I would think yes. This would be an easy check though.

But I think I could see myself clicking after the 1 to type a new when clause (expecting that completion would provide it based on what's to the left) before deleting the other one, or more likely just experimenting/trying to see what I can type there. I would expect it to show up personally, because I'm used to it working this way in all other places.

CyrusNajmabadi · 2018-03-01T00:29:03Z

offer when before an existing one? Usually completion ignores this sort of thing and provides completion based on what's to the left, so I would think yes.

Yes. completion should care only what is to the left 99% of the time (unless there is a VERY good reason to do otherwise). This is because sometimes people jsut want to restart a thought, and we shouldn't penalize them for what's coming after where they currently are.

Neme12 · 2018-03-01T00:31:49Z

Then I need to fix the code. It is indeed offered now, but it bypasses the semantic check for whether the expression is actually a type.

case SyntaxNode | when

it will currently offer 'when' even if SyntaxNode is known to be a type. (Without the 'when', it won't be offered. This is because I check for TypeSyntax but not inside a constant pattern, as would happen if the 'when' clause is added)

Neme12 · 2018-03-02T20:18:55Z

With var it's still not great:

Neme12 · 2018-03-02T20:29:55Z

Actually with var it's especially bad because for some reason var doesn't trigger suggestion mode:

…constant

jcouv

LGTM Thanks!

jcouv · 2018-03-20T18:24:15Z

@dotnet/roslyn-ide for review and merging of a community PR. Thanks

jcouv · 2018-03-27T19:15:03Z

@dotnet/roslyn-ide for review and merging of a community PR for 15.7. Thanks

Neme12 · 2018-03-27T22:07:06Z

+
+        [Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
+        public async Task TestForSwitchCase_NotAfterIncompleteMemberAccess() =>
+            await VerifyAbsenceAsync(AddInsideMethod(@"switch (1) { case int.$$ }"));


well... now I have an urge to change this to use the same pattern as I did in #25703 (use a theory called 'NotAfterIncompleteExpression' with all these cases as inline data)

even though here there wouldn't be as big of a benefit since all these tests are just one line

I quickly made the change, I hope it's fine

Neme12 · 2018-03-27T22:58:56Z

@jcouv Do we need to ask @Pilchie or someone if this is wanted for 15.7 or not? Isn't it too late for that?

Neme12 · 2018-03-27T23:18:15Z

I see the build failure is the same as #25689 due to some moving of test helpers to a different namespace that happened on master recently (#25643) - I haven't done a merge since then. ~~But that is only present on master so if this were to go to 15.7, I shouldn't port that change in.~~ I was wrong, I see it was merged to 15.7.

jinujoseph · 2018-03-27T23:20:56Z

@Neme12 not late for 15.7 , @dpoeschl to review

Neme12 · 2018-03-28T21:56:12Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

jcouv · 2018-03-28T22:39:03Z

@Neme12 I don't generally worry about the integration tests much, as they are fairly noisy and don't block merging. IDE team can correct me if that's wrong.

Neme12 · 2018-03-29T22:26:23Z

retest windows_debug_vs-integration_prtest please

Neme12 · 2018-03-29T22:26:41Z

retest windows_release_vs-integration_prtest please

Neme12 · 2018-03-30T16:38:27Z

retest windows_debug_es_unit32_prtest please

jcouv · 2018-04-03T17:41:35Z

@dpoeschl @dotnet/roslyn-ide This community PR is waiting for a last review. It would be great to merge in time for 15.7. Thanks

jinujoseph · 2018-04-03T17:55:23Z

Sorry for the delay @Neme12 , we will get this reviewed and make a call on which release today

dpoeschl

LGTM. Thanks @Neme12!

dpoeschl · 2018-04-03T22:51:26Z

FYI @jinujoseph

dpoeschl · 2018-04-03T22:52:26Z

And don't worry about windows_debug_es_unit32_prtest failing. It's not required and a work in progress.

jcouv · 2018-04-03T23:10:01Z

retest this please

jcouv · 2018-04-03T23:10:55Z

I re-triggered a CI run as the PR last passed a week ago, but the branch changed since.
@jinujoseph for ask-mode approval. Thanks

jinujoseph · 2018-04-04T00:52:59Z

@jcouv Lets take this for 15.8
@Neme12 thanks for the PR !!

Neme12 · 2018-04-04T12:51:45Z

Awesome, thanks!

Neme12 added 5 commits February 28, 2018 00:41

Adding 'when' keyword completion for any switch case, not just after …

821ae7d

…a declaration pattern

Adding full completion after 'when'

728ee14

Fixing (removing) suggestion mode after declaration pattern

37f9747

Not offering 'when' after identifier if it's known to be a type using…

dc1e3df

… semantic checks

Fix space

dca7aac

Neme12 requested a review from a team as a code owner February 28, 2018 21:49

CyrusNajmabadi reviewed Feb 28, 2018

View reviewed changes

Neme12 commented Feb 28, 2018

View reviewed changes

Neme12 commented Mar 1, 2018

View reviewed changes

Neme12 added 7 commits March 2, 2018 18:26

Removing custom method

416c6b4

Cleaning up tests

d12a660

Adding extra tests

357cea3

Fixing NotATypeName check for expression inside constant pattern

bb74a30

Refactoring the logic

11b6c1a

Adding BeforeWhen version to all tests where it makes sense

248a85f

Moving the test variations into their own methods

c325360

Using AddInsideMethod where possible

3b8aa9a

Neme12 added 3 commits March 2, 2018 21:33

Adding BeforeBreak version to SemanticCheck tests too

21f0b77

Using expression bodied tests

67b6f1c

Not offering 'when' after var except when var is an actual name of a …

2a7f58d

…constant

jcouv approved these changes Mar 20, 2018

View reviewed changes

Neme12 commented Mar 27, 2018

View reviewed changes

Using theory

80de137

Neme12 added 3 commits March 28, 2018 01:28

Merge branch 'master' into whenRecommender

cfafb50

Fix after merge

0563b88

Simplifying

bb31aad

dpoeschl approved these changes Apr 3, 2018

View reviewed changes

jinujoseph modified the milestones: 15.7, 15.8 Apr 4, 2018

jinujoseph approved these changes Apr 4, 2018

View reviewed changes

jinujoseph merged commit 0882a48 into dotnet:master Apr 4, 2018

Neme12 deleted the whenRecommender branch April 4, 2018 23:47

Conversation

Neme12 commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Feb 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Feb 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Feb 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Feb 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Feb 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 1, 2018 • 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!

Neme12 commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 1, 2018

Uh oh!

Neme12 commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 commented Mar 2, 2018

Uh oh!

Neme12 commented Mar 2, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 20, 2018

Uh oh!

jcouv commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Mar 27, 2018

Uh oh!

Neme12 commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jinujoseph commented Mar 27, 2018

Uh oh!

Neme12 commented Mar 28, 2018

Uh oh!

Neme12 commented Feb 28, 2018 •

edited

Loading

Neme12 Feb 28, 2018 •

edited

Loading

Neme12 Feb 28, 2018 •

edited

Loading

Neme12 Feb 28, 2018 •

edited

Loading

Neme12 Feb 28, 2018 •

edited

Loading

Neme12 Feb 28, 2018 •

edited

Loading

Neme12 Mar 1, 2018 •

edited

Loading

Neme12 commented Mar 1, 2018 •

edited

Loading

Neme12 commented Mar 1, 2018 •

edited

Loading

Neme12 commented Mar 27, 2018 •

edited

Loading

dpoeschl commented Apr 3, 2018 •

edited

Loading