Roslyn: Avoid hiding all IsCompilerGenerated fields in MemberExpansion

[stephentoub]

MemberExpansion currently hides all members with compiler-generated names.
https://github.com/dotnet/roslyn/blob/6847f1e5a909395aae9456e8f366cbf4deb86b69/src/ExpressionEvaluator/Core/Source/ResultProvider/Expansion/MemberExpansion.cs#L73-L79

Why is that valuable? I'm using a debugger to examine the state of my program, I should be able to look at the state of my program, even if it's stored in compiler-named fields/types, e.g. the state of my async method as stored in the async state machine object.

[jcouv]

Tagging @cston

[r-ramesh]

Perhaps a member group like [Compiler generated] might be useful (if JMC is switched off)?

[tmat]

@stephentoub The EE presents data stored in the compiler generated fields in a form that makes sense to a person who understands C# language but doesn't necessarily know how the compiler represents language constructs in the metadata. For example, variables that are hoisted in state machines and closures are presented as if they were "regular" locals, even though they are actually fields.

I think it's reasonable to hide compiler generated members, since they are implementation details whose meaning only experts would understand. For 99% users it'd be just useless clutter. That said, I can see the value of being able to see raw structure.

One simple tweak we can make is that we display compiler generated members if the containing type is also compiler generated.

@r-ramesh I would not add [Compiler Generated] group -- that would add too much clutter.

[stephentoub]

For example, variables that are hoisted in state machines and closures are presented as if they were "regular" locals, even though they are actually fields.

Paste this code into a desktop console app in VS, build as debug, and F5:
```C#
using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

class Program
{
static void Main() => FooAsync().GetAwaiter().GetResult();

static async Task FooAsync()
{
    int i = 42;
    await new ExtractAwaiter();
    Console.WriteLine(i);
}

class ExtractAwaiter : INotifyCompletion
{
    public ExtractAwaiter GetAwaiter() => this;
    public bool IsCompleted => false;
    public void OnCompleted(Action action)
    {
        object target = action.Target;
        Debugger.Break();
    }
    public void GetResult() { }
}

}

When it breaks in, evaluate this in the watch window:

((System.Runtime.CompilerServices.AsyncMethodBuilderCore.MoveNextRunner)((System.Runtime.CompilerServices.AsyncMethodBuilderCore.ContinuationWrapper)target).m_continuation.Target).m_stateMachine
`` According to "variables that are hoisted in state machines and closures are presented as if they were "regular" locals", I should be able to expand that and see the hoistedi` local, but it's not expandable.

I think it's reasonable to hide compiler generated members

It's a reasonable default. There needs to be an option to override it. I should be able to examine the data in my own program without having to have a constant fight with the debugger.

[r-ramesh]

@jinujoseph @ivanbasov @tmat Hi folks, could we fix this issue? I'm revisiting our Tasks experience in VS and this is a big pain point.
Could we implement this as @tmat suggested: One simple tweak we can make is that we display compiler generated members if the containing type is also compiler generated.

[stephentoub]

@jinujoseph / @ivanbasov, could we look at fixing this for the near future? This is also an issue while trying to debug async iterators.

[ivanbasov]

@stephentoub, @r-ramesh I do not like the idea. User expects to see variables defined by user. Showing compiler external state seems to bring a confusion. Compiler implementation may change and state of compiler generated variables may change as well. For our internal purposes, for people interacting with debugger, showing compiler generated variables could be interesting and helpful. However, I'm not sure the same holds for our customers.

@cston what do you think? We also should wait for @tmat to respond.

[stephentoub]

Showing compiler external state seems to bring a confusion.

I have no problem with us making it less visible / more effort to get to, but as far as I can tell, there's literally no way to do it in the debugger today. That I have to stop using Visual Studio and instead use WinDBG+SOS is a real problem. This isn't just about internal purposes: there's real state in the program here that's of value, and the debugger shouldn't make it impossible to see.

[CyrusNajmabadi]

This sounds like a very good optionto have in the debugger. Have things clean by default, but have an option to show everything if a dev has advanced enough needs to warrant it. It would hopefully not be too hard to just have an option that can flow to this point to be checked by the EE.

[cston]

I like the idea of displaying compiler generated fields, perhaps through an option, or by default for compiler generated types as @tmat suggested.

[stephentoub]

@ivanbasov, here's a simple example of how it's valuable, not just internally.

Consider this C# program:
```C#
using System.Diagnostics;
using System.IO;
using System.IO.Pipes;
using System.Threading;
using System.Threading.Tasks;

class Program
{
static void Main()
{
using (var server = new AnonymousPipeServerStream(PipeDirection.In))
using (var client = new AnonymousPipeClientStream(PipeDirection.Out, server.GetClientHandleAsString()))
{
Task t = CopyAsync(server, Stream.Null, 100);

        client.Write(new byte[] { 1, 2, 3 });
        client.Write(new byte[] { 4, 5, 6 });

        Thread.Sleep(1000);
        Debugger.Break();
    }
}

static async Task<long> CopyAsync(Stream source, Stream destination, int bufferSize)
{
    byte[] buffer = new byte[bufferSize];
    int bytesRead;
    long totalRead = 0;
    while ((bytesRead = await source.ReadAsync(buffer)) != 0)
    {
        totalRead += bytesRead;
        await destination.WriteAsync(buffer, 0, bytesRead);
    }
    return totalRead;
}

}
```
When I run this and look at it in the debugger, given the Task returned from the async method, I can actually navigate to and get to the state associated with that async method, which is a valuable debugging aid, but because it hides compiler-generated state, it only shows me the parameters to the method:

This means that I can't determine where it is in the method (which I could learn from the state field), how many bytes have been read thus far (which I could learn from the totalRead local that's been lifted), etc. I'm forced to drop out of Visual Studio and use WinDBG + SOS instead.

at which point I can learn all that info and more.

[noahfalk]

User expects to see variables defined by user. Showing compiler external state seems to bring a confusion.

I think this is true in some cases, but also very limiting in other cases. The VS debugger has the 'Just My Code' option - if the user were to turn that off I'd treat that as a clear indication that the user is asking the debugger to reveal lower abstraction levels that were not self-authored. I'd propose showing compiler generated fields in this case.

[tmat]

I don't think "Just My Code" should be used here to indicate that the user wants to see the internals. Customers turn JMC off when they want to step into 3rd party libraries. But they are rarely interested in state machines and other generated code.

I don't think we need any switch though. See my comment above:

One simple tweak we can make is that we display compiler generated members if the containing type is also compiler generated.

[noahfalk]

If the compiler generated type tweak is enough to solve this instance I'm happy with that plan too. I agree that attaching it the to the JMC switch is a larger change and would thus have some additional risk of customers disliking the policy change.

[tmat]

Another feature we can consider (once we allow expanding compiler-generated type) is to add $StateMachine pseudo-variable that evaluates to the state machine object within a MoveNext method of a state machine. You can then use this variable to refer to and inspect the state machine. The variable would be strongly typed to the exact type of the state machine.

[tmat]

Looking into this a bit more, turns out we already have hidden modifier that instructs the EE to display all members regardless of visibility. We can generalize it to show compiler-generated members as well. I don't think the modifier is used too much, so displaying compiler generated stuff shouldn't be detrimental. The syntax to print out all fields of the state machine would be <expr>, hidden where <expr> is an expression that evaluates to a state machine object.

We actually do (accidentally, I think) display some state of the machine already - if the kick-off method had any parameters we would emit them unmangled in C# (not in VB) and they would just show up as members of the state machine object. So I think we would also want to display other fields that correspond to user-defined variables on the state machine object using their unmangled names without hidden modifier specified. We would only hide compiler generated helper fields such as the machine state, spilled variables, awaiters, etc.

@r-ramesh Sounds good?

[stephentoub]

The syntax to print out all fields of the state machine would be , hidden where is an expression that evaluates to a state machine object.

While an improvement, that's still pretty cumbersome: it means that when I'm navigating through an object hierarchy, I can't just view the data there, and instead need to copy the expression, break out of that visualization, and start again with the copied expression, modified with ", hidden".

Either it should always be shown, or I should be able to opt-in to say "I want this to always be shown". The debugger should not be in the business of guessing my intentions and then enforcing a random policy about what I can and cannot see. Heuristics are fine, but I should be able to override them when it gets things wrong, and without having to perform a complicated gesture each time.

[r-ramesh]

I think the original solution of One simple tweak we can make is that we display compiler generated members if the containing type is also compiler generated. is the best one.

The issue with the "hidden" format specifier is that it is not clear when things are being hidden, and when to actually use the specifier.

[stephentoub]

if the containing type is also compiler generated

That's a good step, but I don't think it's sufficient. Take the example in https://github.com/dotnet/roslyn/issues/22428#issuecomment-440115510. In most cases, the type being explored here is an AsyncStateMachineBox<T>, where T is the state machine type generated by the compiler, so assuming "if the containing type is also compiler generated" factors in the compiler-generated nature of generic type parameters, then it would cover that. However, there are times where instead of getting an AsyncStateMachineBox<T>, you'd actually have an AsyncStateMachineBox<IAsyncStateMachine>, and nothing about that is compiler-generated; in that case, the StateMachine field will be a boxed object for the compiler-generated state machine, and this wouldn't cover it, right?

[tmat]

I think the original solution of One simple tweak we can make is that we display compiler generated members if the containing type is also compiler generated. is the best one.

@r-ramesh
As it turns out it does not work for iterators. Iterator state machines are compiler generated types but we don't want to display their compiler generated members by default.

@stephentoub The AsyncStateMachineBox<T> case would work since we are talking about the members of the state machine that are currently hidden and their declaring type (the state machine type), which is also compiler generated. Members of AsyncStateMachineBox<T> are already shown.

[stephentoub]

The AsyncStateMachineBox case would work since we are talking about the members of the state machine that are currently hidden and their declaring type (the state machine type), which is also compiler generated.

Ah, I may just have been misunderstanding. If I have type A with a field of type B, and B is compiler-generated, when I'm looking at A in the watch window and I try to expand its field of type B, what matters is the type of B, not A? If the field of type B will happily expand, then yes, that addresses my concern. I thought you were saying that A would need to be compiler-generated as well for the field of type B to expand (and that's insufficient).

[tmat]

The type B always expands if it has any _visible_ members. The problem is that we consider compiler generated members _not visible_. The tweak I proposed would consider them _visible_ if their declaring type (B) was compiler generated. But as I said this doesn't work quite well in iterators. So I'm now considering another approach, which is unfortunately not a simple tweak anymore. But it will likely give you exactly what you want :)

[stephentoub]

But it will likely give you exactly what you want :)

:smile:

[tmat]

A new proposal - seems like this will not be a complex change.

The debugger has an option

which is also available via raw modifier (<expr>, raw).

The meaning for managed objects right now is to disable using debugger visualizer attributes on the object instance. For example, an instance of Dictionary<string,string> is displayed like so:

The proposal is to show all compiler-generated members in this view.

An iterator looks like so:

An async state machine like so:

[stephentoub]

The proposal is to show all compiler-generated members in this view.

That sounds like a reasonable compromise I guess.

[noahfalk]

As it turns out it does not work for iterators. Iterator state machines are compiler generated types but we don't want to display their compiler generated members by default.

Can you explain the rationale behind this? If I imagine a developer inputting the expression "i1", I'd expect they want to see exactly the output you have for "i1,raw". Were you thinking of this scenario exactly or is it some other use-case that is problematic?

I'm not necessarily opposed to the proposal above, but the earlier one sounded better.