Workaround on GodotSharp duplicate key exception when adding generic entries to ScriptTypeBiMap

[Delsin-Yu]

A workaround for #79519, this issue is a blocking issue for most .Net users.
Which is one of the causes of the Mega #78513
While #83204 resolved this issue by simply rejecting the duplicating addition, it still left the issue on the Native end and caused error message prints.
We believe this PR(86305) solves the core issue of this whole chain.
Found by @Smallantsa

Validated using This Repo

It turns out that this PR is yet another workaround on the issue, while it somehow resolves the symptoms, further investigation on Generic Derivatives holding empty Script Path is required to fully resolve this issue.

By returning null on ReflectionUtils.FindTypeInLoadedAssemblies, this PR is a workaround for issues related to the System.ArgumentException: An item with the same key has already been added. in ScriptTypeBiMap.
While side effects are still to be validated.

It looks like the old way of obtaining a generic type will return a different instance for the type, resulting in a duplicate key exception in ScriptTypeBiMap.Add(IntPtr scriptPtr, Type scriptType)

AppDomain.CurrentDomain.GetAssemblies()
            .FirstOrDefault(a => a.GetName().Name == assemblyName)?
            .GetType(typeFullName)

This new approach appears to return the same instance, so it should fix all issues related to:

modules/mono/glue/runtime_interop.cpp:1324 - System.ArgumentException: An item with the same key has already been added. Key: GenericScript`1[System.Single]
     at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
     at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
     at Godot.Bridge.ScriptManagerBridge.ScriptTypeBiMap.Add(IntPtr scriptPtr, Type scriptType) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs:line 23
     whatever  other calls

[AThousandShips]

While #83204 resolved this issue by simply rejecting the duplicating addition, it still left the issue on the Native end and caused error message prints.

To confirm, you tested this PR and confirmed it with the changed code? This PR has not been merged yet so it hasn't solved anything yet

[Delsin-Yu]

While #83204 resolved this issue by simply rejecting the duplicating addition, it still left the issue on the Native end and caused error message prints.

To confirm, you tested this PR and confirmed it with the changed code? This PR has not been merged yet so it hasn't solved anything yet

I tested 83204 with the changed code and confirmed it does NOT solve the core issue.
83204 requires additional changes to make it work properly, which I have commented on there. (While that still doesn't solve the core issue)

[CantyCanadian]

I don't know if I'm missing something, but building latest engine with your changes in, I get the following errors which are very similar. I was using 83204 with 4.1 but dropped the change with 4.2. Now that the error is back, I tried your fix hoping that it was a permanent solution.

[Delsin-Yu]

I don't know if I'm missing something, but building latest engine with your changes in, I get the following errors which are very similar. I was using 83204 with 4.1 but dropped the change with 4.2. Now that the error is back, I tried your fix hoping that it was a permanent solution.

Is it possible for you to share a minimal reproduction for your case?

[CantyCanadian]

I really would, however, it's not 100% reproductible and I am not sure what is the smoking gun in my case. If I had that, I could make a barebones project, but right now, all I could do is send my whole project. The only points I found that could've caused it to happen were:

• A class with a generic inheriting from a Resource type, then that generic class being inherited by various other resources, setting the generic to a value.
• One or many resources being created in the project using that non-generic top-level class.
• Those resources being passed by Export to a node in the scene.

In that case, if I build the code with it being dirty (not just building with no changes), there's a chance it breaks. It will break after a while of trying, but I can't figure what exactly causes it.

[Delsin-Yu]

I really would, however, it's not 100% reproductible and I am not sure what is the smoking gun in my case. If I had that, I could make a barebones project, but right now, all I could do is send my whole project. The only points I found that could've caused it to happen were:

• A class with a generic inheriting from a Resource type, then that generic class being inherited by various other resources, setting the generic to a value.
• One or many resources being created in the project using that non-generic top-level class.
• Those resources being passed by Export to a node in the scene.

In that case, if I build the code with it being dirty (not just building with no changes), there's a chance it breaks. It will break after a while of trying, but I can't figure what exactly causes it.

The issue this PR is focusing on is related to the opening scene from my tests, that is, you need to have a specific scene opened while rebuilding your C# solution (to trigger the issue); hope this info will help you identify the issue in your project. It's okay for me if you send the whole project.

[dsh0416]

From our usages, we can also found this issue 100% reproducible when generic entries being re-built in the editor. We believe that this PR has solved our problem in a patched version of GodotSharp.

[paulloz]

I'm wondering if it is a good thing to essentially silence the error while not solving the main issue (there are duplicate entries in the bimap / we are inserting generics in the bimap). Isn't it useful for us to have an error telling us there is an issue to fix?

[Delsin-Yu]

I'm wondering if it is a good thing to essentially silence the error while not solving the main issue (there are duplicate entries in the bimap / we are inserting generics in the bimap). Isn't it useful for us to have an error telling us there is an issue to fix?

The issue we are discussing here is not user-related, it is an editor bug that requires fixing.
There is no duplicate entry created on the user side (that won't compile at all), and using generic is a standard practice in C#.

The issue here is caused by GodotSharp creating duplicate generic entries out of nowhere (under certain circumstances, one user script that derives from a generic class can cause GodotSharp to generate duplicate System.Type instances) and causing trouble.

The approach we took here is resolving the actual API that causing this issue, not silencing the error (what 83204 did).

[paulloz]

Yes, I know. What I’m saying is: as discussed in the other linked PR, generics should not hit the bimap at all. IINW, what we do here is working around so that, when they do hit it, no duplicate key is raised. Essentially removing the errors, but leaving the generics in the map. Or am I misunderstanding the fix?

…

>

[Smallantsa]

@raulsntos Can you help me take a look？

[Delsin-Yu]

Yes, I know. What I’m saying is: as discussed in the other linked PR, generics should not hit the bimap at all. IINW, what we do here is working around so that, when they do hit it, no duplicate key is raised. Essentially removing the errors, but leaving the generics in the map. Or am I misunderstanding the fix?

Previously, there was a bug that caused generics to hit the Bi-Map.
83204 is the "working around" you mentioned.
This PR discovered the essential issue (causing generics to hit the Bi-Map) and got it fixed.
The approach we took here is NOT a "workaround" under this context.

[paulloz]

My apologies, I misunderstood what the intent behind the patch was. You're right, this would prevent the insertion into the bimap in ScriptManagerBridge.TryReloadRegisteredScriptWithClass by returning early on the null check above.

Though, I'm now a bit concerned (pardon me if this is annoying) by the fact $"{assemblyName}.{typeFullName}" is not what an assembly qualified name looks like. Isn't this always returning null, since the types we're reloading are always in a different assembly from the executing one?

[Delsin-Yu]

My apologies, I misunderstood what the intent behind the patch was. You're right, this would prevent the insertion into the bimap in ScriptManagerBridge.TryReloadRegisteredScriptWithClass by returning early on the null check above.

Though, I'm now a bit concerned (pardon me if this is annoying) by the fact $"{assemblyName}.{typeFullName}" is not what an assembly qualified name looks like. Isn't this always returning null, since the types we're reloading are always in a different assembly from the executing one?

That's a good point, I think I do need to talk to @Smallantsa for more detail.

My testing (in SharpLab) indicates that this API does not work when passing type.FullName, which made me think the typeFullName godot provides is in another format.
Since this actually returns null then there should be more to fix.

How does it even work when returning null.....

[Delsin-Yu]

From the testing project I have here, when unloading assemblies, the FindTypeInLoadedAssemblies API is only called when dealing with generic types.

[paulloz]

It is basically when our script path is empty when iterating through the scripts to reload (here). I'm unsure of other possible cases where the path would be empty here (essentially: part of the class hierarchy for something, but no [ScriptPathAttribute]).

[zaevi]

It is basically when our script path is empty when iterating through the scripts to reload (here). I'm unsure of other possible cases where the path would be empty here (essentially: part of the class hierarchy for something, but no [ScriptPathAttribute]).

After discussion with @Delsin-Yu and @Smallantsa:

It's ok to ignore ScriptPath for generic types. (like Component<T>)

However, constructed generic types (like Component<int>, Component<string>) need the path, and I don't think we can give [ScriptPathAttribute] to them. Therefore, the engine will re-create CSharpScript for constructed generic types and re-register to _scriptTypeBiMap and throw exceptions.

It's necessary to prevent re-instance CSharpScript for constructed-generic types.

cc @raulsntos

[Delsin-Yu]

Superseded by #87550