Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove IBindable.ToString() default interface implementations #5571

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Dec 6, 2022

This is essentially a workaround for what is ostensibly going to be a xamarin or mono bug which is causing ppy/osu#21530.

The explanation is thus: The reason why the config files were zero-byte is that an exception was being thrown (and silenced) in PerformSave(). This is the exception:

System.InvalidCastException: Object must implement IConvertible.
  at System.Convert.ChangeType (System.Object value, System.Type conversionType, System.IFormatProvider provider) [0x00040] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corefx/src/Common/src/CoreLib/System/Convert.cs:340 
  at osu.Framework.Bindables.Bindable`1[T].Parse (System.Object input) [0x000c7] in D:\Open Source\osu-framework\osu.Framework\Bindables\Bindable.cs:269 
  at osu.Framework.Configuration.IniConfigManager`1[TLookup].PerformSave () [0x0004c] in D:\Open Source\osu-framework\osu.Framework\Configuration\IniConfigManager.cs:93 

This stack trace is nonsense. There is no conceivable pathway wherein IniConfigManager can call Bindable.Parse(). The arguments didn't make sense either, it was trying to parse CultureInfo.InvariantCulture as a string.

@frenzibyte suggested on discord that we've seen this sort of black magic before and it was caused by default interface method implementations. It tracked, since the symptoms did look suspiciously like the correct arguments were being passed to the completely wrong method. I tried removing them and that fixed the bug. Hence this diff.

I am very explicitly not looking at the xamarin bug, as to me it is not worth the time to do so and my goal is to keep things mostly working until we can collectively turn our heads towards net6 (relevant discord conversation from earlier today).

Now:

  • The .ToString() default implementations are, as far as I can tell, 100% dead code. I have no idea what can call these ever. A quick sharplab suggests that it is impossible for these to be called, so they are removed without replacement.
  • The .ToString(IFormatProvider) overload was the one actually causing the bug. I considered just removing it, but then ended up adding an extension method to cover for it, and all IFormattables to ever possibly exist. Which is arguably cleaner than duplicating the default implementations in IBindable<T>, at the cost of an extra using.

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested to work as expected on Android with visual-tests and osu!.

@bdach bdach deleted the mono-is-being-silly-again branch December 7, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants