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.

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

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

Open
Pilchie opened this Issue Jan 29, 2016 · 71 comments

Comments

@Pilchie
Member

Pilchie commented Jan 29, 2016

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

This comment has been minimized.

Member

Pilchie commented Jan 29, 2016

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

@gafter

This comment has been minimized.

Member

gafter commented Jun 2, 2016

@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.

@Shadetheartist

This comment has been minimized.

Shadetheartist commented Sep 11, 2018

This bug has been killing dreams since 2016

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

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

@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 rchande removed their assignment Sep 11, 2018

@rchande

This comment has been minimized.

Member

rchande commented Sep 11, 2018

@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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

@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

This comment has been minimized.

Shadetheartist commented Sep 11, 2018

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

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

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

@Shadetheartist

This comment has been minimized.

Shadetheartist commented Sep 11, 2018

Sorry, i have no knowledge of compiler programming :(

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

@Shadetheartist This woudl not be in the compiler. This would be purely in the Roslyn IDE side of things.

@Shadetheartist

This comment has been minimized.

Shadetheartist commented Sep 11, 2018

No knowledge of that either :(((((((

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 11, 2018

I'm happy to help walk someone through it :)

@TechnikEmpire

This comment has been minimized.

TechnikEmpire commented Sep 12, 2018

Doesn't Microsoft have paid developers work on this stuff? I appreciate things are open source but at a certain point it just kind of feels like working for free for a company that makes billions upon billions of dollars per year on closed source software, including software that makes use of projects like this.

@jaredpar

This comment has been minimized.

Member

jaredpar commented Sep 12, 2018

@TechnikEmpire

Doesn't Microsoft have paid developers work on this stuff?

Yes but even paid staff is a finite resource. Roslyn is a big, complicated code base with a number of competing priorities. We get to as much work as we can but even so we can't fix every single issue that is filed.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

Doesn't Microsoft have paid developers work on this stuff?

There are limited resources, and a near infinitely long backlog :) Everyone on the team has a full workload continuously. Basically, the same as every other project out there. The nice thing about being open source is that instead of being in the positoin where that means that you are beholden to that, you now have the power to contribute yourself if this really is that important to you. I've done this numerous times to roslyn for things that were not as important to the team as the stuff they were working on themselves.

As i mentioned already, i'm happy to help out another contributor here. My plate is currently full, so i can't take it on myself. If you're interested, i'd def work with you on this. If this that important to you then this would fast track getting things fixed :)

If it isn't that high a priority for you... well... then it's easy to understand why it might not be for anyone on the Roslyn team either ;-) As i said above, when i encounter this myself, i try to contribute fixes/features. They're happy to take them, and will work with you to facilitate that. This means i get the benefit of the features they are prioritizing, while also being able to address somewhat less important things for them that still matter a lot for me. That seems like a great position to be in, and far better than if this was not open source.

@TechnikEmpire

This comment has been minimized.

TechnikEmpire commented Sep 12, 2018

If someone can point in the general direction that'd be great. Also, while I appreciate the arguments, this bug is responsible for issues using asp.net core with entity framework, a major technology being promoted and heavily invested in my Microsoft, as well as companies pouring money back in. So there's always the option of, ya know, spending those billions on more employees. :)

Anyway general direction please, no promises but I'll see if I can get a look in.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

If someone can point in the general direction that'd be great.

Sure! I'll write something up and get back here soon :)

this bug is responsible for issues using asp.net core with entity framework, a major technology being promoted and heavily invested in my Microsoft, as well as companies pouring money back in.

All the stuff being worked on is important to many groups of people :) And then there's the mythical man month issue. You can't just keep throwing people at things. It doesn't actually scale.

Again, this is the virtue of being open source. If this is important to you (or the asp.net core teams, or entity framework teams, or whoever), then the people who care deeply can show how much they care by investing their own limited resources into things. :)

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

So, i think teh place you'd want to start is here:

CSharpRecommendationService.GetSymbolsOffOfBoundExpression

private static ImmutableArray<ISymbol> GetSymbolsOffOfBoundExpression(

This is the specific completion code that is invoked when you have <some expr><dot> (for C# at least). For now, let's just work on a C# fix. I don't know if VB has the same problem. If so, we'd probably port any fix here over to the equivalent version on the VB side.

This function will be passed in a few things of interest. As you can see, one of the first things checked is the "symbol that the expression on the left of the dot bound to:

If this symbol is an IParameterSymbol (which we check for here:

) then i would say we should now start writing our specialized handling (most likely with a helper function to keep thigns cleanly separated).

I would first check if this parameter was a lambda parameter symbol. You can do that by grabbing it's containing symbol, and seeing if htat is an IMethodSymbol whose kind is an AnonymousFunction. If so, i would then grab the lambda syntax from that IMethodSymbol, and check to see if it is an 'argument expression', and it had a containing invocation. i.e. you're trying to grab this code ...ThenInclude(...

Once you have the invocation, you can use the SemanticModel.GetMemberGroup API to say: "give me all the overloads of 'ThenInclude' that you can find". Those resultant methods may or may not already be in reduced-extension-method form. "Reduced extension method form" just means "the extension method has been transformed into instance-method form". If it's already in that form, that's great. If not, let me know and i'll let you know what to do.

Now that you have all these appropriate overloads of the method (in instance-method form), we'll then find the index of our lambda in its parameter list. This will tell us the type expected for that parameter. If this type is an Expression<...> unwrap the expression part and grab the inner type parameter. If not, just use the type found. If this is a delegate type, then get the parameter-list for its 'Invoke' method, and find the index of the original lambda parameter you had in that invoke-method list. That's going to be the 'parameter' symbol we'll want to now get completions for.

The result of this will be a list of viable parameters that we want completion for. We should only use the different parameters if htey actually have different types. Once you have this, we'd want to then extract this part of GetSymbolsOffOfBoundExpression:

var position = originalExpression.SpanStart;
var symbols = useBaseReferenceAccessibility
? context.SemanticModel.LookupBaseMembers(position)
: excludeInstance
? context.SemanticModel.LookupStaticMembers(position, container)
: SuppressDefaultTupleElements(container,
context.SemanticModel.LookupSymbols(position, container, includeReducedExtensionMethods: true));
// If we're showing instance members, don't include nested types
return excludeStatic
? symbols.WhereAsArray(s => !s.IsStatic && !(s is ITypeSymbol))
: symbols;

And call that for each parameter symbol we found, merging all the results together.

--

Each of these steps is pretty basic, but likely will take some spelunking to learn the proper roslyn APIs for it. I'm happy to help out if you get stuck at any point. Cheers!

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

@TechnikEmpire

This comment has been minimized.

TechnikEmpire commented Sep 12, 2018

Thanks for the direction. Also all companies should reduce to a single staff member immediately because the division of labor and Adam Smith is fake news and trillion dollars market values were not obtained by scaling. Lol

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

https://www.amazon.com/Mythical-Man-Month-Software-Engineering-Anniversary/dp/0201835959

It's good reading. Basically, there usually is a sweet spot for a project, given its size, complexity, etc. Not going over that spot does not imply that one should go under :)

@TechnikEmpire

This comment has been minimized.

TechnikEmpire commented Sep 12, 2018

Book seems irrelevant since it predates the dinosaurs. Anyway I'll apply the summary of this book, that my contribution will only slow down the solution, and withdraw! Looking forward to a faster fix to this issue by not increasing the number of contributors.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

Book seems irrelevant since it predates the dinosaurs.

The book is relevant because nothing has changed here :) These aspects of engineering remain just as relevant today, and understanding them is super helpful for dealing with any software project.

Anyway I'll apply the summary of this book, that my contribution will only slow down the solution, and withdraw!

That's not at all the summary of the book. And this is why it would be valuable to actually read it. :)

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

and withdraw!

Pity. If anyone else is interested, the above instructions still stand, and i'm definitely happy to try to help out if you run into issues here. :) I think this would be great if the community was able to work through issues they run into, and i think that would help out for future contributions when other things are encountered in the future.

It will be a very helpful way to address what is obviously a very real problem when some subset of users wants priorities to be different than what the team itself feels is appropriate at any given time.

@TechnikEmpire

This comment has been minimized.

TechnikEmpire commented Sep 12, 2018

Maybe you should stop preaching to people if you want help lol. Your argument is literally "this is open source so that more people can contribute for free because Microsoft shouldn't have to hire more people because adding more people to the project doesn't help so please everyone on Earth jump in and help because it's open source."

I was interested in helping out but I'm allergic to high horses and condescending motor mouth.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Sep 12, 2018

Maybe you should stop preaching to people if you want help lol

I don't personally care here. I don't use these systems :) So the absence of this functionality here doesn't bother me. I'm just trying to help out the best i can with the information i have available. I'm also trying to help explain why it's not sufficient to just hire more people.

I was interested in helping out but I'm allergic to high horses

talking about engineering issues relevant here is not a high horse. It's attempting to just help get clarity on the very real and important problems that make such solutions not necessarily suitable.

Teams contain finite resources, and there are very real and relevant issues involved with adding to that. In other words, while the first impression of finite resources bein ga problem is "just convert money to resources" it very often does not work to actually solve the problem at hand.

On the other hand, i've laid out an approach that can actually help out here. And i'm totally willing to work more toward trying to alleviate the problem for you. If you are interested let me know. At thsi point i'm not sure this metadiscussion is actually helpful toward solving the actual completion bug. So if you'd like to discuss that further, i'd recommend going over to gitter.im where we could discuss things privately. Otherwise, it would be good to keep things on topic here

Thanks! And i hope we'll be able to work together on this :)

@spottedmahn

This comment has been minimized.

spottedmahn commented Sep 13, 2018

"What one programmer can do in one month, two programmers can do in two months." - Fred Brooks
Source: Twitter

Brook's law

👍 @CyrusNajmabadi 😊

@th0mk

This comment has been minimized.

th0mk commented Nov 9, 2018

Are there any updates on this issue? Ran into this today, cost me about an half an hour before I found it was just an autocomplete issue 😕

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 9, 2018

@th0mk There has not been. However, i have outlined an implementation strategy that someone could follow if they wanted to provide a PR here. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment