IDE0029 Use coalesce expression and LINQ

[jods4]

I am using VS 2017RC, Feb 7th update.

Consider the following code fragment:

from d in db.Deals
let limit = d.DealGroup.TradingDate != null ? d.DealGroup.TradingDate : d.ValidationDate
// ...

C# suggests the usage of a coalesce expression:

let limit = d.DealGroup.TradingDate ?? d.ValidationDate

In general this is a good suggestion, but in this particular case it would result in a runtime exception. Because this is an EF LINQ expression, and it doesn't support the coalesce ?? operator when converting to SQL.

As such this is dangerous because an unaware developer might quickly do a "safe" refactoring and not realize that he's introducing a runtime crash.

I suggest that this refactoring is not suggested inside Expression<> types.

[HaloFour]

EF doesn't support it, but other LINQ providers might. And nothing prevents EF from supporting it at some point either (actually kind of surprised it doesn't already, that would be a pretty simple transformation).

The bigger question here might be how LINQ providers can interact with the IDE to ensure that only supported syntax is used and supported refactorings are offered. An analyzer could cover the first option but I don't think there are any plugin systems that can tie into refactoring options.

[jods4]

@HaloFour Other LINQ providers might support it and removing the suggestion doesn't prevent users from updating their code. After all it's been like that until today. It's just not aggressively pushing them to.

I understand that Roslyn analyzers are a great way to teach people about new C# features and improve their code. I just feel like we should refrain from pushing where it might have negative impact.

That situation will lead to:

• people having bugs after using a refactor -> start thinking Roslyn refactoring are buggy/not safe (generalization).
• people having persistent suggestions in their list that they know they can't apply -> stop paying attention to the suggestions, maybe disable them completely.

Finally, Expression<> are a tiny fraction of code overall, so this issue is not a big change. Analyzers will continue to teach improved C# in the rest (vast majority) of the code.

[HaloFour]

You're probably right. While I think that it would be a nice end-game to allow for VS plugins to better inform the IDE as to what expressions are supported by a given query provider that is a somewhat lofty goal. I'd be curious as to what percentage of LINQ query provider developers are targeting EF but I'd wager that it's pretty high, and the IDE is freer to make those kinds of assumptions.

[Pilchie]

We agree that it's probably just worth disabling this in Expression contexts.

[CyrusNajmabadi]

I don't like the idea of disabling things like this. Basically, we're letting APIs prevent people from moving code to use newer language features. It also seems like this would be super confusing to people. The feature will just not work and users will not be able to tell why.

If the language feature is not actually avaialble in Expression<T> contexts, then it of course makes sense to not offer it. But if we're going to disable things just because some expression-consumer might not be able to handle new language-nodes, then that seems like an issue with that expression-consumer.

After all, users may just manually make this change anyways, and they'll be in the same boat.

[jods4]

just because some expression-consumer

It's EF so not just any expression-consumer. It's probably the most common one.

then that seems like an issue with that expression-consumer

It's also from MS, so maybe you can tell them that. I would love to use ?? inside my EF queries. (TBH I'm not 100% sure whether the limitation is in EF itself or the providers)

After all, users may just manually make this change anyways, and they'll be in the same boat.

Well at least it's their idea and their fault. I surely learned this from my own mistakes. If you ask me what would be awesome, it's that the IDE would add red squiggles when I use features that are unsupported in a specific Expression context. That would help. Adding lightbulbs to suggest a buggy refactoring does not.

Objectively, Expression that could benefit from this refactoring are relatively inexistant (because there are few expressions compared to code in general, fewer that are subject to the refactoring and even fewer where the provider supports it -- since the most used provider doesn't). So I don't think it's a big deal disabling the suggestion. You said it would "cause confusion", I think it will go unnoticed. Introducing bugs will be noticed, though. It's not what I would call a "pit of success".

[CyrusNajmabadi]

It's EF so not just any expression-consumer. It's probably the most common one.

Regardless. I don't like the idea that one API out there basically means we can't offer any refactorings within Expressions. After all, we basically have to know precisely what it supports and what it doesn't, and we have to never do anything that runs afoul of it.

It's also from MS, so maybe you can tell them that. I would love to use ?? inside my EF queries. (TBH I'm not 100% sure whether the limitation is in EF itself or the providers)

Sure. I'm happy to help with that. Looks like we can start by just porting to this repo: https://github.com/aspnet/EntityFramework6

Well at least it's their idea and their fault. I surely learned this from my own mistakes. If you ask me what would be awesome, it's that the IDE would add red squiggles when I use features that are unsupported in a specific Expression context.

How would we know the expression-context is unsupported? We don't know who will eventually consume these expressions. It might be a consumer that is totally fine with these. If EntityFramework wants to write an analyzer for this, then that seems reasonable. As they're the ones who have these domain-specific restrictions, it seems like they should be the ones warning that they don't support all the features that C# offers.

You said it would "cause confusion", I think it will go unnoticed. Introducing bugs will be noticed, though. It's not what I would call a "pit of success".

The problem is that this bug is talking about just one single IDE feature. But we'd really have to disable every single code-manipulation feature we have in an expression-context because of this. After all, all the same arguments presented here woudl apply to every other feature we have. I think it will go noticed when we suddenly disable lots of features.

[miloush]

I 100% agree with @CyrusNajmabadi on this one. The ternary and coalesce operators must be interchangeable here and it's a bug in the consumer if that's not the case. Improvements might be done on the API side so that new language features do not break existing consumers, but this is wrong end to "fix".

the IDE would add red squiggles when I use features that are unsupported in a specific Expression context

The compiler just can't know how you are going to use the expression, how do you suggest it would find out what's currently supported? It might even not know the consumer until runtime.

Well at least it's their idea and their fault.

I can't see how this is users' fault.

[jods4]

The compiler just can't know how you are going to use the expression, how do you suggest it would find out what's currently supported? It might even not know the consumer until runtime.

I know. I was making a point what a great user experience would be in an IDE that detects your mistakes. Especially in reply to "After all, users may just manually make this change anyways, and they'll be in the same boat.". Yes users can make mistakes and a great UX would be preventing them to, not encouraging.

I can't see how this is users' fault.

Well, if a dev on his own decides to change a ternary to coalesce and the program crash, who do you blame for the bug?

Now, if a junior dev does that because the IDE told him to and it looked nice... Doesn't test because he assumes automatic refactors are safe... I think the blame is shared.

The problem is that this bug is talking about just one single IDE feature. But we'd really have to disable every single code-manipulation feature we have in an expression-context because of this.

@CyrusNajmabadi ok so now I understand that what bothers you are not the benefits or drawbacks of this specific issue. It's that technical limitations prevent you from turning this one off and so the discussion changed to "turn everything off"? Which begs the question: can you lift that restriction?

Don't you have flags when running analyzers that describe the context where you run (e.g. to know if you're inside an async method)? If yes, can't you add one flag to know when you're in an Expression context?

[CyrusNajmabadi]

@CyrusNajmabadi ok so now I understand that what bothers you are not the benefits or drawbacks of this specific issue. It's that technical limitations prevent you from turning this one off and so the discussion changed to "turn everything off"? Which begs the question: can you lift that restriction?

No. That's not what i'm saying.

Here's the issue:

You're arguing that the IDE should not offer a feature because it might break in the context of Expressions. However, that issue applies not just to this specific issue that was raised with ?? but with making any changes within Expressions. After all, any potential language feature we use might break in someone's consumption of Expressions.

It doesn't make sense to me to just disable this specific feature. If this is truly a problem, then we'd need to disable essentially all modification features in Expressions. As we would continue improving features and adding more functionality for users, we'd continually have to hamstring them just because there might be an API out there that didn't understand it.

--

Now, regardless of this, i still feel like this is not an issue with the IDE. It seems like an issue with Entity Framework. Expression.Coalesce is part of the Expression API. Entity Framework not supporting it is not something that should limit us.

[CyrusNajmabadi]

I know. I was making a point what a great user experience would be in an IDE that detects your mistakes. Especially in reply to "After all, users may just manually make this change anyways, and they'll be in the same boat.". Yes users can make mistakes and a great UX would be preventing them to, not encouraging.

Again, that seems like something that Entity Framework should provide. They are the ones that have the problem with consuming Expressions that are part of the Expression API. Since that's a domain-specific problem, the solution should come from that domain.

Roslyn and C# are not the Entity Framework. Limitations in the latter should not be baked into the former.

[CyrusNajmabadi]

Well, if a dev on his own decides to change a ternary to coalesce and the program crash, who do you blame for the bug?

I don't see how blame is even applicable here.

Doesn't test because he assumes automatic refactors are safe...

100% of the refactorings we provide are "unsafe" for some definition of "safety". Extracting a method is 'unsafe' as at runtime something might depend on that new method not being there*. Renaming is unsafe as something might depend on the actual names used. If we start defining safety as "must not change anything from any code that might introspect things at runtime" then we literally cannot make any changes because all changes we make can eventually be observed in some fashion.

We write features that work properly according to the actual specifications of the C# and VB languages. If platforms/APIs have additional restrictions, then the onus is on them to provide the VS plugins that notify people of those limitations. Otherwise, we could never provide any code-modification features as they might end up running afoul of some API out there.

Again, i would recommend porting this bug over to the EF repo. You might be able to also just address it by giving them a PR that adds support for Expression.Coalesce. Then, instead of having an arbitrary restriction in this useful feature, you'd actually be able to use the feature effectively without having to worry about API limitations.

[jods4]

Now, regardless of this, i still feel like this is not an issue with the IDE. It seems like an issue with Entity Framework. Expression.Coalesce is part of the Expression API. Entity Framework not supporting it is not something that should limit us. [...] Roslyn and C# are not the Entity Framework. Limitations in the latter should not be baked into the former. [...] We write features that work properly according to the actual specifications of the C# and VB languages.

I understand and kind of agree but that's the engineer thinking about ideal software. Thinking about the real world user experience: first week of trying VS2017 your code almost introduced a bug in my project. "Almost" only because I have experience and recognized the problem the moment I clicked on that refactor button.

And this is very not an obscure use-case. Where I work (enterprisey stuff) EF is used in almost all projects and right now I can't think of another Expression based API that is widely used. In fact, EF probably has one of the most complete Expression support, compared to NHibernate, LINQ to LDAP and others. Generally speaking, although C# is a fully featured languages, all LINQ providers only support a tiny subset of it in Expression. IMHO reducing the suggested refactorings in that context is acknowledging that reality.

I don't know what the perfect solution is but there is a UX problem and a high risk of introducing bugs. If EF can provide analyzers that detect all invalid patterns in Expression that can be good. Supporting ?? in new releases is great but not all that helpful because upgrading (esp. to EF 6) is not a given in enterprise environment. None of this will help with other LINQ providers.

100% of the refactorings we provide are "unsafe" for some definition of "safety".

Yes, let's use the reasonable definition shall we? It's unlikely that renaming, extracting a method or transforming x == null ? into x ?? in normal code introduces a bug and under some reasonable hypothesis (e.g. no reflection, no dependency on timings or code size) it's provable. I can reasonably expect that replacing return x == null ? with return x.? in an automated way will result in code that compiles and run. This reasonable expectation is broken in the issue we're discussing here.

[CyrusNajmabadi]

This reasonable expectation is broken in the issue we're discussing here.

Right. But that 'reasonable expectation' is broken for all refactorings. Any customer might run in some environment where that expectation does not hold.

None of this will help with other LINQ providers.

Again, i do not want to not provide features because there may be an API out there that does not support the transformations we provide. You may report this issue today, but then someone reports an issue with extract-method in Expressions tomorrow. And Rename in expressions the day after. Disabling IDE features for Expressions just because some expression consumers cannot handle them is not appropriate. With that approach we simply cannot continue providing language features (especially ones that offer to help you use more modern language constructs).

The issue here lies with EF. They do not support part of the Expression API. Either they should add support, or they should write an analyzer that lets you know what you're doing will not work with their incomplete implementation of the consumption API. In either case, the issue is with EF, and the bug should be moved there. Thanks!

[CyrusNajmabadi]

I want to address this bit:

first week of trying VS2017 your code almost introduced a bug in my project.

We have never gone for an approach on refactorings where no-bugs will be introduced. Indeed, we strive for a balance where the feature can be generally widely available and useful, while still working properly for the majority of cases out there. This is true with every refactoring we provide. Refactorings can and do introduce issues (like runtime changes), and we've chosen to keep them over the years because we don't the negative issue outweighs the bad experience of not having the refactoring available.

Right now this refactoring is completely following the rules of the language and behaving as expected. It's clear that any changes to Expressions may end up being an issue for Expression consumers. But that issue lies with them. For example, if you rename a method, that can absolutely break an Expression consumer (after all, tehy may be mapping a method-name to some functionality in their system). Does that mean we cannot offer Rename? Or that rename should not change code in an Expression?

Right now we provide the features that work properly with our language. If that is an issue for other APIs out there, then the onus is on them to properly incorporate support for the later language versions. We're not going to keep C# at version 2. We're going to continue moving it forward and working to help people migrate.

[jods4]

It's your call. But I'd like to point out that, to me, a lot of your comparisons seem inappropriate.

We have never gone for an approach on refactorings where no-bugs will be introduced.

I'm sure that you understand what I mean. Outside of Expression that we discussed here, can you give me a reasonable example where transforming x == null ? into x ?? or x.? can introduce a bug?

Everyone knows Reflection and Dynamic code are the big offenders because they are impossible to statically verify. We don't expect you to be safe with respect to that kind of code. We usually have that expectation for statically typed code. Excluding Reflection, do you have examples where Extract Method can introduce bugs? Rename? Change order of parameters? Transforming x as T into pattern matching?

we've chosen to keep them over the years because we don't [think] the negative issue outweighs the bad experience of not having the refactoring available.

That's an excellent argument, let's apply it here. Do you know a common Expression API that supports ?? properly? I can't think of one right now.

• The negative issue is that proposing this refactoring in Expression context is almost guaranteed to be suggesting a bug to the user (unless I missed a prevalent API).
• What is the bad experience of not having the ?? refactoring in Expression contexts? Is there enough Expression APIs that will benefit from it to outweight suggesting bugs to many users?

Or that rename should not change code in an Expression?

The general experience is that rename works in Expression contexts, for example EF. I use it all the time. So let's keep it in. The general experience is that ?? does not work in Expression contexts.

We're not going to keep C# at version 2.

That's not even remotely close to this issue.

[CyrusNajmabadi]

Excluding Reflection, do you have examples ... Rename?

Yup :)

I've been bitten by rename bugs with Expressions. Since the expression consumer wants to map that name to something it knows about, if you change the name, you break things at runtime.

Change order of parameters?

Yup. Same issue. I've had htat break since the parameters no longer map to a sproc properly on the other end.

Another example is 'convert property to method'. This broke with Expressions since there was code that blindly casted the Expression node to the proprty-access form, and could not handle the form corresponding to a method-call.

[CyrusNajmabadi]

The general experience is that rename works in Expression contexts, for example EF. I use it all the time. So let's keep it in.

What if someone comes and says "Rename doesn't work in my Expression consumer XXX context"? :) What should we do then?

[jods4]

Since the expression consumer wants to map that name to something it knows about, if you change the name, you break things at runtime.

If it can get the name then it involves Reflection. The question was to cite an unsafe C# refactoring outside of Reflection and Dynamic code. As I said, nobody expects Reflection (or framework that use names in your code, hence reflection under the hood) to be refactoring safe.

Same for your change parameters answer.

The Convert property to method is more like it. Makes me wonder if at least a warning in the UI when the transformation happens in an Expression could be helpful.

What if someone comes and says "Rename doesn't work in my Expression consumer XXX context"? :) What should we do then?

As always the answer is "it depends". I can't answer a general hypothetical question like that. Today nobody said that. Based on a concrete case I think there can be an argument for both keeping, removing or finding another clever solution.

You did not answer the question: what benefits does this bring to outweigh the drawbacks? I can't think of a common Expression API that supports ??, so right now my assessment of the feature (in Expression contexts) is 100% bugs and 0% usefulness.

[kuhlenh]

Hi all!

We can all agree that refactorings and quick actions are great because they not only help developers be more productive but also they can be valuable learning tools. Roslyn enables us to quickly write these productivity tools and have confidence that our tools have a high-fidelity understanding of the language so that we can empower all .NET developers to efficiently build the products/games/websites they work on. That said, as software engineers, we all know that there are always corner cases and bugs will happen.

@jods4 has correctly identified a case where we offer a suggestion that causes a runtime exception because it is offered within an Expression<T> context. I agree that this is not a great experience for developers and the more refactorings we add, the more and more developers will run into this problem.

There is no level of static analysis that will let us make the perfect refactoring here. Thus, we have three options:

• Limit refactorings that suggest newer language features on Expression trees on the assumption that it might do something unexpected.
• Split these refactorings into two--one that runs on Expressions and one that runs on everything else--and provide different editor treatments. When a refactoring appears on an Expression tree we can make it a Hidden diagnostic, meaning there will be no visual text decoration, but a lightbulb will appear if the cursor is in the expression (note: this means it will not be fixed in a Fix All operation). When a refactoring appears elsewhere, we continue to use whatever severity is set in Tools>Options or EditorConfig.
• Keep showing the refactoring everywhere and take the hit to our reputation.

We will continue this discussion next week in person, and I will update this thread with what we've decided.

[jods4]

One last thought that just occured to me: I don't have VS 2017 at hand right now, but does this have a Fix all occurrence command? If yes, it makes this 100x worse. Because you might be looking at normal code, thinking "yeah great simplification, go ahead on the whole project" and totally miss the problematic sites.

This new consideration makes me lean toward @kuhlenh's second option: distinct "in Expression" / "outside Expression" treatment, so that we can "fix all" in the project yet not break expressions.

[CyrusNajmabadi]

A couple of things to point out:

If it can get the name then it involves Reflection.

That's not actually the case :) Lots of the Expression API just exposes the names of things. For example "Parameter.Name" and whatnot. You can get at the names of things without using reflection as that's sort of the point of Expression (to give you the information about the shape of the source code without having to go through reflection).

Same for your change parameters answer.

Same thing. InvocationExpression.Arguments will give you information about the arguments. So reordering things can break, precisely because the end API expected a certain order to parameters :)

The Convert property to method is more like it. Makes me wonder if at least a warning in the UI when the transformation happens in an Expression could be helpful.

Yes. I would be ok with us giving a general purpose warning saying "code in an Expression will be changed, and that may have an effect at runtime".

You did not answer the question: what benefits does this bring to outweigh the drawbacks?

I did answer that question. The benefit is that people aren't left in the positoin of going "why on earth isn't this refactoring that i'm expecting not available?" When this happens people do think the IDE is busted and that there's something wrong. We've generally erred on providing people with the tools, even if it means they might possibly run into situations where that might be problematic.

Note: we will discuss this quite a bit. I'm back in the office on Monday and we'll hopefully come up with a plan on what to do here.

[CyrusNajmabadi]

I've got a mitigation change available here: https://github.com/dotnet/roslyn/issues/17264

We will warn if you do this. But we will still allow the user to make such a change if they want.

[jods4]

Don't know if this is included in your change, but would be nice if I could do a "fix all occurrences" with "except in Expression".

[CyrusNajmabadi]

It is not included in the change. We can consider making such functionality available.

[alrz]

Rather than a separate option, fix-all preview could deselect those cases by default as there is a warning?

[CyrusNajmabadi]

Mitigated with https://github.com/dotnet/roslyn/pull/17264