Bug C# Record: Autogenerated ToString() generates stack overflow for to records linking to each other

[PeterHuberSg]

Version Used:
net5.0
Steps to Reproduce:
Console Application

using System;

namespace RecordToStringProblem {
  record Parent() {
    public Child? Child;
  }

  record Child() {
    public Parent Parent;

    public Child(Parent parent):this() {
      Parent = parent;
      parent.Child = this;
    }
  }

  class Program {
    static void Main(string[] args) {
      var parent = new Parent();
      var child = new Child(parent);
      Console.WriteLine("Hello World!");
    }
  }
}

1. Set breakpoint on Console.WriteLine("Hello World!");
2. Run debugger
3. Hover mouse over parent => debugging session ends with error: "The targeted process exited with code ..."

Expected Behavior:
The ToString() method automatically created by the compiler should not create a stack overflow
Actual Behavior:
The ToString() method automatically created by the compiler does create a stack overflow

The stacktrace shows:
at RecordToStringProblem.Child.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Child.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Parent.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Parent.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Child.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Child.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Parent.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Parent.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Child.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Child.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Parent.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Parent.ToString()
at System.Text.StringBuilder.Append(System.Object)
at RecordToStringProblem.Child.PrintMembers(System.Text.StringBuilder)
at RecordToStringProblem.Child.ToString()
at RecordToStringProblem.Program.Main(System.String[])

Obviously, Child.ToString() calls Parent.ToString() calls Child.ToString() calls Parent.ToString() calls ...

This raises the question:

Is it legal in C# for 2 records to link to each other ?

• If it is legal, the compiler should not create code which results in a stack overflow
• If it is not legal, the compiler should display an error when 2 records link to each other.

Since this is my first time ever reporting a bug on Github, I am not sure if I have posted this bug in the right place, if I have provided all information needed nor do I have a suggestion what really needs to be changed in the compiler. I just hope that this is taken seriously and fixed, even if no other people report this bug.

[Youssef1313]

Thanks for reporting @PeterHuberSg. Yes this is the correct repository, however, this behavior is "By Design". See dotnet/roslyn#49396.

[alrz]

This issue is reported so many times at this point I think it deserves a warning and a suggestion to override ToString.

[PeterHuberSg]

I read dotnet/roslyn#49396 and I must say this feels very strange. The problem with this "by design" feature is that VS just crashes, without a proper error message (it only says: "The targeted process exited with code ..."). I only figured out what the problem was when I wrote a Console Application to report this bug and the Console Application wrote the recursive call stack to my monitor.

It is simply not right. If ToString() needs to be overriden, otherwise the program cannot be debugged, then the compiler MUST refuse to compile and emit an error when a record is used cyclical (like referencing itself) and ToString() is not overriden.

[RikkiGibson]

Perhaps if a cycle is detected across mutable record fields, and ToString() is not manually overridden somewhere in the cycle, then we report a warning.

[CyrusNajmabadi]

The problem with this "by design" feature is that VS just crashes,

Can you provide a repro of htat happening. When i try pasting the above code into VS it worked just fine and VS didn't crash.

[CyrusNajmabadi]

Perhaps if a cycle is detected across mutable record fields, and ToString() is not manually overridden somewhere in the cycle, then we report a warning.

That seems excessive. For example, it's possible that this is fine because your call to ToString one of your members will end up virtually calling into an overridden tostring that you're not aware of at this point. i.e. the static ToString invocation doesn't match the runtime dispatch since this is all virtual.

--

I would not be opposed to an out of band analyzer warning on this that users could choose to opt into if they want.

[RikkiGibson]

To see the issue you have to use the debugger or call ToString in your program and run the program.

For example, it's possible that this is fine because your call to ToString one of your members will end up virtually calling into an overridden tostring that you're not aware of at this point.

I would definitely favor giving a warning in a scenario like this:

record R1 // warning: R1 has a mutable cyclic reference via property 'R2' but does not override ToString()
{
    public R2 R2 { get; set; }
}

record R2 // warning: R1 has a mutable cyclic reference via property 'R1' but does not override ToString()
{
    public R1 R1 { get; set; }
}

record R3 : R2
{
    public override string ToString() => "R3";
}

In this case, require the user to override ToString() in one of the base records to make the warning go away. Otherwise we are asking the user to know where the landmine is and to please step around it.

There are some cases where I'd agree that such warnings might be too onerous, such as when a record contains a mutable field of type object. I'm not so sure what to do there.

I feel like the failure mode for this is bad enough that the warning is warranted for any C# version that uses records. But I can be fine with it being in wave 6.

[CyrusNajmabadi]

That case is precisely why i wouldn't want a warning here. I can easily see a record hierarchy with members in the inheritance hierarchy that are pointed at, where only the leaves overrde teh functionality.

Also, i don't really see waht this has to do with ToString. Everything about the record has this potential issue (like Equals and GetHashCode).

We explicitly thought about this in the design and decided this was the approach we wanted. If we felt this was bad, we would have required this to be an error up front. If users want a more restricted view of things, then i woudl say an analyzer is appropriate.

[RikkiGibson]

I can easily see a record hierarchy with members in the inheritance hierarchy that are pointed at, where only the leaves overrde teh functionality.

I agree and that's fine, but if the base record is not abstract, it's extremely plausible that a graph would be created using the base record even if the user didn't intend it. The cost of removing the warning is very cheap: just declare a member. A valid implementation using base.ToString() could be added automatically.

Similarly, for a long time we have warned if Equals is overridden in a type but not GetHashCode. Users may never intend to use the type as a dictionary key, but we give the warning regardless because we consider the failure mode to be sufficiently bad.

Also, i don't really see waht this has to do with ToString. Everything about the record has this potential issue (like Equals and GetHashCode).

ToString is a worse case than other generated members because all it takes to crash the debugger is to hover on the wrong thing, even if you were never using the "badly behaving" members in your code. However, I actually think your point is a decent reason to extend the warning also to require override of GetHashCode() and perhaps other implicit members on records (I wasn't able to reproduce the issue with Equals, perhaps because of the reference equality check that Equals performs).

[alrz]

I can easily see a record hierarchy with members in the inheritance hierarchy that are pointed at

The code looks benign as far as I can see it in the source, I think it's the compiler's job to show problems with the code, whether or not I could notice it or not. The endless recursion is generated by the compiler, so that particular hierarchy should definitely produce a warning. I'd agree that that would not be an "invalid program" but the generated ToString is in fact erroneous right off the bat.

If there's a bug in my overridden ToString, that'd be my fault, but if I'm going to get invalid code by simply declaring such hierarchy (which is completely valid for classes), I want the compiler to either handle it gracefully, or at least warn that this is going to throw.

[PeterHuberSg]

My original code was a a unti test using Microsoft.VisualStudio.TestTools.UnitTesting;

When I run the test, it worked fine. But when I set a breakpoint and just put the mouse over an instance of those records, the debugging session stopped abruptly with the error message:

The error message does not give any hint and I am even not able to see the call stack, because the debugging session has ended. At this point, I had no idea what was the problem. For posting the bug here, I wrote a Console application and there I could see the call stack on my screen and the problem became obvious.

Regarding the discussion if the compiler should show an error or not:

What is the advantage of strongly typed languages ? The compiler can find more programming errors and show them to the developer. This principle is very important. Most users of records are new to them and they will not know the intrinsic details when overriding ToString() is required. Each year, C# gets more complex. I am using C# since 15 years exclusively, I love it, but I still get sometimes caught in the differences between classes and strucs. Now there are classes, strucs and records and they all behave differently. I guess this cannot be avoided, but the compiler should give all the help it can.

I feel the compiler should not create a method that can impossible run. Therefore, it should show at least an error, not just a warning. But better would be, of course, if the compiler would recognise the cyclic reference problem and adjust the created code as necessary.

Sorry to say that, but it sounds kind of lazy to say it is a design feature that the compiler compiles invalid code and the developer must figure out himself why the debugger suddenly stops without providing any infromation what could be wrong.

The fact that my unit test run successfully, even the ToString() would not work, shows how dangerous this is. The debugging problem could have occured only in a few years for completely different people and it would be extremely difficult for them to figure out what had happened. That I now need to unit test methods autocreated by the compiler is bad.

By the way, the records were not part of the code under test, but I used them to keep track of some test data. I really should not need to write tests if ToString() works if I never intend to use it.

[CyrusNajmabadi]

the debugging session stopped abruptly with the error message:

This sounds like an issue I would want fixed in the debugger as well :-)

[CyrusNajmabadi]

Regarding the discussion if the compiler should show an error or not:

I'm not opposed to an error. As I've said, what I think is the right thing here is an analyzer.

[PeterHuberSg]

@CyrusNajmabadi

The problem with this "by design" feature is that VS just crashes,

Can you provide a repro of htat happening. When i try pasting the above code into VS it worked just fine and VS didn't crash.

I read https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record

There is no hint at all that there could be any problem if 2 record types link to each other. This guarantees that countless developers will run into stack overflows and abruptly stopping debugging sessions without any good way to figure out what has happened. If C# wants to be a safe language, it must inform the developer during compilation or better even, autocreate code which will not cause that problem in the first place.

[CyrusNajmabadi]

This guarantees that countless developers will run into stack overflows and abruptly stopping debugging sessions

That is an aspect of the debugger. I don't see why that has be the case. And i don't necessarily think the language should change due this quirk of how that works. I'd be fine with a debugger fix that addressed this by simply stating that the ToString here was "ToString terminated due to too deep a callstack".

[PeterHuberSg]

The main point is not that the debugger stops, but that the reason given for not correcting the problem is that it is not a bug but a feature. The specification (the link I provided) says that record will behave like a class. There is nowhere mentioned that 2 records referencing each other requires that ToSring(0 gets overridden and that otherwise a stack overflow will occur. If this is by design, this should be at least clearly stated ! Anyway, this is so contrary to the design of C#, that the compiler should find as many problems as possible and show them. But in this case, the compiler autogenerates code that can impossibly run and the developer's job is to rectify the problem, but nobody tells the developer that this problem exists.

[CyrusNajmabadi]

If this is by design, this should be at least clearly stated !

I have no problem with additional information being provided that can clarify this.

[CyrusNajmabadi]

But in this case, the compiler autogenerates code that can impossibly run

The compiler generates code which can crash at runtime. This happens in many places in the language. Before records you could trivially encounter this with other recursive structures. For example, if you had recursive anonymous types, or tuples.

As I mentioned, I would be fine with an analyzer here calling this out, with appropriate knobs to control it. You seem to think I'm opposed to this information being provided. I am not. I simply am stating where I think the fix should appropriately go.

[svick]

What if the generated ToString() called RuntimeHelpers.EnsureSufficientExecutionStack()? This would turn the StackOverflowException into InsufficientExecutionStackException, which should be much easier to diagnose.

[jcouv]

The call to EnsureSufficientExecutionStack was added in dotnet/roslyn#54967
This can probably be closed.

[RikkiGibson]

This issue tracks introducing an analyzer to report warnings on some cases where records with synthesized members have cycles. I think there is still merit in such an analyzer.