Skip to content
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

VS Intellisense, generic parameters, and value-tuples #29949

Closed
TomFinley opened this issue Sep 17, 2018 · 24 comments · Fixed by #56025
Closed

VS Intellisense, generic parameters, and value-tuples #29949

TomFinley opened this issue Sep 17, 2018 · 24 comments · Fixed by #56025
Assignees
Labels
Area-IDE Concept-Continuous Improvement Design Notes Need Design Review The end user experience design needs to be reviewed and approved.
Milestone

Comments

@TomFinley
Copy link

TomFinley commented Sep 17, 2018

Hello Roslyn friends, I was asked to write an issue here since it was identified as something best addressed through Roslyn. However I originally phrased it as a VS issue, so my language reflects that somewhat, since I am not entirely certain how Roslyn is involved. So, please bear with me. 😄

Visual Studio intellisense, hoverovers, and such things like this currently do not handle value-tuples in what I would consider a good way. However, other vaguely similar situations on more established idioms already have good practice, just maybe the UI hasn't quite caught up to value-tuples yet since they are so new.

The problem becomes obvious in cases where there is a library with generic type parameters, and value-tuples are ever used in those generic type parameters. LINQ would be the most obvious example, but I'll use immutable collections here. (No particular reason, I just happened to be playing with them in a scratch project, so that's what I wound up using here.)

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
 
namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            var foo = ImmutableArray.Create(1, 2, 3, 4);
            var a = new { well = 5, gee = 3, that = 5, sounds = 2, terrific = 0 };
            var bar = ImmutableArray.Create(a, a, a, a);
            var t = (well: 5, gee: 3, that: 5, sounds: 2, terrific: 0);
            var biz = ImmutableArray.Create(t, t, t, t);
        }
    }
}

I'll give a series of examples, marked with ✔️ and ❌ examples to show what I feel are good and bad. I also list the good examples since I feel like they inform some ways in which we could address the bad examples.

  1. ✔️ Cursor over the first lines Create:

    image

  2. ✔️ Cursor over third line's var. This one is slightly odd, but at least it is comprehensible by a human.

    image

  3. ✔️ Cursor over third line's Create:

    image

  4. ✔️ Cursor over fifth line's var:

    image

  5. ❌ Cursor over fifth line's Create:

    image

    This sort of seems like a missed opportunity. I would argue this is not easily comprehensible by a human, but it is easy to see how we might make it so. From example 3, I know these method can have sort of auxiliary explanatory information on the types of methods, and from example 4 I know that at least the type description is capable of calling them out.

    Hypothetically, in this case (generic parameter that is a value-tuple) maybe something that looks more like the following, in the case where the generic type parameter relevant to a method is a value-tuple, it could look like this. Please forgive my limited formatting skills:

    image

  6. ✔️ Speaking not just of hover-overs but Intellisense, I think it's more or less the same but just to emphasize, if I cursor over the third line just prior to the ;, then type ., we get this method Add as the first one. This looks pretty good.

    image

  7. ❌ If I do the same on the fifth line, this is what Add's intellisense display looks like:
    image

    Similarly to point 5, I feel like the enumeration of the type every single time it appears decreases the helpfulness of the display. We could imagine solving it in a similar fashion.

    image

/cc @jinujoseph @heejaechang @KrzysztofCwalina @Zruty0

@CyrusNajmabadi
Copy link
Member

Yes. We could do better here. Relevant parties also include @jcouv . We do a good job here for anonymous types (for the exact reasons outlined above). We should consider treating tuple types like anonymous types for the purpose of intellisense presentation to help declutter things.

@CyrusNajmabadi
Copy link
Member

Note: IMO, this issue is somewhat less critical due to my totally gut belief that people will not make large tuples that often. mainly because it does become unwieldy to use them (especially if you ever want to put them in a signature for something yourself). i.e. i'm not going to write:

(int well, int gee, int that, int sounds, int terrific) DoSomething((int with, int another, int tuple) t1, (int and, int another, int one, int just, int to, int be, int obnoxious) t2)

Large tuples themselves just get obnoxious in source. So i think the tendency for people will be to keep things somewhat minimized.

This is contrasted with anonymous types (which can never show up in user sigs) and are very often releated to just a small part of the code, and which can easily grow the number of members they have without incurring the same textual overhead.

So, while i think it was critical for anonymous types to keep things sane, i think it would be nice (but fas r less important) to the same for tuples.

@jcouv If you need info/help on prototyping this, let me know.

@heejaechang
Copy link
Contributor

heejaechang commented Sep 17, 2018

copy and paste what I wrote in the email.

...

We probably need to first see whether this is what we want. And find out why we went to inline route rather than anonymous type route. Pretty sure people have considered both and choose one (inline)

For example, without special syntax for tuple in C#, things would have shown as regular generic type such as

ImmutableArray<ValueTuple<int, int, int, int>>

Rather than

ImmutableArray<v’>

v’ is ValueTuple<int, int, int, int>

so this could lead to more discussion about how we should display long type or generic type when we usually do inline. I believe anonymous type we never do inline since there is no syntax to inline it.

some cases, we show original definition + type argument type (like when one hover “var” in var a = new List), so I am not talking about these cases but the cases where we show types inline (like when one hover “a” in var a = new List)

  • heejae

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 17, 2018

We probably need to first see whether this is what we want. And find out why we went to inline route rather than anonymous type route.

Pretty sure people have considered both and choose one (inline)

I don't think we did here. We simply didn't do anything special because we had a sensible way of representing tuples. And because SymbolDisplay does work for them without any effort. We had to do the anonymous type work because there is no canonical way of SymbolDisplay doing anonymous-types. And we were not going to use the way the compiler represents things (where it literally spits out things like <anonymous type ...> in sigs.

I was the person who did the anon-type display back in the pre-roslyn and roslyn days. The reason for it was very specific, the display simply got too large and cluttered and redundant when you inlined the anony-type sigs. We really wanted people to be able to distinguish the signatures from the individual types, and so that display was borne.

The same concerns apply to tuples (though they are less important IMO due to #29949 (comment)). But the same reasoning and justification applies to tuples just as for anon-types.

@heejaechang
Copy link
Contributor

so, if we never had a design meeting on how to display tuple in symbol display, probably we should have one then since now we have a request to do it differently.

@heejaechang heejaechang added the Need Design Review The end user experience design needs to be reviewed and approved. label Sep 17, 2018
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 17, 2018

so this could lead to more discussion about how we should display long type or generic type when we usually do inline.

Right. But in general it's an antipattern to have long generic types. We did an analysis at one point and i believe 99.9% of all generic types had 2 type parameters or less. And it was somehting like 99.999 for 3 type params or less. So we never bothered optimizing for that.

One thing though i don't know if we measured was nesting depth with generics. i.e. how often someone has nested generics, and how big the total instantiated type is. It might be that our initial assessment of just type arg count didn't properly reflect how common or relevant these sorts of scenarios are. Def something to think about.

@heejaechang
Copy link
Contributor

heejaechang commented Sep 17, 2018

sure. got your opinion. so, let's hear other's opinion.

@CyrusNajmabadi
Copy link
Member

so, if we never had a design meeting on how to display tuple in symbol display, probably we should have one then since now we have a request to do it differently.

Agreed. I'm just giving historical context on the design thinking here, as well as a view of relative priority.

IMO, it's fairly reasonable to assume that people will have tuples of 3-4ish values, and at that point it does start getting pretty cluttered as we see these in signatures. For generics, given the data indicates things are almost never above 2-3. So i would prioritize this as:

  1. anon types. necessary, very common to have many members. no good way to even represent anon-types.
  2. tuple types. probably more relevant as they become more common and people put more members in them. Probably relevant enough as expected tuple sizes already get unwieldy with these scenarios.
  3. generics. definitely interesting. gets relevant more due to nesting versus length of type parameters themselves. i.e. if i have Dictionary<(int, string, bool), List<List<string>>> do we start feeling like this needs special handling?

@heejaechang
Copy link
Contributor

my opinion is rather than making it complex. just take a simple approach.

we already have 2 modes. one is showing original definition + type argument vs inline.

so for generic, if inline makes definition too long, just turn off inline mode and always use showing original definition + type argument mode.

since we already have code for both, all we probably need is condition that says if inline is too long, use the other code path.

and leave non generic one (type just too long including tuple) as they are until we get more feedbacks on it.

@CyrusNajmabadi
Copy link
Member

so for generic, if inline makes definition too long, just turn off inline mode and always use showing original definition + type argument mode.

I'd definitely like to see that. It would be interesting to get a sense of what we think of as "too long".

@heejaechang
Copy link
Contributor

70 char might be good start? since some might not be able to see the string on screen anymore since it is too long.

@CyrusNajmabadi
Copy link
Member

@heejaechang Yup yup. I'm sure we can hav an internal option, then just whip up a few examples at different lengths to see what feels like a good balance of clarity and comfort. I look forward to see any work here! :)

@jinujoseph jinujoseph modified the milestones: 16.0, 16.0.P2 Sep 18, 2018
@ManishJayaswal
Copy link
Contributor

@CyrusNajmabadi - long tuples are common when using ML.NET. They recently did a bunch of work to make their model strongly typed for better intellisense. If cost is not significant then we should probably do this.

@CyrusNajmabadi
Copy link
Member

@ManishJayaswal It would be useful to drive this work through some real world examples. Do we have some we can use for ml.net? We can then validate that any potential ideas here do work out well for those cases.

@ManishJayaswal
Copy link
Contributor

@TomFinley - can you share a real world case with us here. Based on the other email thread, I gather that you guys are hitting this frequently while working with ML.NET?

@TomFinley
Copy link
Author

TomFinley commented Sep 25, 2018

HI @ManishJayaswal sorry didn't see this till now.

So what happens in ML.NET is that you have some dataset, and then you apply featurization to certain columns of your dataset, to produce new records, upon which you apply more featurization, and yet more featurization, and so forth. Your dataset starts in a pretty raw form with some heterogeneous data, and then you eventually over several steps translate into some sort of "regular" format (usually, a single scalar label, and a float vector of features, at least, for most ML algorithms).

The trouble comes when we're doing statically typed pipelines, where we have a dataset with a certain schema, then we apply featurizers to get new data, then apply more featurization, and so forth, until we can apply the learning algorithm, and this altogether once fit on data comprises a model. Think of the .Append as being somewhat analogous to .Select in LINQ. (Not quite, but close enough. My expectation is that if we can make LINQ intellisense with value-tuples work well, the same innovations will just carry over to ML.NET.)

We don't want to force the user to explicitly declare types at each stage, because that would discourage people from experimenting with how they featurize their data (this iterative experimental process is very important in actually using ML, so any barriers to that are anathematic). We can allow either anonymous types, or value-tuples. Anonymous types appear to work well and produce sensible intellisense, but value-tuples are an absolute nightmare.

So for example, I was translating a pipeline someone gave me. That had something on the order of I guess 30 columns of raw data. The "pipeline" I only list a small part of to give the idea, but there are perhaps four stages of it. Each "intellisense" over that lengthly value-tuple is of ridiculous size to the point where intellisense's usefulness is compromised.

Now, intellisense is still useful when you actually get into the applications on the individual columns, so if a user can get that far they're probably OK. But in the middle of getting there they run across the intellisense for this .Append method.

var env = new ConsoleEnvironment(seed: 0);
var dataPath = GetDataPath(TestDatasets.generatedRegressionDataset.trainFilename);
var dataSource = new MultiFileSource(dataPath);

var reader = TextLoader.CreateReader(env, c => (
    incidentId: c.LoadText(0), title: c.LoadText(1), status: c.LoadText(2),
    severity: c.LoadFloat(3), sourceName: c.LoadText(5),
    sourceCreatedBy: c.LoadText(6), siloId: c.LoadText(7),
    originatingTenantId: c.LoadText(8), owningTenantName: c.LoadText(9),
    owningTenantId: c.LoadFloat(10), owningTeamName: c.LoadText(11),
    owningTeamId: c.LoadFloat(12), firstOwningTeamId: c.LoadText(13),
    secondOwningTeamId: c.LoadText(14), thirdOwningTeamId: c.LoadText(15),
    responsibleTenantId: c.LoadText(16), incidentType: c.LoadText(17),
    impactStartDate: c.LoadText(18), customerName: c.LoadText(19),
    impactedScenarios: c.LoadText(20), isCustomerImpacting: c.LoadText(21),
    keywords: c.LoadText(22), occurringDatacenter: c.LoadText(23),
    occurringDeviceGroup: c.LoadText(24), occurringDeviceName: c.LoadText(25),
    occurringServiceInstanceId: c.LoadText(26), raisingDatacenter: c.LoadText(27),
    raisingDeviceGroup: c.LoadText(28), raisingDeviceName: c.LoadText(29), 
    raisingServiceInstanceId: c.LoadText(30), description: c.LoadText(31)),
        allowQuoting: false);

var est = reader.MakeNewEstimator().Append(r => (
    label: r.owningTeamId.ToKey(),
    labelTenant: r.owningTenantId.ToKey(),
    ldaText: r.description.FeaturizeText(advancedSettings: s =>
        { s.KeepPunctuations = false; s.KeepNumbers = false; s.OutputTokens = true; })));

/// More featurization down here, based on more `Append`.

If I hover over, say, that .Append, the intellisense isn't great, as you can imagine.

@CyrusNajmabadi
Copy link
Member

Canyou supply enough code to make that a compilable sample? Thanks!

@TomFinley
Copy link
Author

TomFinley commented Sep 25, 2018

Sure, edited above, w.r.t. commit hash a02807c7a805b72ef12970a37279c8cec4ea667d (current master at time I am writing now)... the usings are these.

using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.RunTests;
using Microsoft.ML.Runtime.Training;
using Microsoft.ML.StaticPipe;

@TomFinley
Copy link
Author

I hasten to add though that this is not ML.NET specific really. Just using something as simple as LINQ with value-tuples easily shows the flaws in current tooling.

@CyrusNajmabadi
Copy link
Member

I hasten to add though that this is not ML.NET specific really. Just using something as simple as LINQ with value-tuples easily shows the flaws in current tooling.

Hi Tom, my point above was that intellisense is designed around common coding patterns, and doesn't normally care about pathological stuff. So, even with Linq, it's not likely the case that massive tuples are being created. Even here in Roslyn (where people are tuple-happy), we rarely see tuples more than 3 elements large. This is mainly due to the readability and maintainability of the code itself, not because of intellisense failovers.

That's why the ML example is so useful. It makes a reasonable case that this isn't just some pathological, unlikely-to-happen, example. But it's really something that we would expect a reasonably sized group of people to do quite a bit.

If ML.net makes working with tuples more mainstream and common, then that definitely encourages us to improve intellisense here. So far, that hasn't been the case with Linq+tuples (as opposed to Linq+anon-types, which was a huge motivation here).

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 30, 2021

Design notes 8/30/21:

  1. We could definitely try this out. But we should probably limit it to when the signature has >=2 uses of that particular tuple type.
  2. If we did this, would we want an option here to allow the user to turn this off?
  3. We should resurrect jnm2's PR to make delegates work like anonymous types.

--

We will update the UI so that if a tuple shows up 2 or more places that we use the anonymous types: location to show it's signature.

@DoctorKrolic
Copy link
Contributor

@CyrusNajmabadi You fixed this issue in #56025, so it should be closed now

@CyrusNajmabadi
Copy link
Member

@TomFinley can you try our latest release and let me know if it's better for your use case? Thanks!

@CyrusNajmabadi
Copy link
Member

This work was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-Continuous Improvement Design Notes Need Design Review The end user experience design needs to be reviewed and approved.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants