Skip to content

Recommend keyword static for static local functions#32187

Merged
jcouv merged 2 commits intodotnet:dev16.0-preview2from
jcouv:recommend-static
Jan 10, 2019
Merged

Recommend keyword static for static local functions#32187
jcouv merged 2 commits intodotnet:dev16.0-preview2from
jcouv:recommend-static

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Jan 5, 2019

Customer scenario
User types a static local function (C# 8.0 beta).
Completion gets in the way of typing static, by auto-completing to System.ContextStaticAttribute.

Bugs this fixes
Fixes #32174

Workarounds, if any
You can dismiss the incorrect completion, but more often than not, you'll need to delete System.ContextStaticAttribute and type static and dismiss the incorrect completion.

Risk
Performance impact
Low. This is a one-line fix in the recommender for static keyword.

Is this a regression from a previous update?
No, this is the result of a feature introduced in preview2.

Root cause analysis
New feature requires some adjustments to tooling.

How was the bug found?
Ad-hoc IDE validation as part of compiler test plan for new language features.


Filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/763373 for shiproom purpose


@dotnet/roslyn-ide for review. Thanks

CC @cston
Relates to #32069

image

@jcouv jcouv added the Area-IDE label Jan 5, 2019
@jcouv jcouv added this to the 16.0.P2 milestone Jan 5, 2019
@jcouv jcouv self-assigned this Jan 5, 2019
@jcouv jcouv requested a review from a team as a code owner January 5, 2019 01:13
@jaredpar jaredpar mentioned this pull request Jan 5, 2019
70 tasks
@jcouv
Copy link
Member Author

jcouv commented Jan 5, 2019

Thanks!

@jinujoseph Any chance to get this in for preview2? Without this completion gets in the way of typing static local functions.

@jinujoseph

This comment has been minimized.

@JoeRobich JoeRobich changed the base branch from master to dev16.0-preview2 January 5, 2019 02:15
@jinujoseph

This comment has been minimized.

@jcouv

This comment has been minimized.

@jcouv jcouv closed this Jan 6, 2019
@jcouv jcouv reopened this Jan 6, 2019
@jinujoseph
Copy link
Contributor

@jcouv oops , yes thatz what i wanted to say , but coped the wrong branch name, thanks for correcting.
Marking this for shiproom approval.

public async Task TestNotBetweenUsings()
{
await VerifyAbsenceAsync(AddInsideMethod(
await VerifyKeywordAsync(AddInsideMethod(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Why did this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should expect the static keyword to show here since it's inside a method. That said, test name needs to be changed too.

Copy link
Contributor

@sharwell sharwell Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain the test was not originally intended to test behavior inside of methods (this is not a valid location for using directives, and there is no equivalent test for cases outside of methods where the directives are allowed). I would prefer to fix the test so it's testing the behavior of using directives in the correct location rather than what we have here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, 👍

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good, and I support @sharwell's block that we should fix that test. If GitHub had a button for "have the same status as @sharwell" I'd use that here, but I'll hit approve so it doesn't require me to revisit once @sharwell approves.

else
{
var result = (await RecommendKeywordsAsync(position, context)).Single();
var result = (await RecommendKeywordsAsync(position, context)).SingleOrDefault();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 this gives us the benefit of the assertion message on next line.

@jcouv
Copy link
Member Author

jcouv commented Jan 7, 2019

Updated test. It uncovered existing bug with static recommend in scripting context. Filed #32214

@jcouv
Copy link
Member Author

jcouv commented Jan 7, 2019

Thanks

@jinujoseph Let me know if there is anything else I need to do to get approval. Otherwise, I assume you'll drive the process. Thanks

@jinujoseph
Copy link
Contributor

yes will do once our current payload is inserted

@jinujoseph
Copy link
Contributor

@jcouv can you pls fill in the ask mode template pls

@jcouv
Copy link
Member Author

jcouv commented Jan 9, 2019

@jinujoseph Do we have an example of the template to use at this point?

@sharwell
Copy link
Contributor

sharwell commented Jan 9, 2019

@jcouv Your first post has already been edited to show the template headings

@jcouv
Copy link
Member Author

jcouv commented Jan 9, 2019

Ah, thanks!
Filled in the OP template.

@jinujoseph
Copy link
Contributor

@jcouv good to merge

@jcouv jcouv merged commit 8fa41de into dotnet:dev16.0-preview2 Jan 10, 2019
@jcouv jcouv deleted the recommend-static branch January 10, 2019 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants