Skip to content

AsmResolver System.Enum IsValueType=true workaround. Fixes #211#212

Merged
ds5678 merged 3 commits intoBepInEx:masterfrom
AndrewSav:fix-enums
Jun 3, 2025
Merged

AsmResolver System.Enum IsValueType=true workaround. Fixes #211#212
ds5678 merged 3 commits intoBepInEx:masterfrom
AndrewSav:fix-enums

Conversation

@AndrewSav
Copy link
Contributor

This is what you get when you ask a person completely unfamiliar with a complex codebase to submit a PR.

As discussed in #211 I've tested that the code generated for one particular method I was having problem is now fine. I have no slightest idea, if this IsValueType matters elsewhere. For, example when a value of this type is passed as a method parameter. I also do not know how to find or test relevant code paths.

IMO, this change is unlikely to break anything already not broken, but this may be not a comprehensive fix. I briefly considered patching this at run-time in AsmResolver using reflections, but this is going to be complex because this is not a field you can change the value off, it actually checks the binary metadata when you call this property, so you would need to patch code not just data to make it work. My guess that we do not want that in Il2CppInterop.

Another workaround is to wrap AsmResolver types, but it looks it will require a whole lot of types to wrap to the point of being unfeasible.

If you can reasonably be sure, that we cover everything here, that's good, but if not, I would not know what else we need to patch.

@ds5678
Copy link
Collaborator

ds5678 commented Jun 2, 2025

I would feel more comfortable if you made an extension method IsValueType() and replaced all uses of the AsmResolver property.

@AndrewSav
Copy link
Contributor Author

@ds5678

I think your suggestion should work generally , although I have a couple of questions:

I do not think extension method can override a non-extension one?
Extension method on which type?

@ds5678
Copy link
Collaborator

ds5678 commented Jun 2, 2025

I do not think extension method can override a non-extension one?

This would be true if both were methods, but the original is a property.

Extension method on which type?

ITypeDescriptor

@AndrewSav
Copy link
Contributor Author

@ds5678 again, I have no way to determine, which ones need to be changed, so I changed them all and hope I did not miss one or two. Can you have a look and give your opinion please?

@ds5678
Copy link
Collaborator

ds5678 commented Jun 3, 2025

I changed them all

This is exactly what I wanted.

hope I did not miss one or two.

Do a text search for IsValueType and verify.

@AndrewSav
Copy link
Contributor Author

AndrewSav commented Jun 3, 2025

Do a text search for IsValueType and verify.

That's how I did this in the first place. Yet, it's easy to miss one in the list of several dozens. What I'm saying is that I'm happy with my verification, but it might make sense for someone else to verify as well, up to you.

@ds5678
Copy link
Collaborator

ds5678 commented Jun 3, 2025

I assume you verified that it fixes your issues?

@AndrewSav
Copy link
Contributor Author

I assume you verified that it fixes your issues?

Yes, I did.

@ds5678 ds5678 merged commit 51abbdd into BepInEx:master Jun 3, 2025
2 checks passed
@ds5678 ds5678 added this to the 1.5.1 milestone Sep 2, 2025
@AndrewSav
Copy link
Contributor Author

@ds5678 it seems that the fix in AsmResolver was released now. I do not have time to revert the changes and test the result in the immediate future, but I wanted to give the headups in case somone else would like to do that. May be it can be done as part of the dependency upgrade.

@ds5678
Copy link
Collaborator

ds5678 commented Sep 11, 2025

@AndrewSav 1.5.1 is the last release before v2, so don't worry about it.

XtraCube pushed a commit to XtraCube/Il2CppInterop that referenced this pull request Sep 18, 2025
BepInEx#212)

* fixes BepInEx#211

* use extensoin method

* add a comment

(cherry picked from commit 51abbdd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants