Skip to content

Resolve reference comparison warnings (CS0252)#576

Merged
notfood merged 2 commits intorwmt:devfrom
ianleeder:clear-warnings-CS0252
Jul 19, 2025
Merged

Resolve reference comparison warnings (CS0252)#576
notfood merged 2 commits intorwmt:devfrom
ianleeder:clear-warnings-CS0252

Conversation

@ianleeder
Copy link

@ianleeder ianleeder commented Jul 18, 2025

Feel free to reject, but the warnings are triggering me. This PR resolves 41 compiler warnings.

CS0252 warns:

warning CS0252: Possible unintended reference comparison; to get a value comparison, cast the left hand side to type 'type'

Basically we could inadvertently be doing reference checks instead of value checks. The compiler suggests casting to a concrete class (since most of these are generic objects). But that introduces new risks, and complicates the checks. Switching to using .Equals on the known concrete class will, at worst do the same reference check, and at best do an value check. Plus it has the added bonus of clearing the compiler warnings.

I plan to resolve a bunch more warnings, but I figured splitting them up by warning type may be more likely to get at least some of them merged. I tried to prevent as many auto-format changes as possible (to prevent overcomplicating the changes), but a few whitespace ones slipped through.

@ianleeder ianleeder marked this pull request as draft July 18, 2025 15:07
@ianleeder
Copy link
Author

Everything compiles and looks good, but I want to run some tests on this before merging.

@ianleeder ianleeder changed the title Resolve warnings for CS0252 Resolve reference comparison warnings (CS0252) Jul 18, 2025
@ianleeder ianleeder marked this pull request as ready for review July 18, 2025 15:44
@notfood
Copy link
Member

notfood commented Jul 18, 2025

I'm wary, these aren't the same. What happens if the result of AccessTools.Method is null?, suddenly you get null pointer exception whereas before it just failed the if case. The correct way to fix the warning is to do as it says, cast the side to a type. Or just use Object.Equals, it makes it less readable though.

@ianleeder ianleeder force-pushed the clear-warnings-CS0252 branch from ea2d4e7 to 786e32d Compare July 18, 2025 23:18
@ianleeder
Copy link
Author

I'm wary, these aren't the same. What happens if the result of AccessTools.Method is null?, suddenly you get null pointer exception whereas before it just failed the if case. The correct way to fix the warning is to do as it says, cast the side to a type. Or just use Object.Equals, it makes it less readable though.

You're right of course. I avoided the direct cast ((MethodInfo)inst.operand) for safety, just in case it was a different class. I assumed that the reflection result would exist. But assumptions make fools of us all...

I've updated it to cast using the syntax inst.operand as MethodInfo since this is safe against InvalidCastException and gone back to the == syntax.

@notfood notfood merged commit 7871885 into rwmt:dev Jul 19, 2025
1 check passed
@ianleeder ianleeder deleted the clear-warnings-CS0252 branch July 19, 2025 05:07
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