Adding missing completion after 'ref' by Neme12 · Pull Request #25570 · dotnet/roslyn

Neme12 · 2018-03-18T15:21:13Z

Customer scenario

Type 'ref' and expect completion for 'readonly'
Type 'ref' or 'ref readonly' and expect symbol & keyword completion
(examples in the issue)

Bugs this fixes

fixes #25569

Workarounds, if any

Type whatever you need to type manually.

Risk

not sure

Performance impact

I only added syntax checks, so... low?

Is this a regression from a previous update?

No.

Root cause analysis

New language feature without full completion.

How was the bug found?

I found it by exploration.

… completion

Neme12 · 2018-03-18T15:25:33Z

+        // the syntax tree is completely broken and there is no lambda expression at all here.
+        // 'ref' starts a new local declaration and therefore we do offer 'readonly'.
+        // Fixing that would have to involve either changing the parser or doing some really nasty hacks.
+        // Delegate lambda = (ref $$ int p) => p;


not sure what to do here

I don't think this is a bug

Yeah, I agree with @CyrusNajmabadi. File a bug. We can do a better job with error recovery in the parser here.

jcouv · 2018-03-18T17:48:20Z

Thanks! Tagging @VSadov and @OmarTawfik as FYI

CyrusNajmabadi · 2018-03-18T21:27:21Z

            // cases:
            //  const var
+            //  ref var
+            //  ref readonly var


these comment should move down to the new block you added. right?

actually all cases are on the top of the method here, see the rest of the comment - it's all below code i added

or do you want me to move each comment to the relevant place where it's handled?

i'm not sure if i should make code style changes like that unrelated to what i'm doing and mess with half of this method by moving comments, given that this PR is already large enough

that's fair. do as you wish.

CyrusNajmabadi · 2018-03-18T21:27:42Z

            }

+            if (token.IsKind(SyntaxKind.RefKeyword, SyntaxKind.ReadOnlyKeyword) &&
+                token.Parent.IsKind(SyntaxKind.RefType) &&


didn't you jsut add a new helper that does this?

no I didn't... and even if I did, this is a special case because this is used for var, which can't be offer in other places after a ref type (eg in a method return type)

So this is not just the same as "IsAfterRefTypeContext"?

no it's not:

class C { ref $$ }

here you're in a ref type but not in a local declaration and can't use var

this method is called 'IsLocalVariableDeclarationContext'

CyrusNajmabadi · 2018-03-18T21:29:02Z

Tagging @dotnet/roslyn-ide @jinujoe . it looks large, but the majority is just a super-exhaustive test suite (yaay). Actual code change is quite minor and easy to review.

Neme12 · 2018-03-18T21:36:59Z

I should note: all the "type" "keywords" behave the same and have the same tests except for

var, as expected it's only offered after a ref type in a local declaration
dynamic - it's offered after a ref type but not after a ref expression (I can't think of any case where that would be legal - correct me if I'm wrong)

jcouv · 2018-03-19T16:54:28Z

+    }
+}
+";
+            await VerifyItemExistsAsync(markup, "String");


Why do we offer "String" here?

@jcouv String.Empty.MyRefReturningExtensionMethod()

jcouv · 2018-03-19T16:59:52Z

                context.IsPrimaryFunctionExpressionContext ||
                context.IsCrefContext ||
                syntaxTree.IsAfterKeyword(position, SyntaxKind.ConstKeyword, cancellationToken) ||
+                syntaxTree.IsAfterKeyword(position, SyntaxKind.RefKeyword, cancellationToken) ||


I'm surprised this doesn't fall under a more abstract category, like "IsTypeContext" or "IsPossibleTypeContext" (those are made-up, I'm not sure whether such methods exist). If there is not one, consider factoring one out, or filing an issue to do so.
Note: this would avoid duplicating the fix to multiple recommenders.

@dotnet/roslyn-ide for advice. Is there already a check that would be suitable for this?

One more thought on this. Is there a reason that all the recommenders for intrinsic types (bool, byte, string, object, etc) don't just share code?

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

@jcouv I was really surprised by this too and asked @CyrusNajmabadi who said this is very much by design. I would still like to do some cleanup here and reduce the duplication to some degree but I'm not going to do that as part of this PR. Is that OK?

@CyrusNajmabadi mentioned to me that for starters there are some cases (enum base list, stackalloc) where only some of those keywords are allowed, that's why they can't share everything and often do require special treatment. That said, I still think the duplication could be reduced significantly, but I'm treating this PR as a bugfix rather than code quality improvement/refactoring. It would not be as trivial as it seems.
(but I agree with you 100%, I don't like the code the way it is)

It woud require refactoring some of the existing helpers to be more fine-grained to support this sort of distinction. Yes, there is a helper called IsTypeContext but it is more general and offered in more places than these keywords, not less - so it's not as easy as using that helper and then adding more checks with ||.

But @CyrusNajmabadi has a lot more to say about this than I do. I talked to him about this and and he was clearly opposed to the idea of generalizing these too much. I think I understand his concerns and hope we can find a good tradeoff here. But that would be for another PR.

I'm surprised this doesn't fall under a more abstract category, like "IsTypeContext" or "IsPossibleTypeContext" (those are made-up, I'm not sure whether such methods exist). If there is not one, consider factoring one out, or filing an issue to do so.

We can consider these sorts of things. However, as i mentioned to @Neme12 it gets a little subtle due to different types being legal in different contexts.

One more thought on this. Is there a reason that all the recommenders for intrinsic types (bool, byte, string, object, etc) don't just share code?

Yes. Not all of them are allowed in the same contexts. For example, after stackalloc, you can have types, but not all types.

Similarly, you can have integral types (but not floating point types) in an enum-base-list.

--

Now, as i mentioned to @Neme12 we could certainly extract out some contexts that were well defined, and would make sense for large sets of these providers. For example: IsIntegralTypeContext (for non-floating point numeric types).

Thanks for the clarification. I certainly agree it doesn't need to be solved in this PR.

@Neme12 has shown a great interest in improving things here based on supporting all our existing goals, while also massively reducing duplication. I'm really excited to see what we can do here!

jcouv

LGTM Thanks

jcouv · 2018-03-19T18:28:18Z

@dotnet/roslyn-ide for review of this community PR. Thanks

jcouv · 2018-03-21T16:53:13Z

@dotnet/roslyn-ide Please review this community PR and merge. Thanks

Neme12 · 2018-03-21T17:01:24Z

retest windows_release_unit32_prtest please

Neme12 · 2018-03-21T17:40:35Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

Neme12 · 2018-03-21T19:39:03Z

the test shows 404 😕

HTTP ERROR 404
Problem accessing /job/dotnet_roslyn/job/master/job/windows_debug_vs-integration_prtest/10936/. Reason:

    Not Found
Powered by Jetty:// 9.4.z-SNAPSHOT

jcouv · 2018-03-22T21:37:22Z

@dotnet/roslyn-ide Please review this community PR and merge. Thanks

Neme12 · 2018-03-29T23:36:44Z

retest windows_debug_vs-integration_prtest please

Neme12 · 2018-03-29T23:36:57Z

retest windows_release_vs-integration_prtest please

jcouv · 2018-03-30T16:46:16Z

@dotnet/roslyn-ide Cyrus and I reviewed this community PR. Can you complete the review? It's been a week without response. Thanks

DustinCampbell

This is great work @Neme12! Thank you for all of the added tests.

DustinCampbell · 2018-04-05T12:03:23Z

@dotnet-bot retest this please

jcouv · 2018-04-05T16:46:10Z

@dotnet-bot retest this please

Neme12 · 2018-04-05T22:03:46Z

retest windows_release_unit64_prtest please

Neme12 · 2018-04-05T22:04:06Z

retest windows_debug_unit32_prtest please

jcouv · 2018-04-05T22:34:34Z

Whoever ends up merging this PR. Beware that it contains some merge commits (ie. do not squash this merge).

DustinCampbell · 2018-04-05T23:39:29Z

This is another great contribution @Neme12 -- thanks!

Neme12 added 13 commits March 17, 2018 20:00

Adding few missing tests

bbf1c69

Expression context in ref expression

f9c7cb3

Type context after ref & readonly

861ce5c

Type keyword recommenders (not var)

cacab0e

Completion for 'var'

25c1fb7

Adding more tests

f61b46b

Completion for 'readonly' after ref

d648fd6

Adding more tests to keyword recommenders

7e0b53c

Not offering 'dynamic' after ref expression

fd2bc73

Adding more tests for ref readonly

92d9d14

Adding tests with local functions to symbol completion

3fa7c10

Renaming & more thorough testing for both types & variables in symbol…

13473bb

… completion

Adding issue links

0c1d2da

Neme12 requested a review from a team as a code owner March 18, 2018 15:21

Neme12 commented Mar 18, 2018

View reviewed changes

Renaming keyword tests for consistency

c147f58

jcouv added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 18, 2018

jcouv added this to the 15.8 milestone Mar 18, 2018

Neme12 changed the title ~~Adding missng completion after 'ref'~~ Adding missing completion after 'ref' Mar 18, 2018

CyrusNajmabadi reviewed Mar 18, 2018

View reviewed changes

CyrusNajmabadi approved these changes Mar 18, 2018

View reviewed changes

Neme12 added 3 commits March 19, 2018 15:02

Simplifying helpers

dc123d0

Addressing Cyrus' nits

6f96042

Adding tests for other keywords

089955a

jcouv reviewed Mar 19, 2018

View reviewed changes

jcouv approved these changes Mar 19, 2018

View reviewed changes

jinujoseph assigned sharwell Mar 21, 2018

DustinCampbell approved these changes Apr 5, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into refCompletion

f0a5a63

DustinCampbell merged commit d27cfde into dotnet:master Apr 5, 2018

Neme12 deleted the refCompletion branch April 6, 2018 06:51

huoyaoyuan mentioned this pull request Oct 14, 2019

Keyword recommender doesn't know about struct's readonly members #39265

Closed

Conversation

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

Uh oh!

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?

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!

jcouv commented Mar 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 18, 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 18, 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!

CyrusNajmabadi commented Mar 18, 2018

Uh oh!

Neme12 commented Mar 18, 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!

Neme12 Mar 19, 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!

Neme12 Mar 19, 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 Mar 19, 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 Mar 19, 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 Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Neme12 commented Mar 18, 2018 •

edited

Loading

Neme12 Mar 18, 2018 •

edited

Loading

Neme12 Mar 18, 2018 •

edited

Loading

Neme12 commented Mar 18, 2018 •

edited

Loading

Neme12 Mar 19, 2018 •

edited

Loading

Neme12 Mar 19, 2018 •

edited

Loading

Neme12 Mar 19, 2018 •

edited

Loading

Neme12 Mar 19, 2018 •

edited

Loading

Neme12 Mar 19, 2018 •

edited

Loading

Neme12 commented Mar 21, 2018 •

edited

Loading

DustinCampbell left a comment •

edited

Loading