Dont suggest this in static local functions #35822

YairHalberstadt · 2019-05-20T20:16:43Z

The Problem

Currently Symbol.IsStatic is not very well defined. The compiler assumes it means any/all of the following:

A type is marked static,
A member is marked static,
A member does not require an instance receiver (with the exception of a non-static constructor - I will need to investigate more there)
A member cannot capture this
For some reason a namespace is considered static, but an assembly is not 🤷

In general this has worked till now because these definitions have mostly coincided, and when they haven't it has usually been in an understandable way (eg. constants are static even though they are not marked static).

Unfortunately Local Functions break that:

Static local functions are marked static, cannot capture this, and do not require an instance receiver.

Non-Static local functions are not marked static, can capture this, but do not require an instance receiver.

Currently all local functions are marked static, in order to make the compiler work with the fact that they do not require an instance receiver. This causes two problems:

It is non-intuitive that a non-static local functions IsStatic returns true. Since this is a public API, that's a problem.
We need an API to tell us if a local function is static or not. Indeed the ultimate purpose of this PR is to prevent this being suggested in local functions, which depends on such an API.

Solutions

We need to decide what IsStatic means:

Option 1.

Make non-static local functions IsStatic return false. Replace all usages of IsStatic where we are checking to see if a member Requires an instance receiver with a new property, RequiresInstanceReciever. This property can be internal, since it is only really of interest to the compiler.

advantages
IsStatic now matches our intuition much better.
We've now increased the explicitness of the compiler. Rather than mashing up lots of different concepts into one, we've begun seperating them out into different concepts.

disadvantages
This is risky. RequiresInstanceReciever now behaves exactly like IsStatic used to, but IsStatic behaves slightly differently. If we forget to replace all relevant usages of IsStatic with RequiresInstanceReciever, we may introduce subtle bugs that only occur with non-static local functions.

Option 2.

We currently have a temporary internal API IsStaticLocalfunction. Make this public and move it to IMethodSymbol. Document what IsStatic means.

advantages
Very low risk. Simple to do.

disadvantages
IsStatic now doesn't match our intuitions.
Rather than solving the problem, we've worked around it. We've just increased the technical debt of the codebase, rather than decreasing it.

Approach taken here

The ideal solution is obviously the first, which is what I've done, so that the risk can be properly evaluated. I've created a pr for the second solution at #35825 so they can be compared.

Changing a call from IsStatic to RequiresInstanceReciever is always safe, since the latter behaves like the former used to.

Leaving a call as a call to IsStatic is dangerous, as IsStatic now has different behaviour.

For fields/events/properties IsStatic and RequiresInstanceReciever are synonyms.

I've gone through every usage of IsStatic in the compiler code (not tests/workspaces/IDE) and if it appears to be about receivers, and is not explicitly talking about a field property or event, I have replaced it with a call to RequiresInstanceReciever.

This requires changing 27 files.

Depending on the opinion of the roslyn team, I am happy to either take the alternative solution, do more work here, or drop this altogether.

CyrusNajmabadi · 2019-05-20T20:49:53Z

tagging @dotnet/roslyn-compiler @jcouv . Note: this PR should be reviewed commit by commit as it shows first the safe mechanical translation.

333fred · 2019-05-20T21:09:50Z

I'm going to copy some of the internal discussion we had around this last time I made a PR in this space (lightly pruned for irrelevant things). We did not come to any conclusions about this at that point, and my PR languished until I closed it as it didn't seem to matter. This provides a reasonable use case, and I'm in favor of solution 1.

@agocke said:

It’s not entirely clear what meaning we’re talking about with IsStatic. On the one hand, you could say that the methods are static if they’re emitted as static. This seems wrong, because it’s meaningless at the language level. You could also see static as meaning “invoked without a receiver, implicit or otherwise”. By this meaning, all local functions are static (this is the meaning it has today). Or you could say, “does it have the static keyword?” That’s the proposed change.
I think we should consider what we want the contract of IsStatic to be, and decide what to do for local functions from that.

@DustinCampbell said:

At the source level, I believe that "IsStatic" is intended to reflect what's in source. At the metadata level, well, I guess local functions wouldn't appear as local functions, would they?
Or, is that not true and "IsStatic" maps differently for lambda symbols depending on whether it is compiled as an instance or static method?

@gafter said:

The symbol API IsStatic does not depend on the translation strategy. I agree it should reflect the language sense of “static” once we define what that means.

@333fred said:

From an API naming standpoint, I feel like it would be wrong for it not to reflect the semantics imposed by the static keyword. For regular members, this means it will be emitted as a static member, doesn’t have a receiver, and cannot access instance members on that type. For local functions, this will mean it’s guaranteed to be emitted as a static member, cannot capture anything from the containing function (including this), and any other semantics I’m missing. For regular local functions, we may indeed emit them without a receiver as a static member of the class, or we might emit them as instance members, or we may do some other crazy thing. We specifically do not make any guarantees about this for the reason of being able to change it later as we so choose.
I’ve implemented this strategy here: #30832.

@agocke said:

I can follow along this logic, but the PR looks like ISymbol.IsStatic will return the old behavior (always true). How are we rationalizing the difference between the public and private API? Why should our private API return something different from our public API?
If the argument is that this is a breaking change because it’s the right thing to do, shouldn’t we “put our money where our mouth is” and change the behavior in the internal compiler implementation as well?

@333fred said:

ISymbol.IsStatic will return the old behavior (always true)

No. ISymbol.IsStatic will never return true with this PR. When I add support for the static keyword, then the public ISymbol.IsStatic will start returning true for those functions. The rationalization for the difference is pretty simple: our private Symbol.IsStatic is the implementation detail of how the local function will be emitted. The public IsStatic is not privy to this implementation detail.

@agocke said:

Sorry, got it reversed – the internal one is the one that’s always true.
The problem is

our private Symbol.IsStatic is the implementation detail of how the local function will be emitted

This is not true. LocalFunctionSymbol is our binder-level symbol that has no control over the emit. For that we generate a new symbol, SynthesizedClosureMethod, which is static depending on the analysis we did in lowering.
LocalFunctionSymbol always returns true because we semantically always treat local function invocations as static invocations, i.e. they never have a receiver. This is a semantic level difference that we’re hiding from the public API.

@agocke said:

Thinking about this more, we already have special overload resolution rules for local functions (they shadow and aren’t permitted to have overloads), but is there a natural way to slot this in for overload resolution. Overload resolution recently had a change to filter candidates based on whether or not they were static, right @gafter? Does it feel better to make a decision about local functions for this context? Or spec it separately and stay silent about the static-ness of a local function?

@gafter said:

The change to overload resolution included only static members when the method group is qualified by a type name, and only instance members when the group is qualified by a value. None of this applies to local functions because you cannot invoke them in any qualified way. I think we can give “IsStatic” a special language-defined meaning (as we are about to do) specific to local functions (just like give IsStatic a special meaning for classes) and leave it at that.

CyrusNajmabadi · 2019-05-20T21:12:24Z

Note: i agree with this:

I think we can give “IsStatic” a special language-defined meaning (as we are about to do) specific to local functions (just like give IsStatic a special meaning for classes) and leave it at that.

It seems like it would be very bizarre to have the language concept of "local functions" and "static local functions" and then not have IsStatic just be the natural way to distinguish those.

jasonmalinowski · 2019-05-21T16:45:00Z

@YairHalberstadt: what did "make ISymbol.IsStatic and don't add a new member" ultimately look like? Did IDE tests break or did things still work?

YairHalberstadt · 2019-05-21T16:50:05Z

what did "make ISymbol.IsStatic and don't add a new member" ultimately look like?

I'm not sure what you're referring to by this?

jasonmalinowski · 2019-05-21T16:52:58Z

@YairHalberstadt Oh nevermind that's effectively this PR. I saw this adding stuff to Symbol and read that as ISymbol. Nevermind. Time to find some caffine.

…nReadOnlyMember

cston · 2019-05-21T19:32:20Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

+        /// <summary>
+        /// Returns true if this symbol requires an instance reference as the implicit reciever. This is false if the symbol is static, or a <see cref="LocalFunctionSymbol"/>
+        /// </summary>
+        public virtual bool RequiresInstanceReciever => !IsStatic;


Reciever [](start = 44, length = 8)

Typo: Receiver

I always get my "ei"s and "ie"s mixed up :-)

Anyway, fixed now.

YairHalberstadt · 2019-05-21T19:33:07Z

I've fixed the broken test.

The issue was a place where I thought on an initial look that it wouldn't be necessary to convert IsStatic to RequiresInstanceReciever, but it actually was.

I think this just highlights - I will have to go through all usages of IsStatic by the compiler with a fine tooth comb in order to avoid any possible bugs, if the compiler team chooses to take this approach. I've already gone through it once, but it's possible I missed other cases as well.

On the other hand, a large percentage can be ruled out as irrelevant at first sight. I'd estimate about 50 - 100 references have to be checked properly.

333fred · 2019-05-21T20:50:19Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

+        /// <summary>
+        /// Returns true if this symbol requires an instance reference as the implicit reciever. This is false if the symbol is static, or a <see cref="LocalFunctionSymbol"/>
+        /// </summary>
+        public virtual bool RequiresInstanceReceiver => !IsStatic;


It feels like this is probably the wrong implementation approach. I do think that the public ISymbol.IsStatic should reflect what this PR achieves, but there's a lot of internal code that depends on the implementation of IsStatic. A less risky approach would be to explicitly implement the public version of IsStatic separately from the internal Symbol.IsStatic, and doc what the difference is.

In the discussion you quoted, I think @agocke argued that:

I can follow along this logic, but the PR looks like ISymbol.IsStatic will return the old behavior (always true). How are we rationalizing the difference between the public and private API? Why should our private API return something different from our public API?
If the argument is that this is a breaking change because it’s the right thing to do, shouldn’t we “put our money where our mouth is” and change the behavior in the internal compiler implementation as well?

It doesn't look like that was ever resolved.

I'm happy to go either way once a decision is madr

@agocke is oof for the next couple of weeks, so @gafter would you care to weigh in? I think the real question here is whether we should be pragmatic and attempt to change the compiler implementation as little as possible, or whether we should attempt to adapt our use cases and potentially miss some?

I must admit I'd be a little worried about:

A less risky approach would be to explicitly implement the public version of IsStatic separately from the internal Symbol.IsStatic, and doc what the difference is.

It does reduce risk now, but I'd then be worried if our public API was getting the same attention of the internal code. Or also if the compiler at one point would accidentally call the ISymbol implementation and get the "wrong" behavior.

@333fred @agocke @gafter Which approach do we want to take here? I think @YairHalberstadt is blocked on it at this point and we can address the lack of tests or other concerns once we sort that out.

I haven't reviewed the PR in full yet, but I like the approach @YairHalberstadt is taking here.

My primary concern with the whole thing is that we have a very loose contract on what "IsStatic" actually means, but then we used IsStatic in a lot of the code to make very specific language determinations.

From my perspective, the right approach is to pull out a new API, like @YairHalberstadt has done with RequiresInstanceReceiver, give that a strictly defined meaning, and then use that to make the appropriate language decisions.

Given this path I would actually argue that in a perfect world we would remove ISymbol.IsStatic since I can not think of a description in a doc comment that would be useful to a user, but clearly that ship has already sailed.

333fred · 2019-05-21T20:51:40Z

I don't see any test changes here? I would expect to see some compiler tests added to cover the the return values of the public API. Apparently we might have a test gap here.

Done review pass (commit 4)

jasonmalinowski · 2019-06-10T18:09:21Z

@YairHalberstadt I do see you added additional tests and if @agocke is liking the approach, then is this on us to get this reviewed again?

jasonmalinowski · 2019-06-17T17:36:01Z

@YairHalberstadt Anything you need from us?

YairHalberstadt · 2019-06-17T18:09:26Z

@jasonmalinowski

Sorry - I didn't see you were asking me something in the previous message.

No - this is good to review.

Thanks

jasonmalinowski · 2019-06-17T18:14:33Z

@dotnet/roslyn-compiler: can we get a few reviews of this community PR then? Please note the conversation above regarding the design decision there. I think I'm willing to say the compiler goes first here and once signed off the IDE will review and then merge. This is a case where the IDE really doesn't have any big skin in the game here -- it's all about the API you all get to support and maintain.

agocke

Sorry, one thing -- I agree that having RequiresInstanceReceiver on Symbol itself is pretty shaky. Could we put it on the deriving symbols instead and do a type check at the appropriate points?

It feels like we're reproducing the earlier mistake of making the API so broad it doesn't mean anything

RikkiGibson · 2019-06-21T20:52:52Z

By my reckoning that means:

MethodSymbol
PropertySymbol
EventSymbol
FieldSymbol

Please chime in if I missed one :)

YairHalberstadt · 2019-06-25T07:10:10Z

Requested changes made

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

RikkiGibson

LGTM

agocke

LGTM

agocke · 2019-06-25T21:58:06Z

@jasonmalinowski I'll leave it to you to sign off on the IDE side

jasonmalinowski

IDE change is fine other than yes, we want braces even on trivial ifs. ;-)

The one thing I don't quite get though -- so the compiler now has a new API internally, but we're not making that public and instead continuing to make IsStatic public? It seems odd if we're saying "we can't rationalize what IsStatic means...so we'll just change it to whatever it needs to be"?

jasonmalinowski · 2019-06-25T22:07:06Z

Also @agocke and @RikkiGibson is this something you want in 16.2 or 16.3? The IDE changes are obviously trivial and if this was an IDE-only fix we'd take in 16.2. Since it's churning your stuff there's extra risk so I'm happy to defer to you.

agocke · 2019-06-25T23:51:47Z

@jasonmalinowski Well, I think the new API can be internal for now, and public if it becomes necessary for other people, same as our standard API process. As far as target, let's wait until the snap tomorrow to merge, since it's < 24 hours away

Add braces around trivial if in SyntaxExtensions

jasonmalinowski · 2019-06-26T17:28:51Z

Alright, I will merge this later today post snap, which pushes this into shipping in 16.3.

jasonmalinowski · 2019-06-26T21:06:02Z

Thanks @YairHalberstadt for the awesome contribution, especially for one that always has the fun mix of design issues and cross compiler/IDE work!

* updated to Roslyn 3.3.0-beta1-19365-07 * support sync namespace refactoring * do not run into null reference * handle newly created files * better relative path handling * small fixes * improve tests * added SyncNamespaceFacts * fixed test and added more resiliency * added support for live changing of root namespace * added more tests - for live reload of a namespace * code review feedback * roslyn 3.3.0-beta2-19376-02 * reflection fix due to a method change from public to internal in SymbolKey * fixed code action tests * address changes to dotnet/roslyn#35822 * fixed cake test

…ways return true for all local functions, but now with C# 8 static local functions feature, it only returns true for local functions declared with a static modifier. See dotnet/roslyn#35822 for details.

OQO · 2019-10-30T05:20:50Z

Could you please check #39565? The regression seems to be related to these changes.

An exception is incorrectly thrown thinking that it requires an instance receiver.

Thanks for your help.

YairHalberstadt added 2 commits May 20, 2019 20:40

Distinguish between RequiresInstanceReciever and IsStatic

393f18b

Don't recommend 'this' in static local functions

4619f1d

YairHalberstadt requested review from a team as code owners May 20, 2019 20:16

YairHalberstadt mentioned this pull request May 20, 2019

don't suggest this in a static local function alternative #35825

Closed

RikkiGibson added Area-Compilers Area-IDE labels May 20, 2019

jasonmalinowski self-assigned this May 21, 2019

Change IsStatic to RequiresInstanceReciever in CheckImplicitThisCopyI…

0006aea

…nReadOnlyMember

cston reviewed May 21, 2019

View reviewed changes

Rename RequiresInstanceReciever to RequiresInstanceReceiver

7076d92

333fred reviewed May 21, 2019

View reviewed changes

YairHalberstadt added 2 commits May 22, 2019 08:52

Add tests For LocalFunctionSymbol.ISymbol.IsStatic

966ed74

merged from master

9e2417b

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 14, 2019

jcouv assigned agocke Jun 18, 2019

agocke requested changes Jun 21, 2019

View reviewed changes

YairHalberstadt added 3 commits June 24, 2019 21:03

Move RequiresInstanceReceiver out of Symbol and to derived symbols

053a954

Add tests for requires instance receiver.

c437ee8

fix EventTests.InstanceEventRequiresInstanceReceiver

e27c37d

RikkiGibson reviewed Jun 25, 2019

View reviewed changes

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/SyntaxTreeExtensions.cs Show resolved Hide resolved

RikkiGibson approved these changes Jun 25, 2019

View reviewed changes

RikkiGibson requested a review from agocke June 25, 2019 19:40

agocke approved these changes Jun 25, 2019

View reviewed changes

jasonmalinowski approved these changes Jun 25, 2019

View reviewed changes

Update SyntaxTreeExtensions.cs

c3d2d42

Add braces around trivial if in SyntaxExtensions

jcouv added this to the 16.3 milestone Jun 26, 2019

jasonmalinowski merged commit a571221 into dotnet:master Jun 26, 2019

filipw added a commit to filipw/omnisharp-roslyn that referenced this pull request Jul 31, 2019

address changes to dotnet/roslyn#35822

a550617

svick mentioned this pull request Aug 19, 2019

Local functions not available in intellisense/autocomplete #38074

Closed

sharwell mentioned this pull request Sep 19, 2019

SA1101 mis-fires on generic local method in .NET Core 3 Preview DotNetAnalyzers/StyleCopAnalyzers#3018

Closed

OQO mentioned this pull request Oct 30, 2019

CreateDelegate throws exception on calling function with "out var x" (Regression in Microsoft.CodeAnalysis.CSharp.Scripting 3.3.1)) #39565

Closed

CyrusNajmabadi mentioned this pull request Mar 9, 2020

this keyword should not be recommended within a static local function #35644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont suggest this in static local functions #35822

Dont suggest this in static local functions #35822

YairHalberstadt commented May 20, 2019 •

edited

Loading

CyrusNajmabadi commented May 20, 2019

333fred commented May 20, 2019

CyrusNajmabadi commented May 20, 2019

jasonmalinowski commented May 21, 2019

YairHalberstadt commented May 21, 2019

jasonmalinowski commented May 21, 2019

cston May 21, 2019

YairHalberstadt May 21, 2019

YairHalberstadt commented May 21, 2019

333fred May 21, 2019

YairHalberstadt May 21, 2019

333fred May 22, 2019

jasonmalinowski May 24, 2019

jasonmalinowski Jun 3, 2019

agocke Jun 5, 2019

333fred commented May 21, 2019 •

edited

Loading

jasonmalinowski commented Jun 10, 2019

jasonmalinowski commented Jun 17, 2019

YairHalberstadt commented Jun 17, 2019

jasonmalinowski commented Jun 17, 2019

agocke left a comment •

edited

Loading

RikkiGibson commented Jun 21, 2019

YairHalberstadt commented Jun 25, 2019

RikkiGibson left a comment

agocke left a comment

agocke commented Jun 25, 2019

jasonmalinowski left a comment

jasonmalinowski commented Jun 25, 2019

agocke commented Jun 25, 2019

jasonmalinowski commented Jun 26, 2019

jasonmalinowski commented Jun 26, 2019

OQO commented Oct 30, 2019

Dont suggest this in static local functions #35822

Dont suggest this in static local functions #35822

Conversation

YairHalberstadt commented May 20, 2019 • edited Loading

The Problem

Solutions

Option 1.

Option 2.

Approach taken here

CyrusNajmabadi commented May 20, 2019

333fred commented May 20, 2019

CyrusNajmabadi commented May 20, 2019

jasonmalinowski commented May 21, 2019

YairHalberstadt commented May 21, 2019

jasonmalinowski commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YairHalberstadt commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented May 21, 2019 • edited Loading

jasonmalinowski commented Jun 10, 2019

jasonmalinowski commented Jun 17, 2019

YairHalberstadt commented Jun 17, 2019

jasonmalinowski commented Jun 17, 2019

agocke left a comment • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented Jun 21, 2019

YairHalberstadt commented Jun 25, 2019

RikkiGibson left a comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

agocke commented Jun 25, 2019

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski commented Jun 25, 2019

agocke commented Jun 25, 2019

jasonmalinowski commented Jun 26, 2019

jasonmalinowski commented Jun 26, 2019

OQO commented Oct 30, 2019

YairHalberstadt commented May 20, 2019 •

edited

Loading

333fred commented May 21, 2019 •

edited

Loading

agocke left a comment •

edited

Loading