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.

Sign up for GitHub

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

Struct layout check – break an infinite generic expansion cycle that doesn’t include the target struct (a cycle at the end of the path) #75702

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Struct layout check – break an infinite generic expansion cycle that doesn’t include the target struct (a cycle at the end of the path) #75702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c98a05a
Struct layout check – break an infinite generic expansion cycle that …
AlekseyTs Nov 1, 2024
0928410
Add tests
AlekseyTs Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,30 +65,50 @@ internal static bool StructDependsOn(NamedTypeSymbol depends, NamedTypeSymbol on
Debug.Assert((object)on != null);
Debug.Assert(on.IsDefinition);

var hs = PooledHashSet<Symbol>.GetInstance();
StructDependsClosure(depends, hs, on);
var hs = PooledHashSet<NamedTypeSymbol>.GetInstance();
var typesWithCycle = PooledHashSet<NamedTypeSymbol>.GetInstance();
StructDependsClosure(depends, hs, typesWithCycle, ConsList<NamedTypeSymbol>.Empty.Prepend(on));

var result = hs.Contains(on);
var result = typesWithCycle.Contains(on);
typesWithCycle.Free();
hs.Free();

return result;
}

private static void StructDependsClosure(NamedTypeSymbol type, HashSet<Symbol> partialClosure, NamedTypeSymbol on)
private static void StructDependsClosure(NamedTypeSymbol type, HashSet<NamedTypeSymbol> partialClosure, HashSet<NamedTypeSymbol> typesWithCycle, ConsList<NamedTypeSymbol> on)
{
Debug.Assert((object)type != null);

if ((object)type.OriginalDefinition == on)
if (typesWithCycle.Contains(type.OriginalDefinition))
{
return;
}

if (on.ContainsReference(type.OriginalDefinition))
{
// found a possibly expanding cycle, for example
// struct X<T> { public T t; }
// struct W<T> { X<W<W<T>>> x; }
// while not explicitly forbidden by the spec, it should be.
partialClosure.Add(on);
typesWithCycle.Add(type.OriginalDefinition);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typesWithCycle.Add

Are there any examples where typesWithCycle willl have more than one item?

Copy link
Contributor Author

@AlekseyTs AlekseyTs Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any examples where typesWithCycle willl have more than one item?

I see nothing preventing that. If that is what you are asking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's possible, it would be useful to have a test that has multiple entries in typesWithCycle, to prevent us from changing this to a single value in the future.

return;
}

if (partialClosure.Add(type))
{
if (!type.IsDefinition)
{
// First, visit type as a definition in order to detect the fact that it itself has a cycle.
// This prevents us from going into an infinite generic expansion while visiting constructed form
// of the type below.
visitFields(type.OriginalDefinition, partialClosure, typesWithCycle, on.Prepend(type.OriginalDefinition));
}

visitFields(type, partialClosure, typesWithCycle, on);
}

static void visitFields(NamedTypeSymbol type, HashSet<NamedTypeSymbol> partialClosure, HashSet<NamedTypeSymbol> typesWithCycle, ConsList<NamedTypeSymbol> on)
{
foreach (var member in type.GetMembersUnordered())
{
Expand All @@ -99,7 +119,7 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet<Symbol> p
continue;
}

StructDependsClosure((NamedTypeSymbol)fieldType, partialClosure, on);
StructDependsClosure((NamedTypeSymbol)fieldType, partialClosure, typesWithCycle, on);
}
}
}
Expand Down
310 changes: 310 additions & 0 deletions src/Compilers/CSharp/Test/Emit3/FlowAnalysis/StructTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,316 @@ public void GenericStructWithPropertyUsingStruct()
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S<T>.P", "S<T[]>?").WithLocation(3, 13));
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void InstanceMemberExplosion_01()
Copy link
Member

@jjonescz jjonescz Nov 6, 2024 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing ref structs with ref fields. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing ref structs with ref fields.

Wy do you think this will be interesting in context of this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref fields cannot cause cycles in the struct layout, but I think an error would be reported today. However, since ref to ref struct is an error anyway, it's probably fine, and unrelated to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this scenario is not interesting for the purpose of this PR regardless whether ref fields are considered as a possible source of cycle. Because this PR doesn't change what fields are looked at. It simply ensures we do not infinitely expand types in the process.

{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
A<A<T>> x;
}

struct B<T>
{
A<B<T>> x;
}

struct C<T>
{
D<T> x;
}
struct D<T>
{
C<D<T>> x;
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,13): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 13),
// (14,10): error CS0523: Struct member 'C<T>.x' of type 'D<T>' causes a cycle in the struct layout
// D<T> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("C<T>.x", "D<T>").WithLocation(14, 10),
// (18,13): error CS0523: Struct member 'D<T>.x' of type 'C<D<T>>' causes a cycle in the struct layout
// C<D<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("D<T>.x", "C<D<T>>").WithLocation(18, 13)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void InstanceMemberExplosion_02()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
A<A<T>> x;
}

struct B<T>
{
A<C<B<T>>> x;
}

struct C<T>
{
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,13): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 13)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void InstanceMemberExplosion_03()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct E
{}

struct X<T>
{
T _t;
}

struct Y
{
X<Z> xz;
}

struct Z
{
X<E> xe;
X<Y> xy;
}
";
CreateCompilation(program).VerifyDiagnostics(
// (12,10): error CS0523: Struct member 'Y.xz' of type 'X<Z>' causes a cycle in the struct layout
// X<Z> xz;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "xz").WithArguments("Y.xz", "X<Z>").WithLocation(12, 10),
// (18,10): error CS0523: Struct member 'Z.xy' of type 'X<Y>' causes a cycle in the struct layout
// X<Y> xy;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "xy").WithArguments("Z.xy", "X<Y>").WithLocation(18, 10)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
Copy link
Member

@cston cston Nov 8, 2024 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#66844

Are these static cases affected by this issue? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these static cases affected by this issue?

The scenario is not broken, if that is what you are asking. But I consider the test related to the issue.

public void InstanceMemberExplosion_04()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
A<A<T>> x;
}

struct C<T>
{
C<C<T>> x;
}

struct B<T>
{
A<B<T>> x;
C<B<T>> y;
B<T> z;
}

struct D
{
B<int> x;
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,13): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 13),
// (9,13): error CS0523: Struct member 'C<T>.x' of type 'C<C<T>>' causes a cycle in the struct layout
// C<C<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("C<T>.x", "C<C<T>>").WithLocation(9, 13),
// (16,10): error CS0523: Struct member 'B<T>.z' of type 'B<T>' causes a cycle in the struct layout
// B<T> z;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "z").WithArguments("B<T>.z", "B<T>").WithLocation(16, 10)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void InstanceMemberExplosion_05()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
A<A<T>> x;
}

struct C<T>
{
C<C<T>> x;
}

struct B<T>
{
B<T> z;
A<B<T>> x;
C<B<T>> y;
}

struct D
{
B<int> x;
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,13): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 13),
// (9,13): error CS0523: Struct member 'C<T>.x' of type 'C<C<T>>' causes a cycle in the struct layout
// C<C<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("C<T>.x", "C<C<T>>").WithLocation(9, 13),
// (14,10): error CS0523: Struct member 'B<T>.z' of type 'B<T>' causes a cycle in the struct layout
// B<T> z;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "z").WithArguments("B<T>.z", "B<T>").WithLocation(14, 10)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void InstanceMemberExplosion_06()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
A<A<T>> x;
}

struct C<T>
{
C<C<T>> x;
}

struct B<T>
{
A<B<T>> x;
B<T> z;
C<B<T>> y;
}

struct D
{
B<int> x;
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,13): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 13),
// (9,13): error CS0523: Struct member 'C<T>.x' of type 'C<C<T>>' causes a cycle in the struct layout
// C<C<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("C<T>.x", "C<C<T>>").WithLocation(9, 13),
// (15,10): error CS0523: Struct member 'B<T>.z' of type 'B<T>' causes a cycle in the struct layout
// B<T> z;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "z").WithArguments("B<T>.z", "B<T>").WithLocation(15, 10)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void StaticMemberExplosion_01()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
static A<A<T>> x;
}

struct B<T>
{
static A<B<T>> x;
}

struct C<T>
{
static D<T> x;
}
struct D<T>
{
static C<D<T>> x;
}
Copy link
Member

@cston cston Nov 8, 2024 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing a mix of static and instance members? For instance:

struct C<T>
{
    static D<T> x;
}
struct D<T>
{
    C<D<T>> x;
}
``` #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing a mix of static and instance members? For instance:

These are not interesting in context of the problem we are facing because cycles across static and instance fields are detected separately and by different methods.
Cycles between static and instance fields never intersect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example above currently compiles, but peverify fails, and the native compiler reports a cycle. Do we need to log an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to log an issue?

Please do, we can investigate in more details later..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#75832

";
CreateCompilation(program).VerifyDiagnostics(
// (4,20): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// static A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 20),
// (14,17): error CS0523: Struct member 'C<T>.x' of type 'D<T>' causes a cycle in the struct layout
// static D<T> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("C<T>.x", "D<T>").WithLocation(14, 17),
// (18,20): error CS0523: Struct member 'D<T>.x' of type 'C<D<T>>' causes a cycle in the struct layout
// static C<D<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("D<T>.x", "C<D<T>>").WithLocation(18, 20)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
public void StaticMemberExplosion_02()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct A<T>
{
static A<A<T>> x;
}

struct B<T>
{
static A<C<B<T>>> x;
}

struct C<T>
{
}
";
CreateCompilation(program).VerifyDiagnostics(
// (4,20): error CS0523: Struct member 'A<T>.x' of type 'A<A<T>>' causes a cycle in the struct layout
// static A<A<T>> x;
Diagnostic(ErrorCode.ERR_StructLayoutCycle, "x").WithArguments("A<T>.x", "A<A<T>>").WithLocation(4, 20)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66844")]
[WorkItem("https://github.com/dotnet/roslyn/issues/75701")]
public void StaticMemberExplosion_03()
{
string program = @"#pragma warning disable CS0169 // The field is never used
struct E
{}

struct X<T>
{
static T _t;
}

struct Y
{
static X<Z> xz;
}

struct Z
{
static X<E> xe;
static X<Y> xy;
}
";
CreateCompilation(program).VerifyDiagnostics(
// Errors are expected here, see https://github.com/dotnet/roslyn/issues/75701.
);
}

[Fact, WorkItem(1017887, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1017887")]
public void EmptyStructsFromMetadata()
{
Expand Down
Loading
Loading