Repository navigation
Unused generic type parameter should not cause loader failure #6924
Description
Activity
Stumbled over exactly this issue the last days. 🙁 Really thought it would work, and it would have been a nice complement to the new
System.Memory<T>type.because the field involved here is an empty struct.
People might reply, "True but nobody needs an empty struct in practice", therefore here is an example that throws TypeLoadException with a non-empty struct. i.e. a more severe example of the failure.
class Program { static void Main(string[] args) { Test(); } static SNode Test() { return new SNode(); } struct ArrayElementReference<T> { public T[] Array; public int Index; } struct SNode { ArrayElementReference<SNode> x; int y; } }
Reacted by Sewer.Suggest changing the title from "Unused generic type parameter ..." to "Self-referencing generic type parameter ...", as it's not related to whether the type parameter is used or unused - see @verelpode's example above, and also https://github.com/dotnet/coreclr/issues/20220, which now seems to be a duplicate of this.
Reacted by The-Futurist, Adeel Mujahid, Katelyn Gadd, Tom Brady and RomanThis issue becomes less academic and much more significant when one begins to leverage recently added support for generic pointers, the
unmanagedgeneric constraint andref.Here is a concrete and realistic example (which comes from actual code I've developed) dotnet/roslyn#35324.
Reacted by Nikita Tsukanov, Luiz, Hugh Gleaves - admin and xparadoxicalStill no progress on this bug. I can only assume that a few users are raising this, because a truly fundamental failure to process completely legal generated code strikes me as something that would get a little more attention.
Reacted by Luka Mandić and Gamma_Draconis@Korporal the 3.0 release is getting closer and the team is becoming more selective in what issues to take on. That might be the reason. (I'm not a team member.)
Also, this issue was reported by 3 people so far. So while your realistic example may be bad experience (I didn't check it yet), the reality is not too many people hit it yet, which means lower priority. Hardly something as must-have for 3.0 (just my opinion).
It's been around for three and a half year though!
the reality is not too many people hit it yet
We've had to work around this issue multiple times in Roslyn. Did you need separate reports each time we hit this to realize that it is a persistent problem?
Reacted by Adeel Mujahid, The-Futurist, STU, Martin Johns, Tom Brady, Nikita Tsukanov, masonwheeler, flerka, Antonia, Katelyn Gadd and 8 more@karelz I understand this seems to be relativly small impact but it is very fundamental in nature, a very foundational capability. In my case I'm working on a C# rewrite of a custom memory allocator that sits on top of an application's unmanaged memory space. Recent advances to C# in areas like type constraints and the
refkeyword have made this possible. However this bug is a solid roadblock because we have a generic struct typeSmartPointer<T>which supports lists and trees that cannot be implemented because code compiles but fails at runtime.The struct type
SmartPointer<T>when being used in a list, appears as a member field in a struct typeTthat can "point" to another instance of aT, this is where the recursive reference comes up. For reasons I can't go into here, we cannot use a conventionalunsafepointer.(Just to be clear these struct instances are not in managed memory but allocated from unmanaged area of a process's addess space using the custom allocator).
Is there any possibility the bug could be given to a competent intern to see if they can do anything with it? I dont care about it being in 3.0, but currently its going nowhere and this could continue for years more if Microsoft continue to treat it as low importance.
Leaving it unfixed for year after year could also make it harder to fix because the CLR may get refactored or restructured in ways that make the fix more difficult. This feature really is a basic expectation.
Id love to look at it myself even but will not pretend I have the internals knowledge or experience to actually deliver.
Reacted by Bjarke Elias, Nikita Tsukanov, Michael Delz and Tom BradyFWIW, this is an issue we hit repeatedly, particularly in the context of generated C# code, where it is often not easy or feasible to work around (changing output of the code generator wholesale to avoid this problem would be API breaking or have unacceptable knock-on effects, detecting and avoiding on a case-by-case basis would lead to inconsistent or incompatible APIs in some cases).
Reacted by Jan Polášek, Tom Brady and Thomas Köppe@karelz
I see there's no movement on this, is anyone in a position to say that this will or won't be addressed and if so when approx it might be fixed?No idea - maybe @davidwrighton or @jkotas can comment?
I still believe it is not serious enough to prioritize it. If you have a fix, I think we would be open to a contribution (if it is not overly complex, it is high-quality and not breaking).
General rule is Future = maybe one day.Reacted by Bjarke Elias, STU, Tom Brady, Katelyn Gadd, Luka Mandić, Gamma_Draconis, Chronos Ouroboros and xparadoxical37 remaining items
@pengweiqhca In your example you're using a
class, and therefis pointing to one of its fields. So in this scenario, therefis not pointing to unmanaged/native memory. As a result it's not safe to convert it to an unmanaged pointer because the GC can relocate it between theAsRefandAsPointercalls.@DaZombieKiller Sorry, I forgot to modify it, but it has been fixed now. In my actual code I use struct.
Whether this issue will be resolved or not, the user experience is currently really bad. We just ran into this issue because a colleague wanted to keep a LUT inside a value type using
ImmutableArray<T>(company coding rules say no mutable static members soT[]would be forbidden).All I get is a
TypeLoadExceptionwith no hint as to what is going wrong. When it first showed up it was in the middle of a massive file that was affected by several different source generators and it was very hard to know what it was causing the problem.I even tried using
dotnet-tracein hopes of getting a better idea, but it just crashed and produced a truncated trace.I eventually built runtime to debug it myself and found the exception thrown in
clsload.cpp:3451if (PendingTypeLoadHolder::CheckForDeadLockOnCurrentThread(pLoadingEntry)) { // Attempting recursive load ClassLoader::ThrowTypeLoadException(pTypeKey, IDS_CLASSLOAD_GENERAL); }
So even here there is not indication as to what is actually going wrong.
If the runtime will not support this then Roslyn should not allow the code to compile in the first place. Internally we should be able to add this case to our analyzers so people are not caught off guard in the future, but we shouldn't be having to to do that ourselves.
Reacted by artdedeco, TechPizza, The-Futurist, Neal Gafter, Gamma_Draconis, D3-LucaPiombino, xparadoxical and SupinePandora43I decided, seeing as it's been forever and likely gonna take forever more, that I'd put careful thought into an analyzer of my own design to help deal with this issue.
https://github.com/sunkin351/GenericStructCyclesAnalyzer
Using this would deal with any user experience issues revolving around this. The analyzer itself is incredibly simple, single file and catches every situation I could think of.
I would like to propose adding it to the existing suite of .NET Analyzers, but I'm not sure it would stand up to their guidelines. Maybe some of you could check that for me.
Reacted by xparadoxicalI run into this again when using a nested struct of a generic class, not immediately realizing I was dealing with this issue. I guess this happens because a nested type of a generic type is essentially a generic type as well.
So, the following code throws a TypeLoadException:
var tmp = new Other(); public class Container<T> { public struct Nested { } } public struct Other { public Container<Other>.Nested Nested; }- added a commit that references this issue
on Mar 27, 2023 - added a commit that references this issue
on Apr 4, 2023 Should this be reopened since there are still cases not covered by the fix, such as the one mentioned here?
struct N<T> { } struct M { public N<Nullable<M>> E; }
Reacted by Gamma_Draconis and vladdShould this be reopened since there are still cases not covered by the fix, such as the one #83995 (comment)?
That should probably be a new issue so that it has a new thumbs up counter with people who're blocked by this. We can wait until someone runs into it.
The counter is used to prioritize the bug against any other type loader work - i.e. there's a finite number of people who can fix such bug and working on fixing the bug means other things will not be worked on. It's zero sum.
Reacted by Joe4evr and nathan-moore- ghost locked as resolved and limited conversation to collaborators
on May 5, 2023
Consider the following code:
This code will compile with C# 6.0 and is legal according to the CLI spec. The type definition is recursive but the layout of the struct is not because the field involved here is an empty struct. The CLR is unable to handle this though and fails at runtime with a
TypeLoadException:See also #5479