dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.03k stars 4.03k forks source link

Completion missing members of lambda parameter in fault tolerance case #8237

Closed Pilchie closed 4 years ago

Pilchie commented 8 years ago

In the code below, Task should be shown in completion from one of the overloads of ThenInclude but it doesn't.

This case is interesting because there are two applicable overloads. The ideal thing here would be to merge their individual members for the completion lists, but we don’t do that – we just pick one overload as the “best so far”, and it’s the one with ICollection<TPreviousProperty>, not the one with just TPreviousProperty.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;

namespace ThenIncludeIntellisenseBug
{
    class Program
    {
        static void Main(string[] args)
        {
            var registrations = new List<Registration>().AsQueryable();

            // type "a => a." and only ICollection<T> and Enumerable members appear
            var reg = registrations
              .Include(r => r.Activities).ThenInclude(a => a.$$
        }
    }

    internal class Registration
    {
        public ICollection<Activity> Activities { get; set; }
    }

    public class Activity
    {
        public Task Task { get; set; }
    }

    public class Task
    {
        public string Name { get; set; }
    }

    public interface IIncludableQueryable<out TEntity, out TProperty> : IQueryable<TEntity>
    {
    }

    public static class EntityFrameworkQuerybleExtensions
    {
        public static IIncludableQueryable<TEntity, TProperty> Include<TEntity, TProperty>(
         this IQueryable<TEntity> source,
          Expression<Func<TEntity, TProperty>> navigationPropertyPath)
         where TEntity : class
        {
            return default(IIncludableQueryable<TEntity, TProperty>);
        }

        public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
            this IIncludableQueryable<TEntity, ICollection<TPreviousProperty>> source,
            Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) where TEntity : class
        {
            return default(IIncludableQueryable<TEntity, TProperty>);
        }

        public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
            this IIncludableQueryable<TEntity, TPreviousProperty> source,
            Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) where TEntity : class
        {
            return default(IIncludableQueryable<TEntity, TProperty>);
        }
    }
}
Pilchie commented 8 years ago

Is this something we can fix on the IDE side, or do we need changes/additions to compiler APIs.

gafter commented 8 years ago

@Pilchie This was on your list of places where the compiler doesn't help much. I'm not sure what you would want of the compiler here. The compiler doesn't make up types that combine the members of existing types for the purposes of error recovery. Is that what you would want? @rchande Please feel free to suggest what we could do to help here.

divega commented 7 years ago

@Pilchie @gafter @rchande Could you please provide an update on this issue? It is marked as blocked but from reading the comments it seems there was some outstanding question on whether this is really a bug on Roslyn, or something that would need to be fixed on the way the IDE uses Roslyn to get the complete list of applicable members. If the latter, what would be the best way to get some movement on this?

Customers keep hitting this limitation, as you can see by the number of GitHub issues that reference this one (there is about twice as many questions on email and other websites about the same problem).

CyrusNajmabadi commented 7 years ago

@gafter Given the code:

var reg = registrations
              .Include(r => r.Activities).ThenInclude(a => a.$$

When the IDE asks "what is the SymbolInfo for 'a'?" I think it would be good if we could get a SymbolInfo with no Symbol, but two candidate symbols. One would be The parameter called 'a' with type 'ICollection<TPreviousProperty>' and the other would be The parameter called 'a' with type 'TPreviousProperty'

The IDE could then get the members of each and union them in a single list to show users.

colltoaction commented 7 years ago

Hey guys,

Just wanted to let you know I'm hitting this issue. I'm using the latest VS 2017 Community RC.

Shadetheartist commented 7 years ago

I am also feeling this issue. Also using the latest VS 2017 Community RC.

0xdeafcafe commented 7 years ago

This issue is also present in VSCode using the C# extension.

divega commented 7 years ago

Same issue reported here: https://developercommunity.visualstudio.com/content/problem/50498/intellisense-not-working-for-ef-core-theninclude.html

spottedmahn commented 7 years ago

Any updates? Please fix! :)

gafter commented 7 years ago

Fixing this would probably require a few weeks of effort.

There is no mechanism right now for the semantic model to “merge” results from distinct (erroneous) trial bindings of a lambda. There is no mechanism for binding an expression with the type of a variable being “either this or that”. The GetTypeInfo API, which the IDE currently uses to get the type from which its completion list is populated, has no mechanism for returning more than one type. This would require some effort in the compiler and in the IDE code and possibly some new APIs. It also risks degrading the IDE experience for the cases that we already handle well (because instead of returning what we think is the “best” result, we’d return all possible results). If we make the completion story better, we are likely to degrade the quality of data we provide when you hover over the parameter (e.g. its type).

The most likely way we’d handle this would be for an unbound lambda’s error-recovery binding to use a new kind of error type for the parameters that merge the members of all the possible types from the parameters of the delegates the lambda could bind to. Error types today don't have any members, but these new types would have lots of members merged from different types. In order to maintain the invariant that a types's members have a ContainingType that is the type from which the member was fetched, we may have to invent a number of other kinds of error symbols.

Pilchie commented 7 years ago

Reported again in https://developercommunity.visualstudio.com/content/problem/50498/intellisense-not-working-for-ef-core-theninclude.html

gafter commented 7 years ago

@Pilchie That's what https://github.com/dotnet/roslyn/issues/8237#issuecomment-298243181 says.

divega commented 7 years ago

There is no mechanism right now for the semantic model to “merge” results from distinct (erroneous) trial bindings of a lambda...

@gafter then the solution that @CyrusNajmabadi described at https://github.com/dotnet/roslyn/issues/8237#issuecomment-278251000 does not apply?

gafter commented 7 years ago

@divega I'm saying that there is no mechanism to do that today. We could add such a mechanism, but that could be a lot of work.

gafter commented 7 years ago

For example, if we expose two different parameters as @CyrusNajmabadi suggests, each would have to have a corresponding lambda (even though there is only one lambda in the source).

spottedmahn commented 7 years ago

Hey guys - just want to say thanks for problem solving this for us in the community. Hopefully you guys can find a solution that is a reasonable number of hours to enchance It.

dguisinger commented 7 years ago

This really does need to get fixed, I'm glad I found the bug reference after only 30 minutes of trying to figure out why it wasn't giving me the options I expected and wasn't matching EFCore documentation. This bug is likely a huge source of wasted time and frustration for anyone trying to use Include().ThenInclude().... or at least someone should update the ThenInclude documentation to indicate that there is a intellisense bug to stop people like me from pulling their hair out.

divega commented 7 years ago

@gafter, @Pilchie, @CyrusNajmabadi and others:

TL;DR: Even getting the union of all possible members is not ideal for this API. However we believe a (hopefully) simpler solution would be sufficient to fix the EF Core experience: somehow change Intellisense to give preference to the other overload from the one it is using now.

Details

Today @smitpatel (one of the EF Core developers) came up with an insight on this issue that I think may create an opportunity for a stopgap solution. Hopefully this will save you two weeks :smile::

It turns out that from the perspective of the usability of this particular API, we are not really interested in Intellisense showing the union of all the possible members in the completion list. It should be sufficient for the vast majority of real-world EF Core scenarios if Intellisense gave preference to the "right" overload of ThenInclude().

E.g. currently given this query:

var reg = registrations
              .Include(r => r.Activities).ThenInclude(a => a.$$

The following overload is (seemingly) arbitrarily picked:

public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
            this IIncludableQueryable<TEntity, TPreviousProperty> source,
            Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) where TEntity : class

... and hence Intellisense shows the members of ICollection<Activity> in the completion list.

We actually need this one to be picked:

 public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
            this IIncludableQueryable<TEntity, ICollection<TPreviousProperty>> source,
            Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) where TEntity : class

... so that the members of Activity are displayed in the completion list.

I am not sure if it would be acceptable to plainly invert the choice in the code used by Intellisense or if we would need a way (e.g. an attribute) to designate the preferred overload. If the later, I think it could have other uses besides this scenario, like addressing the question in
http://stackoverflow.com/questions/1904876/can-visual-studios-c-sharp-intellisense-be-given-a-hint-to-display-a-certain-me.

There is of course a corner case in which an entity type happens to implement ICollection<T>, e.g.:

public class Registration : ICollection<Foo> { ... }
...
var act = activities
              .Include(r => r.Registration).ThenInclude(a => a.$$

From the perspective of everything that is possible to express in the language, I believe it is still valid to say that ideally Intellisense should show the union of all the possible members, but pragmatically I believe this scenario is kind of fringe and it should be ok if Intellisense always preferred the overload that takes an IIncludableQueryable<TEntity, ICollection<TPreviousProperty>>.

I would say even more: if you come up with a solution that displays the union of all the possible members in the completion list, the usability of this EF Core API is still going to be bad unless the members corresponding to our preferred overload are displayed at the top of the completion list.

cc @ajcvickers

divega commented 7 years ago

@dguisinger good point about the documentation. I have crated an issue in our docs: https://github.com/aspnet/EntityFramework.Docs/issues/394.

CyrusNajmabadi commented 7 years ago

It should be sufficient for the vast majority of real-world EF Core scenarios if Intellisense gave preference to the "right" overload of ThenInclude().

Do you have a suggestion on how the compiler would know that one particular overload was better than the other?

divega commented 7 years ago

@CyrusNajmabadi, my knowledge of the inner workings of the compiler is very limited, but I suspect it might not be an option for the compiler to automatically know that a particular overload is better. E.g. I suspect rules similar to what the compiler uses to choose the most specialized overload in overload resolution wouldn't be applicable here, although the problem seems similar on the surface.

So, I had in mind these alternatives:

  1. Just invert whatever order is being used today: I assume there is an order of preference today and that somehow the "first" overload is being picked according to that order. Then in theory, that order could just be inverted. In fact @smitpatel and I tried to test the hypothesis that we could influence the order by just changing reflection order or even the order in which the overloads were defined in the source code, but the things that we tried had no effect. The main downside of this hack approach is that it could break the Intellisense experience for an existing API for which Intellisense is doing the most desirable thing today. It would also not be guaranteed to be stable over time, e.g. it could stop working in the desired way if you change some implementation detail in the future.

  2. Define an attribute that can be used to designate preferred overloads: Maybe this doesn't qualify as making the compiler know which overload is better, but there is precedence of attributes influencing the behavior of Intellisense, e.g. EditorBrowsableAttribute and ObsoleteAttribute.

gafter commented 7 years ago

We select the overload with the "fewest" errors. I'll look to see if that heuristic can be refined to improve the situation for these APIs.

brunoalreis commented 7 years ago

Greetings,

Today i'm hitting this issue. Do you have any news? Hope you find a simple solution to fix it. Thanks and keep the good work.

akw123 commented 7 years ago

Hey guys,

Appreciate the great work. Am looking forward for this to be fixed. :)

smitpatel commented 7 years ago

@divega - Can we follow-up with this just to make one overload preferable over other?

divega commented 7 years ago

@smitpatel thanks for the reminder.

@gafter, @jaredpar is there any new data on this? Last time @gafter mentioned he would look at tweaking the current heuristic. Also, it is not clear to me what "15.later" maps to in terms of a timeframe.

Thanks.

louislewis2 commented 7 years ago

Just found this issue. Just wanted to say I am also getting this problem. Using VS 2017 CE 15.3.3.

Also looking forward to a fix :)

SharePointRadi commented 7 years ago

An issue for me too, intellisense doesn't work for .Include(p => p.AccessIdentifiers).ThenInclude(p=>p.Identity)

Looking forward to a fix as it lost me quite some time.

VeldaHalvorson commented 6 years ago

the same problem for me, I have the latest update for visual studio and i had this issue intellisense not working perfectly before I knew this is an intellisense problem i lost alot of time figuring out what the issue!

darting commented 6 years ago

hi there, may I know any workaround solution?

gafter commented 6 years ago

@darting The workaround is to type your program in C#.

darting commented 6 years ago

Hi @gafter, may I know what you mean?

VeldaHalvorson commented 6 years ago

@gafter iam using Dot Net Core 2.0 and for sure C# and same bug ! its not the lang its the IDE i think...

Suchiman commented 6 years ago

@961Group i think gafter meant that the workaround is to write it yourself, its only the autocompletion that is broken so the workaround is just to type it yourself.

VeldaHalvorson commented 6 years ago

@Suchiman yes, i know but to knew it i lost a day rereading my project couple of times to see if any relationship design error. at the end, it was autocompletion problem... they should fix it

CyrusNajmabadi commented 6 years ago

@961Group PRs welcome :)

spottedmahn commented 6 years ago

Hi @gafter - did you see @divega's comment? Did you ever get a chance to:

I'll look to see if that heuristic can be refined to improve the situation for these APIs.

Reference

gafter commented 6 years ago

@spottedmahn No, I did not yet.

ajean42 commented 6 years ago

In EF core 2.1.1 with the latest VS, still having this issue as of today. I'm lucky I found this page fast. I could have lost a bunch of time on this issue!

GeroIwan commented 6 years ago

Would it be an option to have two differently named methods instead of two overloads? (Asked on https://github.com/aspnet/EntityFrameworkCore/issues/4117#issuecomment-404461872, too.)

TechnikEmpire commented 6 years ago

This bug has been killing dreams since 2016

Shadetheartist commented 6 years ago

This bug has been killing dreams since 2016

Try using Jetbrains Rider, it actually works in that ide.

CyrusNajmabadi commented 6 years ago

@rchande @jinujoseph This seems to have a fair amount of interest (26 votes), but no milestone assigned. Is there an intent that this be looked at in any sort of timeframe?

rchande commented 6 years ago

@CyrusNajmabadi @jinujoseph IIRC this requires work from the compiler in order to be completed. I've unassigned myself (but left @gafter) and defer to Jinu in choosing a new IDE advocate for this compiler work 😄

CyrusNajmabadi commented 6 years ago

@gafter you mentioned this:

For example, if we expose two different parameters as @CyrusNajmabadi suggests, each would have to have a corresponding lambda (even though there is only one lambda in the source).

Is that actually problematic though? For example, when the compiler is binding (for example, to determine which lambda would be best, and which will have the least errors), doesn't it have to make a lambda for each potential candidate? So aren't there multiple lambdas during binding to begin with?

In terms of consumption, it doesn't feel strange to me that we might get many 'candidate' parameter symbols, each with a different potential different Lamdba symbol (even if each of those potential candidate lambda symbols pointed back to the same place in teh code). In effect, it would simply be the compiler saying: here's all the potential things i considered for this chunk of code. I couldn't decide which was best, but maybe you coudl find this information useful.

Shadetheartist commented 6 years ago

Maybe someone should just email the Jetbrains team and ask what they did to fix it in their ide?

CyrusNajmabadi commented 6 years ago

Another option as well might be to change how intellisense just works here when it comes to extension methods. Specifically, if we could find out there was a compiler error, we could try to fallback to an alternative approach where we got the method group for 'ThenInclude' ourselves. We coudl then attempt to Reduce that method group using ReduceExtensionMethod to get the reduced extensions with enough type information filled in.

IDE could then try to extract out the many potential parameters we could be arguments for. We could then apply our own heuristics here to pick a 'better' candidate. Or we could try some sort of 'merging' approach. This would alleviate the burden on the compiler here, and allow us to try to iterate on some solution at the IDE level that coudl result in a better experience.

--

In the above code, we woudl see that we had:

registrations.Include(r => r.Activities). We'd get the type of that, and the overload group for .ThenInclude. Calling ReduceExtensionMethod on each of the IMethodSymbols in that overload group would then give us several potential reduced method options. If we were then passing a lambda to either a delegate type (or Expression<DelegateType>) we'd then use those overloads to try to decide what to offer off of a, and would then show a merged list (i.e containing Task).

Note: from this point we should be good. I just tested this out, and completion once you have written Task is totally fine. So as long as we just do this for an identifier that binds to a lambda being passed to an overloaded method, then we should be able to give proper completion, and get the user back on track.

CyrusNajmabadi commented 6 years ago

If anyone here would be interested in trying out the above potential solution, let me know. I can try to help guide you toward the right places in the code to look at for making this happen.

CyrusNajmabadi commented 6 years ago

@Shadetheartist Would you be interested in contributing a fix here? I think teh approach i outlined above would be viable.

Shadetheartist commented 6 years ago

Sorry, i have no knowledge of compiler programming :(