No suggestion to add null check for inner function

[CyrusNajmabadi]

Ported from https://devdiv.visualstudio.com/DevDiv/_workitems/edit/466136

Paste: static void Foo(Stuff outer) { int Bar(Stuff inner) { return 0; } Bar(outer); } Click on outer. There is an offer to add a null check. Click on inner. There is no such offer.

[CyrusNajmabadi]

This is pretty low priority. The purpose of 'add null check' is to actually guard your entrypoints for your externally facing API. Local functions are only being used by your own code, so there's far less need for this.

That said, i would not be opposed to someone picking this up and just adding this feature.

[Neme12]

I couldn't find any interface that would abstract over BaseMethodDeclarationSyntax and LocalFunctionStatementSyntax. They have a lot of things in common, I'm thinking there should probably be something like IFunctionDeclarationSyntax. Is there a way at all to write code that would work with both of them without duplication?

[Neme12]

Should this also include lambdas? 😄

[CyrusNajmabadi]

I couldn't find any interface that would abstract over BaseMethodDeclarationSyntax and LocalFunctionStatementSyntax. They have a lot of things in common, I'm thinking there should probably be something like IFunctionDeclarationSyntax. Is there a way at all to write code that would work with both of them without duplication?

You can break teh method and local function into the parts you need to care about and then operate on those distinct pieces. Often we use a (possibly generic) base class for this sort of common logic, and then have specific subclasses for the individual derivations.

[Neme12]

@CyrusNajmabadi Yes, I already did that, thank you.

I stumbled on a bit of an issue though. CSharpSyntaxGenerator doesn't know anything about local functions. I'm pretty sure I only need to change CSharpSyntaxGenerator.WithStatements to add a case for LocalFunctionStatement (I would also do GetStatements for symmetry) but it should probably be supported throughout the class in other places such as GetParameterList.

I tried changing what I need but I can't get those changes to take effect when I run or debug from VisualStudioSetup... is debugging the CSharpWorkspace project supported? It always says the source code is different from what's running when I step into there.

[CyrusNajmabadi]

I'm pretty sure I only need to change CSharpSyntaxGenerator.WithStatements to add a case for LocalFunctionStatement (I would also do GetStatements for symmetry) but it should probably be supported throughout the class in other places such as GetParameterList.

Yup. That sounds right. Looks like something we missed as we haven't had a need for it until now.

Tagging @dpoeschl . David, we should probably update our test-pass matrix so that new language features are tested against SyntaxGenerator. This way we update that, along with teh compiler APIs.

I tried changing what I need but I can't get those changes to take effect when I run or debug from VisualStudioSetup... is debugging the CSharpFeatures project supported

@dotnet/roslyn-ide Can someone help @Neme12 out with debugging? F5 VisualStudioSetup (or maybe you need VisualStudioSetup.next?) should work.

[Neme12]

Sorry, I misspoke. I can debug Features & CSharpFeatures but when I step into CSharpWorkspace it says the source is different and when I hit a breakpoint anyway it's clearly on the wrong line as if that code never compiled. Doesn't work with VisualStudioSetup or VisualStudioSetup.Next.

[CyrusNajmabadi]

Yup. That should all work. Maybe try a rebuild to force that library to be rebuilt and picked up. Note: you shouldn't have to do this. But sometimes things break for random reason. This might get you back into a good state.

[Neme12]

I've been trying various things, builds and rebuilds for the last 3 hours without any success.

EDIT: Sorry for the confusion and for bothering you. I've set my debugging to launch 15.5 experimental from 15.6 preview but that doesn't work apparently. I thought it did but the extensions must not be getting updated. I can't try it in 15.6 experimental either because that thing keeps throwing exceptions, so I guess I have to try to work with 15.5.

[CyrusNajmabadi]

Yup, no clue. Someone from @dotnet/roslyn-ide will have to help out here.

[Neme12]

Serious question. First I thought it would be unnecessary for lambdas but from I've read about VB, the reasoning as to why they don't have local functions is that you don't really need them, you would use lambdas in those scenarious because they aren't as limited as C# lambdas and are also optimized. So I'm wondering if it isn't a little unfair that C# would have this for local functions but VB is out of luck.

It does seem like it would be useless for lambdas but as you said, you wouldn't think people would need this for local functions either, but apparently some people do, so I guess who are we to judge? Also, the code already has to lose the appeal/"type safety" anyway of only dealing with one syntax like BaseMethodDeclarationSyntax and has to be generalized to use SyntaxNode, so I think it wouldn't add much more complexity to support anonymous functions as well (except for complexity in the tests). Can people be bothered by being offered more refactorings than they need?

[CyrusNajmabadi]

I never said local functions/lambdas should not have this refactoring offered. Just that it's low priority as it's less common for these entities to need them. For example, in Roslyn itself i can't recall seeing a single case where a null check was contained in a lambda/inner-function.

[CyrusNajmabadi]

Can people be bothered by being offered more refactorings than they need?

Yes definitely. We actually made many concerted efforts to trim things down over VS releases because the set of refactorings/fixes was getting too large and becoming too difficult for people to use effectively.

[Neme12]

@CyrusNajmabadi
Sorry for the misunderstanding, I was talking specifically about lambdas/anonymous functions as opposed to local functions. I'm not sure if the proposal implies that too.

You didn't say they shouldn't but the proposal doesn't ask for that specifically so I wanted to ask you for your opinion on that, do you think it should be implemented for lambdas as well if it's implemented for local functions?

[CyrusNajmabadi]

This is pretty low priority. The purpose of 'add null check' is to actually guard your entrypoints for your externally facing API. Lambdas are generally are only being used by your own code, so there's far less need for this.
That said, i would not be opposed to someone picking this up and just adding this feature.

:)

[CyrusNajmabadi]

Basically: i don't care. i wouldn't go and proactively do it myself. But i have no issue with someone else wanting to do it :)

[Neme12]

Yes definitely. We actually made many concerted efforts to trim things down over VS releases because the set of refactorings/fixes was getting too large and becoming too difficult for people to use effectively.

I see. I didn't know that was a concern. I know that when I install some analyzers, many of them can be annoying but I'm never offended by extra on-demand refactorings. I generally only miss those when I need them and wish there were more. I would expect there to be more in-box in an IDE like VS (but yeah, when I install some library that offers many "refactorings" like remove parameter, remove this piece of code and generally break your code for no particular reason, I'm pretty quick to uninstall that).

[CyrusNajmabadi]

Quantity is not better than quality :)

We could easily ship 100+ less useful refactorings. But the experience around using those refactorings would degrade heavily because of how much irrelevant stuff we were foisting on the user.

Instead, we conscientiously add refactorings/fixes that we think will add significant value without being too much of an experience pain for users. Telemetry helps us out here as we can add things, but hten remove them if we find that there's practically no usage of it in our customer base.

[Neme12]

It would also be helpful if CSharpSyntaxGenerator.GetDeclarationKind supported local functions, but somone needs to decide whether that should return Method or there should be a new DeclarationKind. It doesn't recognize anonymous methods either although it does recognize lambdas... This class really feels incomplete.

[CyrusNajmabadi]

Lambdas and Anonymous delegates aren't considered declarations. You can observe that by trying to call SemanticModel.GetDeclaredSymbol on one of these.

It would also be helpful if CSharpSyntaxGenerator.GetDeclarationKind supported local functions

Sure. Feel free to do that :)

somone needs to decide whether that should return Method or there should be a new DeclarationKind.

It should be a new declaration kind. They're not methods, they're local functions.

This class really feels incomplete.

Feel free to submit PRs to flesh it out :)

[Neme12]

You can observe that by trying to call SemanticModel.GetDeclaredSymbol on one of these

I ran into that too. But why not? It seems like it should be possible to get a symbol from a LambdaExpressionSyntax. What I had to do is get a parameter symbol and then its containg symbol, which feels like a workaround. I understand they're not considered declarations in the syntax, but semantically they sort of are because they declare a symbol. Lambdas are treated as declarations in SyntaxGenerator.

It should be a new declaration kind. They're not methods, they're local functions.

What about anonymous methods? Should they be a new declaration kind or use LambdaExpression? I feel like if instead of DeclarationKind.LambdaExpression there was LambdaExpression.AnonymousFunction it would be good to use for both but since it's called LambdaExpression, it should probably only mean lambdas?

[CyrusNajmabadi]

I ran into that too. But why not?

Because it's not a declaration of a symbol :) It's an expression that gets converted into a Delegate.

What about anonymous methods?

Neither should be Declarations at all. I'm not sure why you need a DeclarationKind for them.

[CyrusNajmabadi]

It seems like it should be possible to get a symbol from a LambdaExpressionSyntax.

You can. Just not using GetDeclaredSymbol. A lambda is not a declaration for a symbol, same as "new Foo()" is not hte declaration of the Foo type. There is the point where the symbol is declared. i.e.:

class C {} or
delegate void Action()

And then there are expressions that create values that have these symbols as their types. So new C() is not a declaration of C. It's an object creation expression that creates an instance of that type. Similarly, with Action a = () => {}, () => {} is not a declaration of a symbol. It's an expression that creates an instance of the Action type.

[Neme12]

Because it's not a declaration of a symbol :) It's an expression that gets converted into a Delegate.

But I can get an IMethodSymbol from them from the parameter, what is that symbol's declaration then?

Neither should be Declarations at all. I'm not sure why you need a DeclarationKind for them.

Ok I don't know if they should, you know better than me, I just noticed that lambdas are there and thought that if lambdas are there, anonymous methods should be there too. I don't need it, but I thought it might reduce code duplication (since the syntax generator would take care of C# & VB differences).

[Neme12]

I understand it now, sorry for my ignorance.

[CyrusNajmabadi]

But I can get an IMethodSymbol from them from the parameter

The parameter is fresh. If you have (int x) => { ... } then nowhere else is that 'x' declared. For example, the Delegate may actually be delegate void Foo(int y);. The 'y' there is one parameter (which actually can be referenced if you do:

delegate void Foo(int y);
Foo foo = (int x) => { /* use 'x' in here */ };
foo(y: 0); // reference y here.

[CyrusNajmabadi]

Ok I understand now, sorry for my ignorance.

Nothing to be sorry about! Here's an important thing to know: the Roslyn compiler APIs are designed to be accurate and to properly reflect how the compiler and language work. That means a lot of subtlety and complexity that people generally don't have to think about when using the language. But, when exposing compiler guts, and wanting to be able to write really precise features that use them properly, you end up having this stuff foisted on you.

[Neme12]

@CyrusNajmabadi
What about lambdas being considered declarations in the SyntaxGenerator class then? Apart from DeclarationKind, it refers to them as declaration in parameter names all over the place.

[CyrusNajmabadi]

Now, importantly, that's the APIs in Microsoft.CodeAnalysis.CSharp/VisualBasic. The code in Microsoft.CodeAnalysis.Workspaces and above is not under the same constraints. The code in there (like SyntaxGenerator and whatnot) are intended to just smooth things out a bit and make it easier to write features. And SyntaxGenerator is especially lossy because it's intended to smooth out feature coding against both C# and VB (and thus tries to be a common-denominator API). That means it's great for whipping up features that behave roughly the same for both language. But it can mean you run into things it isn't great at the more complex and language-specific you get.

[CyrusNajmabadi]

What about lambdas being considered declarations in the SyntaxGenerator class then?

Sorry, it's still not clear to me why you would need to do this. Let's step back: what are you trying to accomplish, and why is SyntaxGenerator not working for you? Maybe from that we can come up with a more suitable solution.

Thanks!

[Neme12]

Now, importantly, that's the APIs in Microsoft.CodeAnalysis.CSharp/VisualBasic. The code in Microsoft.CodeAnalysis.Workspaces and above is not under the same constraints

So do you think lambdas should stay in DeclarationKind?

Sorry, it's still not clear to me why you would need to do this. Let's step back: what are you trying to accomplish, and why is SyntaxGenerator not working for you? Maybe from that we can come up with a more suitable solution.

I don't. One thing I'm a little bothered by is that lambdas are in there and anonymous methods are not. I think either both should be there or none.

And if they were both in there, I just thought it would allow some of the code that tries to find either a MethodDeclarationSyntax or lambda etc to be refactored into the common code rather than having separate implementations for C# & VB if it could use the SyntaxGenerator. But that's not a big deal, it was just an idea that occured to me. The code for that is pretty short anyway.

[CyrusNajmabadi]

Honestly DeclarationKind.LambdExpression seems like a mistake. As far as i can tell, we don't even use it for anything. It's not even ever referenced in the VB code.

Given that no one in the entire roslyn codebase ever even reads this, i would just deprecate it.

[CyrusNajmabadi]

Tagging @mattwar for his thoughts here. @mattwar it seems like DeclarationKind.LambdaExpression was unintentionally added. First off, while it is returned from GetDeclarationKind, absolutely no code ever reads this value or does anything special with it. Also, VB doesn't ever use it period.

Is there value in this flag? I get that we probably can't remove it for binary compat. But i think it would be fine to deprecate it and not have C# return it anymore for lambdas.

Thoughts?

[Neme12]

@CyrusNajmabadi would you review the code please? I think you are the original author of those refactorings, is that correct?

[CyrusNajmabadi]

Can you ping me on monday? I'm relaxing :D

[Neme12]

@CyrusNajmabadi fair enough, sorry, I didn't expect you to do it now