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

Stackoverflow on circular referenced records. #8

Open
WhiteBlackGoose opened this issue Jun 15, 2021 · 7 comments
Open

Stackoverflow on circular referenced records. #8

WhiteBlackGoose opened this issue Jun 15, 2021 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@WhiteBlackGoose
Copy link
Contributor

WhiteBlackGoose commented Jun 15, 2021

First, awesome project!

Second... yes, it's the first thing I tried in it. Here's the repro:

record A(int P1) { public A Node { get; set; } }
var a = new A(5);
a.Node = a;
a

I suspect it's a problem of dotnet/interactive. Should I redirect the issue there?

@jonsequitur jonsequitur added the bug Something isn't working label Jun 15, 2021
@jonsequitur
Copy link
Owner

This is actually a bug here. .NET Interactive will handle this correctly because the default formatters will check for recursion correctly. dotnet-repl has different formatter implementations targeting Spectre.Console.

@jonsequitur
Copy link
Owner

This is fixed in version 0.1.30.

@jonsequitur
Copy link
Owner

jonsequitur commented Jun 30, 2021

This is actually not fixed for records.

The underlying issue here is one with ToString when records have reference cycles. See: dotnet/roslyn-analyzers#5068

@jonsequitur jonsequitur reopened this Jun 30, 2021
@jonsequitur jonsequitur changed the title Stackoverflow on circular referenced objects. Stackoverflow on circular referenced records. Jun 30, 2021
@WhiteBlackGoose
Copy link
Contributor Author

@jonsequitur it's more than that. It's not ToString which causes it. Maybe you're calling PrintMembers? Otherwise, here's my output for my lib's records:
image

@jonsequitur
Copy link
Owner

The formatters do recursive evaluation but they also have built-in recursion controls, e.g.:

image

Does this repro on the latest version and does it repro in a .NET Interactive notebook?

How can I repro it?

@WhiteBlackGoose
Copy link
Contributor Author

Updated the tool to 1.44 version.
image

You are using classes, and I'm using record. In record it is by default overriden to printing its members recurisively => circular reference implies an SO.

But if I add my own overriding,

record A(int P1) { public A Node { get; set; } public override string ToString() => "Quack!"; }
var a = new A(5);
a.Node = a;
a

it results in a similar output you have
image

Now, if I override PrintMembres too

sealed record A(int P1) { public A Node { get; set; } public override string ToString() => "Quack!";  bool PrintMembers(System.Text.StringBuilder sb) => true; }
var a = new A(5);
a.Node = a;
a.ToString()

image

But if I don't do ToString in the end, I get

image

My suggestion is to simply call ToString on a record. Yes, you would get an SO if the author of the record didn't override the necessary methods, but at least it would coincide with the expectations.

@jonsequitur
Copy link
Owner

Here's a non-recursive example comparing the formatter to the ToString output:

image

The formatter output is definitely a little redundant.

Let's look at the larger goal of using formatters instead of ToString. We wanted to provide something richer than whatever the original developer decided to use ToString for. I agree that records have a more useful default ToString implementation than classes, so maybe a different behavior is warranted in this case. The general expectation in interactive though (analogous for example to F# printers) is that ToString is typically not sufficient.

Take recursive formatting. If you want to format a record with more complex members and those records are of types where you also have custom formatting rules, those will be in effect with the formatter approach:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants