Completion for property-based pattern and 'switch' keyword in switch expression by jcouv · Pull Request #26553 · dotnet/roslyn

jcouv · 2018-05-02T02:52:58Z

The first commit in this PR is the subset of changes from last PR (#26474) which relate to completion. The second commit adds some tests and adopts the GetTypeInfo API.
I'll start a separate PR for the subset of changes related to formatting.

@Neme12 @CyrusNajmabadi Sorry for the PR churn. I believe this will be easier to review this way. Also, this allowed me to rebase to use new GetTypeInfo API which addressed many of the comments.

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

CyrusNajmabadi · 2018-05-02T03:13:24Z

my hope is that this code is very similar to similar code we have for object initializers. Is that the case? If you did have to deviate can you give the broad strokes of why/how?

Yes, I followed object initializers very closely. I copied the abstract object initializer code to start with ;-)

CyrusNajmabadi · 2018-05-02T03:15:17Z

this method looks pretty busted in many ways. For example:

if (token.GetAncestor<BlockSyntax>() == null)

That seems like it means that we won't get 'switch' completion in an arrow method. Can you add test for this and fix up this method accordingly.

Note: afaict this method could be so much simpler. It's really just "are we after a complete expression"

nit: consider renameing "IsAfterCompletelyWrittenExpression". but i don't care :)

…expression

Neme12 · 2018-05-02T08:35:49Z

+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders
+{
+    [Trait(Traits.Feature, Traits.Features.Completion)]
+    public class PropertySubPatternCompletionProviderTests : AbstractCSharpCompletionProviderTests


nit: Pattern should probably be lowercase to match SyntaxKind.PropertySubpattern

Neme12 · 2018-05-02T08:41:07Z

+        _ = this is ( , { $$ })
+    }
+
+    public void Deconstruct(out Program x, out Program y) => throw null;


consider changing this test or adding a new one where these two types are different

Neme12 · 2018-05-02T08:42:44Z

+        }
+
+        [Fact]
+        public async Task PropertiesInRecursivePattern_WithPositionalPattern()


is this test different from PropertiesInRecursivePattern_WithPositional?

Neme12 · 2018-05-02T08:43:36Z

+
+    void M()
+    {
+        _ = this is Program (1, 2) { $$ }


consider making this type different so that this test expects a different behavior from the one below

Neme12 · 2018-05-02T08:46:28Z

+            var document = context.Document;
+            var position = context.Position;
+            var cancellationToken = context.CancellationToken;
+            var workspace = document.Project.Solution.Workspace;


it looks like workspace is unused

Neme12 · 2018-05-02T08:52:17Z

+            var workspace = document.Project.Solution.Workspace;
+            var semanticModel = await document.GetSemanticModelForSpanAsync(new TextSpan(position, length: 0), cancellationToken).ConfigureAwait(false);
+
+            var token = TryGetOpenBraceOrCommaInPropertyPattern(document, semanticModel, position, cancellationToken);


It looks like TryGetOpenBraceOrCommaInPropertyPattern only uses the semanticModel to get the syntax tree. I would prefer if we could:

pass in the syntax tree directly

retrieve the syntax tree through other means than the semantic model and only get the semantic model after the early return in if (token == default) below

Neme12 · 2018-05-02T08:55:29Z

+            => CompletionUtilities.IsTriggerCharacter(text, characterPosition, options) || text[characterPosition] == ' ';
+
+        private static SyntaxToken TryGetOpenBraceOrCommaInPropertyPattern(
+            Document document, SemanticModel semanticModel, int position, CancellationToken cancellationToken)


apart from my comment about the semanticModel, it looks like document is completely unused

Neme12 · 2018-05-02T08:56:59Z

+        internal override bool IsInsertionTrigger(SourceText text, int characterPosition, OptionSet options)
+            => CompletionUtilities.IsTriggerCharacter(text, characterPosition, options) || text[characterPosition] == ' ';
+
+        private static SyntaxToken TryGetOpenBraceOrCommaInPropertyPattern(


nit: should be named TryGetOpenBraceOrCommaInPropertySubpattern because it doesn't actually have to be inside a PropertyPattern

Neme12 · 2018-05-02T08:57:51Z

+                return default;
+            }
+
+            return (token.Parent.IsKind(SyntaxKind.PropertySubpattern) == true) ? token : default;


IsKind returns bool so == true is not needed

if you remove that, please remove the parens as well

Neme12 · 2018-05-02T09:00:05Z

+            }
+
+            var token = tree.FindTokenOnLeftOfPosition(position, cancellationToken);
+            token = token.GetPreviousTokenIfTouchingWord(position);


nit: consider merging this with the previous statement: .FindTokenOnLeftOfPosition(...).GetPreviousTokenIfTouchingWord

Left as-is. I find it better for debugging.

Neme12 · 2018-05-02T09:01:21Z

+            }
+
+            var pattern = (PatternSyntax)token.Parent.Parent;
+            var type = semanticModel.GetTypeInfo(pattern).ConvertedType;


pass in cancellationToken

Neme12 · 2018-05-02T09:03:47Z

+                return true;
+            }
+
+            if (symbol.IsKind(SymbolKind.Property) && !((IPropertySymbol)symbol).IsWriteOnly)


nit: please use pattern matching

what about indexers? are we sure they're not offered?

Correct, no indexers in current recursive patterns. See grammar.

Yes, I just wanted to make sure we correctly don't offer indexers as opposed to showing something like this[] in the completion list, because they're a property symbol after all. Maybe they're filtered out somewhere further down in the completion logic so you don't have to worry about that. But if not, there would need to be an additional check here: !property.IsIndexer && !property.IsWriteOnly But I don't know if that's needed, that's why I was asking.

Neme12 · 2018-05-02T09:07:22Z

+                !m.IsImplicitlyDeclared);
+
+            // Filter out those members that have already been typed
+            var alreadyTestedMembers = GetTestedMembers(semanticModel.SyntaxTree, position, cancellationToken);


you don't need to pass in the syntax tree or the position. it is only used inside to find the target token which is already found at this point, so you can pass in token directly

Neme12 · 2018-05-02T09:08:14Z

+        }
+
+        // List the members that are already tested in this property sub-pattern
+        private static HashSet<string> GetTestedMembers(SyntaxTree tree, int position, CancellationToken cancellationToken)


if you pass token directly, you can remove all these 3 parameters

Neme12 · 2018-05-02T09:09:17Z

+                .GetPreviousTokenIfTouchingWord(position);
+
+            // We should have gotten back a { or ,
+            if (token.IsKind(SyntaxKind.CommaToken, SyntaxKind.OpenBraceToken))


also you can get rid of this check, because this was already done inside TryGetOpenBraceOrCommaInPropertyPattern. the token you get from that will always satisfy this

Neme12 · 2018-05-02T09:10:30Z

+            // We should have gotten back a { or ,
+            if (token.IsKind(SyntaxKind.CommaToken, SyntaxKind.OpenBraceToken))
+            {
+                if (token.Parent is PropertySubpatternSyntax subpattern)


actually, maybe this method should only accept PropertySubpatternSyntax as a parameter, not the token itself

Neme12 · 2018-05-02T09:11:38Z

+            {
+                if (token.Parent is PropertySubpatternSyntax subpattern)
+                {
+                    return new HashSet<string>(subpattern.SubPatterns.Select(


hm, it looks like the compiler itself is inconsistent about the casing of SubPattern

also, the name of this property is a little confusing... it doesn't return subpatterns... maybe it should just be Elements?

The syntax tree is likely to be revised (#26540). I took a note there to review casing. For naming, feel free to chime in there or in the PR (once available).

jcouv · 2018-05-03T17:38:07Z

@DustinCampbell @dotnet/roslyn-ide for review. Thanks

This completion provider is much like the one for object creation.
When you type a property pattern { Property1: pattern1, Property2, pattern2, ...} we'll offer completion on "Property1" and "Property2".

Neme12 · 2018-05-03T17:50:57Z

 {
    [Trait(Traits.Feature, Traits.Features.Completion)]
-    public class PropertySubPatternCompletionProviderTests : AbstractCSharpCompletionProviderTests
+    public class PropertySubpatternCompletionProviderTests : AbstractCSharpCompletionProviderTests


tiny nit: the file name doesn't match anymore

jcouv · 2018-05-03T19:06:59Z

@jaredpar @gafter @dotnet/roslyn-infrastructure For some reason, it looks like the CI legs are no longer getting triggered on this branch (features/recursive-patterns). They were working fine earlier during this PR.

jasonmalinowski · 2018-05-03T23:21:30Z

@dotnet-bot retest this please.

Neme12 · 2018-05-03T23:55:27Z

Note: there should also be a completion provider for name: inside a deconstruction pattern, similar to the one for named arguments. Please file an issue for that

jcouv · 2018-05-04T00:31:30Z

@jasonmalinowski I still only see two CI legs

jcouv · 2018-05-04T00:32:04Z

@jasonmalinowski Never mind. Closing and re-opening fixed the issue. Thanks

jcouv · 2018-05-04T00:36:19Z

@Neme12 Filed #26619 (offer completion in deconstruction pattern)

jcouv · 2018-05-07T15:05:57Z

@DustinCampbell @dotnet/roslyn-ide All the feedback so has been addressed (with the exception of a file rename, which I'll do as last thing). Please review.
This PR offers completion in property-based patterns (similar to completion in object creation) and recommends switch keyword for switch expressions.

jcouv · 2018-05-08T15:54:02Z

@dotnet/roslyn-ide Please review.

jcouv · 2018-05-09T19:24:48Z

@dotnet/roslyn-ide Please review. This PR didn't receive feedback for a week. Thanks

CyrusNajmabadi · 2018-05-09T19:26:44Z

@jcouv i can look later tonight if htat helps!

CyrusNajmabadi · 2018-05-09T21:07:38Z

+";
+            // VerifyItemExistsAsync also tests with the item typed.
+            await VerifyItemExistsAsync(markup, "P1");
+            await VerifyItemExistsAsync(markup, "P2");


do these test when nothing after the $$ is in the file? Or do we need specific tests for that?

@CyrusNajmabadi they do not (I was curious about making them do that once)

CyrusNajmabadi · 2018-05-09T21:08:37Z

+{
+    void M()
+    {
+        _ = this is ({ $$ }) // no deconstruction into 1 element


this is a shame because it dpeends on the rest of the contents being there after the {}. What happen if all you've typed is ```({ $$``?

I think we should be able to make deconstruction works like method invocations, so that we use our best guess/overload while you're typing. Tracked by #26470

I'll add the test you suggested

I think the question is: will this work if you only type ({ $$ without the closing brace and paren? is that even parsed as a pattern?

It is parsed as a pattern (I suspect because what can follow an is is fairly restricted, so that guides the parsing).
But the semantic information is no better than when the closing brace and paren are present.

CyrusNajmabadi · 2018-05-09T21:09:49Z

+            }
+
+            var semanticModel = await document.GetSemanticModelForSpanAsync(new TextSpan(position, length: 0), cancellationToken).ConfigureAwait(false);
+            var pattern = (PatternSyntax)token.Parent.Parent;


so PropertySubpatterns must be parented by patterns? Any concern aobut that ever changing? If so, consider an "is" check.

CyrusNajmabadi · 2018-05-09T21:10:21Z

+            IEnumerable<ISymbol> members = semanticModel.LookupSymbols(position, type);
+            members = members.Where(m => m.CanBeReferencedByName &&
+                IsFieldOrReadableProperty(m) &&
+                !m.IsImplicitlyDeclared);


can implicitly declared members ever be referenced by name? (i'm guessing this is copied from elsewhere... if so, fine to leave as is).

Yes, this is copied from completion logic for object initializers ;-)

CyrusNajmabadi · 2018-05-09T21:12:06Z

+        }
+
+        // List the members that are already tested in this property sub-pattern
+        private static HashSet<string> GetTestedMembers(PropertySubpatternSyntax propertySubpattern)


i feel like this could be inlined. up to you.

jcouv · 2018-05-10T20:14:58Z

@jinujoseph Good to merge?

jcouv · 2018-05-10T22:48:01Z

Thanks all!
I'll go ahead and merge.

jinujoseph · 2018-05-10T22:48:02Z

@jcouv thanks, good to merge.
Thanks @dpoeschl for reviewing

jcouv added Area-IDE Feature - Pattern Matching Pattern Matching labels May 2, 2018

jcouv added this to the 16.0 milestone May 2, 2018

jcouv self-assigned this May 2, 2018

jcouv requested a review from a team as a code owner May 2, 2018 02:52

CyrusNajmabadi reviewed May 2, 2018

View reviewed changes

jcouv mentioned this pull request May 2, 2018

Some fixes for patterns in IDE #26474

Closed

jcouv added 2 commits May 1, 2018 20:52

Completion for property-based pattern and 'switch' keyword in switch …

3ced6c4

…expression

Use GetTypeInfo

1c6184b

jcouv force-pushed the patterns-completion branch from dffe09b to 1c6184b Compare May 2, 2018 03:54

Recommend 'switch' in arrow-bodied members

37b6206

jcouv force-pushed the patterns-completion branch from 2e6e0c4 to 37b6206 Compare May 2, 2018 04:02

Neme12 reviewed May 2, 2018

View reviewed changes

Neme12 reviewed May 3, 2018

View reviewed changes

jcouv closed this May 4, 2018

jcouv reopened this May 4, 2018

jcouv mentioned this pull request May 4, 2018

Complete names inside deconstruction pattern #26619

Closed

CyrusNajmabadi reviewed May 9, 2018

View reviewed changes

CyrusNajmabadi approved these changes May 9, 2018

View reviewed changes

Address PR feedback from Cyrus

e68b09d

dpoeschl approved these changes May 10, 2018

View reviewed changes

jinujoseph added Approved to merge and removed Approved to merge labels May 10, 2018

jcouv merged commit b7b14b6 into dotnet:features/recursive-patterns May 10, 2018

jcouv deleted the patterns-completion branch May 10, 2018 22:48

Conversation

jcouv commented May 2, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

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!

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 May 2, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 May 2, 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 May 2, 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 May 3, 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 May 2, 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 May 2, 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 May 2, 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 May 2, 2018 •

edited

Loading

Neme12 May 2, 2018 •

edited

Loading

Neme12 May 2, 2018 •

edited

Loading

Neme12 May 3, 2018 •

edited

Loading

Neme12 May 2, 2018 •

edited

Loading

Neme12 May 2, 2018 •

edited

Loading

Neme12 May 2, 2018 •

edited

Loading

Neme12 commented May 3, 2018 •

edited

Loading