Autocomplete eagerly completes before second `.` in Range declaration by ivanbasov · Pull Request #35749 · dotnet/roslyn

ivanbasov · 2019-05-16T01:19:57Z

CyrusNajmabadi · 2019-05-16T03:04:20Z

...Features/CSharp/Portable/Completion/SuggestionMode/CSharpSuggestionModeCompletionProvider.cs

+            }
+
+            // On walking up, we should meet BracketedArgumentList before Statement. 
+            // Otherwise, it is not a range expression.


this is a very weak check. For example, ranges are allowed in basically any expression context. And, unlike lambdas, they don't need a specific type at that location for inference. i.e. it is expected that someone may write: Foo(a..b). Having completion mess with this would be concerning.

A better check is "is this a location we expect a range. That will at least deal with the Foo(a..b) case. However, it won't deal with the legal case of writing var x = a..b;. It may be that that case isn't important enough, or that we need to hear about feedback about it before doing something. HOwever, we should not be explicitly looking for [ as that's only a small subset of places we might have type information that indicates a range is acceptable. #Pending

Yes, if we can provide a more delicate approach, we should wider it something like this:
If there is nothing after dot and the type of the symbol before the dot is numeric, we should:

ignore completion if a second dot is typed immediately after.

proceed with a regular completion, if any other character was typed after.

Please correct me if you see more conditions to consider.

In reply to: 284523908 [](ancestors = 284523908)

CyrusNajmabadi · 2019-05-16T03:07:28Z

...Features/CSharp/Portable/Completion/SuggestionMode/CSharpSuggestionModeCompletionProvider.cs

                }
+                if (IsRangeExpression(semanticModel, token, cancellationToken))
+                {
+                    return CreateSuggestionModeItem(CSharpFeaturesResources.range_expression, CSharpFeaturesResources.Autoselect_disabled_due_to_potential_range_expression);


i don't think this is waht we want. instead, i think we just want to change how completin works such that if we think we're in a range-context (which we shuld use similar logic to doing 'lambda contxt' detection), then we change how commit works so that . only commits if you've typed any characters. That way if you're in a range context and you type foo<dot><dot> you get foo... but if you type foo<dot>b<dot> you end up with foo.bar. #Pending

Thank you, @CyrusNajmabadi! Agree.

Will investigate other options rather than suggestion mode for this.

I'm not sure I understand the idea for the lambda context. Do you mean we should correct it as well? Or do you mean it is implemented properly and we should borrow the approach?

In reply to: 284524359 [](ancestors = 284524359)

CyrusNajmabadi

This approach is a starting point, but i don't think it's the right way to go.

it fails to detect usages of ranges in other places they may appear (like Range r = 1..100).
it is changing us to suggestion mode, when really it should be configuring how commit works.

Request changes
Submit feedback suggesting changes.

CyrusNajmabadi · 2019-05-16T21:15:59Z

Do you mean we should correct it as well? Or do you mean it is implemented properly and we should borrow the approach?

Somehwat of a mix of both. Allow me explain:

the lambda-case is one where we want to determine if we're a declaration location so that we can switch to suggestion mode. In that regard it's not what you want to follow. However, what the lambda-case does do properly is determining "is this a context where a lambda would be expected". In that regard, you would want to do something similar. i.e. you don't check for [, you instead check if this is where a Range would be appropriate. Examples of that (which need semantics) are:

Range r = a. //<-- here
Foo(a. //<-- here when Foo takes a Range
arr[a. //<-- here because you have something that has an indexer and a length and the compiler will translated
str[a. //<-- same

etc.

The ITypeInferenceService can help out here a little. But it's likely not going to cover all the cases you need to care about. So you'll have to produce a fuller solution that will allow you to hit all the major cases. #Resolved

CyrusNajmabadi · 2019-05-16T21:19:26Z

Note: i also realized that part of my "solution" mentions a capability that may not exist in the new async completion system. i.e. i want to dynamically change the set of commit characters depending on how many have been written. i.e. if you have:

Range r = a.  //<-- at this point <dot> should not be a completion char.

however, if you do:

Range r = a.f //<-- typing this should have the next <dot> complete whatever `f` matches against.

Is that possible with async completion? #Resolved

CyrusNajmabadi · 2019-05-16T21:25:35Z

Yes, if we can provide a more delicate approach, we should wider it something like this:
If there is nothing after dot and the type of the symbol before the dot is numeric, we should:

ignore completion if a second dot is typed immediately after.
proceed with a regular completion, if any other character was typed after.

My gut tells me this should likely be ok. We may want to only have that happen if you're also in a known range-context... but perhaps we don't need that.

My main concern is someone typing this: string s = num.. this will now not complete on <dot> until something is typed... but perhaps that is actually totally fine. I mean... ints have no fields that are interesting to reference here. And even if you wanted to select and complete something, you'd likely at least type something.

So i think i've convinced myself that your approach is very reasonable and desirable! #Resolved

ivanbasov · 2019-05-17T22:09:16Z

Great! Thank you, @CyrusNajmabadi! I reduced checks. Everything going after a numeric type is a potential range expression.

In reply to: 493237555 [](ancestors = 493237555)

ivanbasov · 2019-05-17T22:10:06Z

Yes, this is implemented in both completions with CompletionItemSelectionBehavior

In reply to: 493235772 [](ancestors = 493235772)

CyrusNajmabadi · 2019-05-17T22:29:00Z

I reduced checks. Everything going after a numeric type is a potential range expression.

Great! I think this is def a good place to start with. Hopefully we don't hear about any problems and this will be sufficient until the next difficult language feature :D #Resolved

CyrusNajmabadi · 2019-05-17T22:29:40Z

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/CSharpSyntaxContext.cs

                                            targetToken.Parent.IsKind(SyntaxKind.DestructorDeclaration) &&
                                            targetToken.Parent.Parent.IsKind(SyntaxKind.ClassDeclaration, SyntaxKind.StructDeclaration);

+            var isSoftSelectionContext = IsLeftSideOfNumericType(leftToken, semanticModel, cancellationToken);


needs strong comment explaining :) #Resolved

CyrusNajmabadi · 2019-05-17T22:30:39Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+                {
+                    rules = rules.WithSelectionBehavior(PreselectedItemSelectionBehavior);
+                }
            }


somethin feels not great here can it not be the case that someone can just supply the SelectionBehavior they want? #Pending

Here are three options of the CompletionItemSelectionBehavior
https://github.com/dotnet/roslyn/blob/master/src/Features/Core/Portable/Completion/CompletionItemRules.cs#L9.

Usually, there are preselected items. In the case, they will be hard selected: https://github.com/dotnet/roslyn/blob/master/src/Features/CSharp/Portable/Completion/CompletionProviders/SymbolCompletionProvider.cs#L145

We explicitly does not want any item to be hard selected in case of typing a single dot after a numeric. @CyrusNajmabadi, do you see a scenario when we still should allow a hard selection in the case?

In reply to: 285306982 [](ancestors = 285306982)

ok... so looking at thsi, i dont' like it, but i think it's a fallout from history (and so i don't thnk it's on you to have to change or make things better). Namely, we have an impedence mismatch betwene how we used to represent things (i.e. "soft select" or "preselect") and how our current final representation works (i.e. "selection behavior"). This feels like an attempt to munge over these different styles, rather than just updating the providers to use the new approach. This was likely convenient, but it just means wonkiness like this.

So, i'm fine with this staying as i think "the right thing" here is likely pretty large and complex to do. #Resolved

Thank you for explanations, @CyrusNajmabadi!
I'll take this into account and see how we can improve this when get a better understanding of how it should be arranged. Please do not hesitate to share any advise on this.

In reply to: 287161564 [](ancestors = 287161564)

CyrusNajmabadi · 2019-05-18T14:27:51Z

@ivanbasov are there tests for typing a.ToStr( where a is numeric? Thanks! #Resolved

ivanbasov · 2019-05-23T22:19:57Z

Added. Thank you!

In reply to: 493681124 [](ancestors = 493681124)

CyrusNajmabadi · 2019-05-23T22:47:52Z

...s/Core/Portable/Completion/Providers/AbstractRecommendationServiceBasedCompletionProvider.cs

+            }
+            else
+            {
+                if (preselect)


nit: merge this with the else. #Resolved

CyrusNajmabadi · 2019-05-23T22:48:04Z

src/Features/Core/Portable/Completion/Providers/AbstractSymbolCompletionProvider.cs

                rules: GetCompletionItemRules(symbols).WithMatchPriority(
-                    preselect ? MatchPriority.Preselect : MatchPriority.Default));
+                    preselect ? MatchPriority.Preselect : MatchPriority.Default)
+                .WithSelectionBehavior(context.IsSoftSelectionContext ? CompletionItemSelectionBehavior.SoftSelection : CompletionItemSelectionBehavior.Default));


nit: indent #Resolved

ideally: write as:

GetCompletionItemRules(symbols) .WithMatchPriority(preselect ? MatchPriority.Preselect : MatchPriority.Default)) .WithSelectionBehavior(context.IsSoftSelectionContext ? CompletionItemSelectionBehavior.SoftSelection : CompletionItemSelectionBehavior.Default)); ``` #Resolved

CyrusNajmabadi · 2019-05-23T22:49:27Z

src/Features/Core/Portable/Completion/Providers/AbstractSymbolCompletionProvider.cs

                using (Logger.LogBlock(FunctionId.Completion_SymbolCompletionProvider_GetItemsWorker, cancellationToken))
                {
-                    var regularItems = await GetItemsWorkerAsync(document, position, options, preselect: false, cancellationToken: cancellationToken).ConfigureAwait(false);
+                    var syntaxContext = await GetOrCreateContext(document, position, cancellationToken).ConfigureAwait(false);


nice savings #Resolved

CyrusNajmabadi · 2019-05-23T22:50:14Z

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/CSharpSyntaxContext.cs

                   isAttributeNameContext, isEnumTypeMemberAccessContext, isNameOfContext,
                   isInQuery, isInImportsDirective, IsWithinAsyncMethod(), isPossibleTupleContext,
-                   isPatternContext, cancellationToken)
+                   isPatternContext, isSoftSelectionContext, cancellationToken)


this really isn't appropriate. this is putting a Completion concept into the SyntaxContext. what might be more appropriate here is simply "IsRightSideOfDotAfterNumericType" or something like that. completion can then interpret that as it wants. #Resolved

(looking below, call this IsRightSideOfNumericType). thanks. #Resolved

CyrusNajmabadi · 2019-05-23T22:50:41Z

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/CSharpSyntaxContext.cs

+            // - or it maybe a start of a range expression like numericExpression..anotherNumericExpression (starting C# 8.0) 
+            // Therefore, in the scenario, we want the completion to be __soft selected__ until user types the next character after the dot.
+            // If the second dot was typed, we just insert two dots.
+            var isSoftSelectionContext = IsRightSideOfNumericType(leftToken, semanticModel, cancellationToken);


this hsould just be var isRightSideOfNumericType and that should be the field/param name as well. #Resolved

CyrusNajmabadi

No real complaints except some naming (which should be done). I'm approving, but please make the cahnge. Thanks!

Approve
Submit feedback approving these changes.

ivanbasov · 2019-05-23T23:11:11Z

@dpoeschl , @genlu, @jasonmalinowski please review

ivanbasov · 2019-05-24T22:12:44Z

@dpoeschl , @genlu, @jasonmalinowski please review

ivanbasov · 2019-05-28T18:26:10Z

pinging @dpoeschl , @genlu, @jasonmalinowski for review

Autocomplete eagerly completes before second . in Range declaration

9a51087

ivanbasov added Area-IDE IDE-IntelliSense Completion, Signature Help, Quick Info labels May 16, 2019

ivanbasov added this to the 16.2.P2 milestone May 16, 2019

ivanbasov requested review from CyrusNajmabadi, genlu and jasonmalinowski May 16, 2019 01:19

ivanbasov requested a review from a team as a code owner May 16, 2019 01:19

CyrusNajmabadi reviewed May 16, 2019

View reviewed changes

CyrusNajmabadi suggested changes May 16, 2019

View reviewed changes

ivanbasov added 2 commits May 16, 2019 17:32

moving the fix from suggestion mode to symbolcompletionprovider. started

f6e6101

code review feedback

5122808

revert formatting

479db0a

ivanbasov force-pushed the range branch from 49a603a to 479db0a Compare May 17, 2019 22:20

CyrusNajmabadi reviewed May 17, 2019

View reviewed changes

code review feedback

06ece73

CyrusNajmabadi reviewed May 23, 2019

View reviewed changes

CyrusNajmabadi suggested changes May 23, 2019

View reviewed changes

code review feedback

360bb41

ivanbasov requested a review from dpoeschl May 23, 2019 23:10

resolving naming collision

c3b4fdd

dpoeschl approved these changes May 28, 2019

View reviewed changes

ivanbasov merged commit 7309697 into dotnet:master May 28, 2019

ivanbasov deleted the range branch May 28, 2019 20:25

ivanbasov modified the milestones: 16.2.P2, 16.2.P3 May 28, 2019

Youssef1313 mentioned this pull request Nov 5, 2020

Completion completes items innapropriately when trying to type range .. operator. #49185

Closed

Conversation

ivanbasov commented May 16, 2019

Uh oh!

CyrusNajmabadi May 16, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov May 16, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 16, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov May 16, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented May 16, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented May 16, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented May 16, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanbasov commented May 17, 2019

Uh oh!

ivanbasov commented May 17, 2019

Uh oh!

CyrusNajmabadi commented May 17, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi May 17, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 17, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov May 23, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov May 23, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented May 18, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanbasov commented May 23, 2019

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

CyrusNajmabadi May 16, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 16, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented May 16, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented May 16, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented May 16, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented May 17, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 17, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 17, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented May 18, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi May 23, 2019 •

edited by ivanbasov

Loading