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
Conversation
…doesn’t include the target struct (a cycle at the end of the path) Fixes dotnet#66844
dotnet-issue-labeler
bot
added
the
untriaged
|
@dotnet/roslyn-compiler Please review |
3 similar comments
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
| @@ -219,6 +219,196 @@ 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() | |||
There was a problem hiding this comment.
Consider testing ref structs with ref fields. #Resolved
There was a problem hiding this comment.
Consider testing ref structs with ref fields.
Wy do you think this will be interesting in context of this PR?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@dotnet/roslyn-compiler For the second review. |
1 similar comment
|
@dotnet/roslyn-compiler For the second review. |
|
@cston for second review. |
| struct D<T> | ||
| { | ||
| static C<D<T>> x; | ||
| } |
There was a problem hiding this comment.
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;
}
``` #ResolvedThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
The example above currently compiles, but peverify fails, and the native compiler reports a cycle. Do we need to log an issue?
There was a problem hiding this comment.
Do we need to log an issue?
Please do, we can investigate in more details later..
| } | ||
|
|
||
| [Fact] | ||
| [WorkItem("https://github.com/dotnet/roslyn/issues/66844")] |
There was a problem hiding this comment.
Are these static cases affected by this issue? #Resolved
There was a problem hiding this comment.
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.
| { | ||
| // 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); |
There was a problem hiding this comment.
Are there any examples where typesWithCycle willl have more than one item?
I see nothing preventing that. If that is what you are asking.
There was a problem hiding this comment.
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.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Fixes #66844