C#: Move GodotSharp to net7.0

[RedworkDE]

Use .NET 7 instead of .NET 6 as the C# runtime.

While programs targeting .NET 6 can generally run under .NET 7, there are some breaking changes, but changing the .net version is way simpler than trying to support multiple versions and on RC it looks like the plan is to drop support for 6 in favor of 7 anyways, so here is that change.

When updating to net 7 the only thing that causes issues in godot are the ref safety changes, which mean that you cannot return a ref struct that was returned from a method that takes a reference to a local parameter. Because all the interop structs are ref structs and most of the interop code takes parameters by in reference, this causes errors in a lot of places.

To fix this either the ref parameters have to be annotated with the scoped keyword to mark that that reference is not captured anywhere or the return types have to no be made ref structs.

The first required fairly extensive changes, including support in the source generator, and in the end only led to consistent JIT crashes that is was not able to diagnose, Thus I went with the second solution, since the interop structs don't actually contain any ref structs or ref fields.

The only issue with this is that these types were presumably made ref structs for a reason, but i cannot see what that reason might be.

[realkotob]

@RedworkDE This looks good, changes seem very sane.

Have you tested this on a project?

Also for reference for everyone else, here's a brief overview of the release notes for .net 7
https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-7

[RedworkDE]

I have been using essentially this patch for about a week a week with a project (it is based around an non godot specific library, which during a recent refactor gained a net7 requirement) and had no problems with it.

I just also tried upgrading another simple C# project, where it was only required to update the TargetFramework in the project file and then everything worked without problems. (I kinda thought it worked without changing the framework, but apparently i miss remembered from my multi-targeting experiments)

[neikeq]

The change from ref struct to struct is not the way to go. At least one of the benefits of ref struct is that it's guaranteed they are not stored in the GC heap, so you can get their address without pinning.
I'll give some insights tomorrow about a different solution (I'm on mobile right now). The rest of the changes look good.

[RedworkDE]

I went back to the other approach I mentioned and was able to find the cause the mentioned crash.

For some reason in .NET 7 referencing a type with the shape of godot_variant crashes the JIT; I was able to work around the crash by making the type sequential while preserving the effective layout of the type.

using System.Runtime.InteropServices;

Console.WriteLine(RuntimeInformation.FrameworkDescription);
Console.WriteLine("Testing for crash.");
FailsToJit();
Console.WriteLine("Did not crash.");

static void FailsToJit()
{
	// any reference (direct or indirect) to godot_variant will crash in during JIT import when resolving the type token
	// https://github.com/dotnet/runtime/blob/d037e070ebe5c83838443f869d5800752b0fcb13/src/coreclr/jit/importer.cpp#L224
	// i was unable to debug into that call to get any more data
	void Test(godot_variant arg) { }
	Test(default);
}

[StructLayout(LayoutKind.Explicit)]
public ref struct godot_variant
{
	[FieldOffset(0)] // offset doesn't matter
	private godot_variant_data _data;

	private ref struct godot_variant_data
	{
		private godot_variant_data_mem _mem;

		public struct godot_variant_data_mem
		{
			private float _mem0; // also happens for int here
		}
	}
}

After fixing the crash and adding scoped all over the place while leaving the interop structs remain ref struct I get about this patch.

Is there some larger test project for checking that everything is working as expected?

If you have any other ideas, I am willing to explore them.

[RedworkDE]

The crash is this: dotnet/runtime#80624. It is a bug in .NET 7 related to the exact shape of godot_variant and how it doesn't actually contain any pointers.

So no, it cannot be reproduced on master.

[neikeq]

Oh, I see. So it was a bug in the .NET runtime. It seems to be fixed now, so let's wait for the patch to be included in a release.

[RedworkDE]

In hindsight it was probably not super clear, but the first line of #71241 (comment) was a spoiler with the details about the crash.

Anyways the only remaining interesting point there was that the workaround for the crash is making godot_variant be of sequential layout with Pack 8, which puts the first field at offset 0 and the second at offset 8, just as the are now with explicit layout. I would do that regardless of if that bug is fixed in the runtime, as otherwise we end up with godot just crashing for users unless they updated to the newest patch of .NET 7 and not just any version of .NET 7.

In general I really dislike putting scoped everywhere, it feels very fragile (did I put the scoped everywhere where it is needed? A scoped ref to a ref struct does not make the ref struct itself scoped it is always assumed to be escaping. it is a new feature and I have little experience with it, ...).
IMO ref structs start having too many attached features to use them just to ensure that objects are always effectively pinned. I would rather remove the ref and write an analyzer to ensure that they don't end up on the heap.

Also you mentioned that you had an idea for a different solution, did you get around to writing that up, at some point?

[raulsntos]

I would do that regardless of if that bug is fixed in the runtime, as otherwise we end up with godot just crashing for users unless they updated to the newest patch of .NET 7 and not just any version of .NET 7.

Why would they not use the latest patch of .NET 7? I can understand staying on an older major version, but I can't think of any reason to hold patches.

In general I really dislike putting scoped everywhere, it feels very fragile (did I put the scoped everywhere where it is needed?

I assume there are analyzers or compiler errors that would let you know if you are doing something wrong. So if the code doesn't compile, I don't think you can forget to add the scoped keyword where it's needed.

A scoped ref to a ref struct does not make the ref struct itself scoped it is always assumed to be escaping. it is a new feature and I have little experience with it, ...).

I don't think it makes sense to define a ref struct as scoped, that depends on the method implementation and whether the method stores the reference outside of the scope (by storing it in a ref field or returning it). Therefore, I think it's up to the method to say if the parameter can escape or not.

IMO ref structs start having too many attached features to use them just to ensure that objects are always effectively pinned. I would rather remove the ref and write an analyzer to ensure that they don't end up on the heap.

I don't think it's possible to make an analyzer that ensures this. It's probably extremely difficult and likely easy to miss cases where the struct ends up on the heap, it seems more sensible to take advantage of the feature provided by the language instead.

Also, the new ref struct features may allow us to avoid unsafe and fixed in some scenarios where we couldn't before due to the limitations of ref structs. Hopefully this means we can simplify our code and potentially make it safer, although we'd have to evaluate if this is the case for us.

Also you mentioned that you had an idea for a different solution, did you get around to writing that up, at some point?

I'm not sure what @neikeq meant but we can probably allow users to target .NET 7 while keeping Godot assemblies in .NET 6 by setting roll-forward to Major or LatestMajor. I haven't looked too much into this, but last time I tried I run into the same crash you did (dotnet/runtime#80624).

[RedworkDE]

Why would they not use the latest patch of .NET 7? I can understand staying on an older major version, but I can't think of any reason to hold patches.

Mostly laziness in installing updates + the workaround is tiny and doesn't really have any cost

I assume there are analyzers or compiler errors that would let you know if you are doing something wrong. So if the code doesn't compile, I don't think you can forget to add the scoped keyword where it's needed.

The problem is that the error happen at the call site and these methods are all public, so they would potentially happen in the user assemblies.

I don't think it makes sense to define a ref struct as scoped, that depends on the method implementation and whether the method stores the reference outside of the scope (by storing it in a ref field or returning it). Therefore, I think it's up to the method to say if the parameter can escape or not.

this was about a scoped in godot_variant name parameter to some method (there is some bit in the diff where i had to add both scoped in to some parameter) but mostly to illustrated that I don't have that much experience with the new feature.

I'm not sure what @neikeq meant but we can probably allow users to target .NET 7 while keeping Godot assemblies in .NET 6 by setting roll-forward to Major or LatestMajor. I haven't looked too much into this, but last time I tried I run into the same crash you did (dotnet/runtime#80624).

diff --git a/modules/mono/glue/GodotSharp/GodotPlugins/GodotPlugins.csproj b/modules/mono/glue/GodotSharp/GodotPlugins/GodotPlugins.csproj
index e720d3878c..e58d730ee3 100644
--- a/modules/mono/glue/GodotSharp/GodotPlugins/GodotPlugins.csproj
+++ b/modules/mono/glue/GodotSharp/GodotPlugins/GodotPlugins.csproj
@@ -8,6 +8,7 @@

         <!-- To generate the .runtimeconfig.json file-->
         <EnableDynamicLoading>true</EnableDynamicLoading>
+        <RollForward>LatestMajor</RollForward>
     </PropertyGroup>

     <ItemGroup>
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
index fa79c2efbc..aa9e2cb8eb 100644
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
@@ -104,7 +104,7 @@ namespace Godot.NativeInterop
         }
     }

-    [StructLayout(LayoutKind.Explicit)]
+    [StructLayout(LayoutKind.Sequential, Pack = 8)]
     // ReSharper disable once InconsistentNaming
     public ref struct godot_variant
     {
@@ -113,11 +113,11 @@ namespace Godot.NativeInterop
             => (godot_variant*)Unsafe.AsPointer(ref Unsafe.AsRef(in _typeField));

         // Variant.Type is generated as an enum of type long, so we can't use for the field as it must only take 32-bits.
-        [FieldOffset(0)] private int _typeField;
+        private int _typeField;

         // There's padding here

-        [FieldOffset(8)] private godot_variant_data _data;
+        private godot_variant_data _data;

         [StructLayout(LayoutKind.Explicit)]
         // ReSharper disable once InconsistentNaming

This actually works surprisingly well, it always uses .NET 7 in the editor and the exported project uses either .NET 6 or 7 depending on the TargetFramework defined in the .csproj. I'll test out the behavior with different sdks installed on the machine and open a new PR about that diff. This PR can then remain about moving the godot assemblies to .net 7.

[raulsntos]

The problem is that the error happen at the call site and these methods are all public, so they would potentially happen in the user assemblies.

Godot API that uses ref struct is usually not meant to be consumed by the end user. If it's publicly exposed it's because it needs to be used by the source generators, so hopefully users won't have to worry about scoped.

[RedworkDE]

Yeah, that just means that the compile error potentially happens in generated code, which is arguably even worse, than when the user wrote it, because then they can't even really work around the error.

[raulsntos]

Yes but that'd be a Godot bug that we'd be responsible to fix. We should make sure our generators don't generate invalid code.

[RedworkDE]

I rebased the scoped changes and pushed them into this branch.

TODO: find some larger project to test this with.

[akien-mga]

Superseded by #71825.

[markdibarry]

@akien-mga I may be totally wrong, but I think these are separate PRs for a reason. This one is to update Godot to .NET 7, where #71825 is to make sure Godot doesn't throw a fit if .NET 7 is installed on the user's machine but .NET 6 isn't.

[RedworkDE]

As per RC, the plan is to keep support for .net6 until we actually need .net7/c#11+ in the GodotSharp libraries, as user projects can now target .net7 without these changes. I am going to convert this PR to draft and leave it open until these changes become relevant again.

Should probably be changed to target 4.x and marked as breaking change, as, unlike what I initially thought, this required changing the target framework in the user project.

[shana]

@akien-mga @RedworkDE Ah, this is very cool, great stuff! I'm not sure where the plans for .NET 7 plan are in the general roadmap timeline, but fyi we'll need it for consoles, so this will probably need to be put back into operation in the near future. I'll be testing this branch out, I'll update here if I run into any issues.

[bekir-ozturk]

.NET 7, being a 'Standard Term Support' release, will go out of support in about 8 months.

Like .NET 6, .NET 8 is a 'Long Term Support' release and will be in support for 3 years. Perhaps it is a good idea to directly move to .NET 8 instead of 7?

[haslingerm]

Just an update: .NET 8 (LTS) is now released.

[hhyyrylainen]

Is there any chance that we would get .NET 8 in Godot anytime soon? I'm not fully sure how things work but I think I'm hitting some bugs in the .NET runtime (at least I'm seeing ThreadLocal<T> causing game lockups when a debugger is present with multiple thread callstacks pointing to internal code in that type seemingly related to debugger checks). I assume the .NET team has fixed various runtime bugs in the releases since .NET 6, which would be awesome to get. If only to rule out one potential cause of my issues.

Also it looks like .NET 6 LTS support will end November 12, 2024. So surely this update needs to be happen pretty soon as there aren't that many months left before there will be a rush to update before the latest Godot release will be using an unsupported runtime.

[paulloz]

Is there any chance that we would get .NET 8 in Godot anytime soon?

If you want for your game to target .NET 8, it is already possible to do so. You simply need to configure your TargetFramework.
If you are talking specifically about GodotSharp (and GodotSharpEditor, etc.) targeting .NET 8, there is no clear ETA for now. This would still need to be merged before that happens.

[hhyyrylainen]

If you want for your game to target .NET 8, it is already possible to do so. You simply need to configure your TargetFramework.

I thought that that didn't work, but turns out that it was just a matter of there being some other issue earlier and I concluded sometime ago that I needed to target net6.0. Testing again today I found out that it is as simple as switching the target framework version. For reference my other message where I initially found this out: #86374 (comment)

[akien-mga]

Superseded by #92131. Thanks for the contribution!