osu-framework
osu-framework copied to clipboard
ppy
Disallow construction of bindables for types that have more specific implementations
Bindable<float> and Bindable<double> are footguns; it's extremely easy to get caught on floating-point comparisons using them mistakenly instead of appropriate subclasses. It might be a bit extreme, but I think we shouldn't allow those to be constructed at all and just throw an exception if anyone tries to use them directly. It's actually bitten us in real usage (see ppy/osu#7708).
Why not move the BindableNumber.IsDefault check into Bindable instead? or rather, move that logic in a static class and call it from Bindable. something like TypeEqualityComparer<T>.
You'd have to move Precision too, which doesn't make sense for non-numeric types.
What about other types like bool, int, MarginPadding? For consistency, we might want to disallow all types which have a Bindable<T> subclass.
Consider this situation:
class ParsableTextField<T> {
public readonly Bindable<T> ValueBindable = new(); // magic crash when T is float
...
void onTextEdited () {
ValueBindable.Parse( Text );
}
}
If anything, you might consider having a debug-only message rather than actually disallowing it (an analizer, perhaps?).
A debug-only message wouldn’t do any favour, and your use case will break on IsDefault for float/double types due to lack of precision. Would suggest using IBindableWithCurrent.Create.
I don't think the new() syntax changes anything here, personally. I have an intense personal dislike for it, we don't (and won't) use it, and the fact remains that using the types in question causes subtle bugs.
The 'new()' syntax has nothing to do with this. It doesnt do anything. Its the fact of using a generic bindable and 'accidentally' creating a Bindable<float>. Which you are planning to disallow, therefore mysteriously crashing.
Maybe I should rephrase my concern.
class A<T> {
Bindable<T> bindable = new Bindable<T>();
}
Crashes when T is float, double, or whatever else you plan to 'ban'. This is not my specific use case, its an example. Neither is it said anywhere that this bindable does anything except hold a value and a 'changed' event. The issue is that T can be anything and code that doesn't rely on the things you want to ban that generic for will break for no actual reason. This is why I think a better solution would be to create an analizer which would error or warn if you manually, specifically created a base Bindable which has a forbidden generic parameter.