Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommend 'this' inside local functions #27690

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Recommend 'this' inside local functions #27690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions src/EditorFeatures/CSharpTest2/Recommendations/ThisKeywordRecommenderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,79 @@ await VerifyKeywordAsync(
static int Goo($$");
}

[Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
[WorkItem(27028, "https://github.com/dotnet/roslyn/issues/27028")]
public async Task TestInLocalFunction()
{
await VerifyKeywordAsync(
@"class C
{
int Method()
{
void local()
{
$$
}
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
[WorkItem(27028, "https://github.com/dotnet/roslyn/issues/27028")]
public async Task TestInNestedLocalFunction()
{
await VerifyKeywordAsync(
@"class C
{
int Method()
{
void local()
{
void nested()
{
$$
}
}
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
[WorkItem(27028, "https://github.com/dotnet/roslyn/issues/27028")]
public async Task TestInLocalFunctionInStaticMethod()
{
await VerifyAbsenceAsync(
@"class C {
static int Method()
{
void local()
{
$$
}
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
[WorkItem(27028, "https://github.com/dotnet/roslyn/issues/27028")]
public async Task TestInNestedLocalFunctionInStaticMethod()
{
await VerifyAbsenceAsync(
@"class C
{
static int Method()
{
void local()
{
void nested()
{
$$
}
}
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
public async Task TestAfterAttribute()
{
Expand Down
7 changes: 7 additions & 0 deletions src/Workspaces/CSharp/Portable/Extensions/ContextQuery/SyntaxTreeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1771,6 +1771,13 @@ public static bool IsInstanceContext(this SyntaxTree syntaxTree, SyntaxToken tar
#endif

var enclosingSymbol = semanticModel.GetEnclosingSymbol(targetToken.SpanStart, cancellationToken);

while (enclosingSymbol is IMethodSymbol method && method.MethodKind == MethodKind.LocalFunction)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about MethodKind.AnonymousFunction?

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems this is always offered inside them regardless of whether the containing method is static

{
// It is allowed to reference the instance (`this`) within a local function, as long as the containing method allows it
enclosingSymbol = method.ContainingSymbol;
Copy link
Member

Choose a reason for hiding this comment

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

i find this fix surprising. Is a local function treated as static (even though it can use 'this')?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging @agocke @gafter to confirm expected semantic model behavior on local functions (they currently have IsStatic = true even if the enclosing method isn't static).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds wrong to me. If they're emitted as static methods, that's just a codegen decision.

Copy link
Member Author

Choose a reason for hiding this comment

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

From discussion with @agocke and @AlekseyTs, there are two bugs:

  • the compiler LocalFunctionSymbol.IsStatic should always return false, but currently always returns true.
  • given that, the IDE should not consider LocalFunctionSymbol.IsStatic to decide whether this can be used in the current context.

Given that, I will revert this PR to my original fix.

Copy link
Member

Choose a reason for hiding this comment

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

the compiler LocalFunctionSymbol.IsStatic should always return false, but currently always returns true.

Not disagreeing... but can an explanation be provided as to why?

This seems similar to props+accessors. Accessors can't have static/non-static info on them. They just 'inherit' from the containing prop. Seems similar to local-functions/containers. And it would seem to make the most sense. It would also tie into one of the main concepts of Static/Instance (namely, whether or not you can use 'this'..). Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like there is some discussion going on for the compiler side (maybe it should always be true after all) ;-)

}

return !enclosingSymbol.IsStatic;
}

Expand Down