Exception handling helpers

[lipchev]

There are currently two types extending the UnitsNetException (AmbigiousUnitParseException and the UnitNotFoundException) - with a bunch of standard exceptions that are scattered around (a lot of them part of the auto-generated code).

Besides the likely inconsistency in the messaging, having the same string format repeated in code is of course also increasing our file size.

Here is the list of exceptions that I think should cover all of our cases:

• UnitNotFoundException: this one is pretty obvious, it is thrown by the UnitConverter / UnitParser etc - it is also the exception that I typically use when given an invalid unit, such as (MassUnit)(-1)
• QuantityNotFoundException: this is thrown when given a type (or type name) that is unknown- this one is thrown by the QuantityInfoLookup (which is itself called by the Quantity.Parse or the UnitConverter)
• InvalidConversionException: I'm not sure if this one applies to the current state of the v6 , but in my new implementation of the UnitConverter this is thrown when attempting to convert one quantity to another, incompatible, quantity type
• AmbiguousUnitParseException: thrown by the UnitParser

In addition, I would create a new solution folder called Exceptions (no namespace generation), with all of the above classed inside it (personally, I think it kinda sucks to have all classes in one namespace, but I don't think we can easily move away from that..).

Finally, for the more typical exception-throwing scenarios, I'd like to add some static helper methods. Here's what I've come up so far:

internal static class ExceptionHelper
{
    internal static ArgumentException CreateArgumentException<TUnit>(Enum unit, string argumentName)
        where TUnit : struct, Enum
    {
        return new ArgumentException($"The given unit is of type {unit.GetType()}. The expected type is {typeof(TUnit)}.", argumentName);
    }

    internal static ArgumentException CreateArgumentException<TQuantity>(object obj, string argumentName)
        where TQuantity : IQuantity
    {
        return new ArgumentException($"The given object is of type {obj.GetType()}. The expected type is {typeof(TQuantity)}.", argumentName);
    }

    internal static ArgumentException CreateArgumentOutOfRangeExceptionForNegativeTolerance(string argumentName)
    {
        return new ArgumentOutOfRangeException(argumentName, "The tolerance must be greater than or equal to 0");
    }

    internal static InvalidOperationException CreateInvalidOperationOnEmptyCollectionException()
    {
        return new InvalidOperationException("Sequence contains no elements");
    }
    
    internal static InvalidCastException CreateInvalidCastException<TQuantity, TOther>()
        where TQuantity : IQuantity
    {
        return CreateInvalidCastException<TQuantity>(typeof(TOther));
    }
    
    internal static InvalidCastException CreateInvalidCastException<TQuantity>(Type targetType)
        where TQuantity : IQuantity
    {
        return new InvalidCastException($"Converting {typeof(TQuantity)} to {targetType} is not supported.");
    }
}

We could probably move these strings to a resources dictionary, but I'm not too concerned with this ATM.

What I'm not sure about is whether the helpers should return or throw the exception.. If I'm not mistaken the second option, while a bit more convenient, would introduce an additional row in the stack trace compared to the first one. I know of the [DoesNotReturn] attribute but I don't think it affects the stack trace. What do you think?

[lipchev]

Ah, and by the way- I'm also aware that we could have the argument name be optional (see [CallerMemberName]) however this one is not available for .netstandard2.0 (unless we also include the polyfills, but I don't know if this would play nicely, add a client dependency etc..)

[angularsen]

Good idea to cleanup and enforce a bit of consistency here, I like your proposal 🙌

I say let it throw and use [DoesNotReturn].
I also propose using the name InvalidUnitConversionException, adding "Unit" into it to make it more explicit.

[angularsen]

ExceptionFactory is an alternative name, but I have no strong opinion here. Helper works too. Guard is often used, but more to bake in logic that conditionally throws on inequality, null etc.

[lipchev]

I say let it throw and use [DoesNotReturn]. I also propose using the name InvalidUnitConversionException, adding "Unit" into it to make it more explicit.

This doesn't seem to work when using the [DoesNotReturn] version

        char IConvertible.ToChar(IFormatProvider? provider)
        {
            throw ExceptionHelper.CreateInvalidCastException<Acceleration, char>();
        }

Doesn't work here either (and even if it did, I wouldn't call it pretty):

        double IQuantity.As(Enum unit)
        {
            if (unit is not AccelerationUnit typedUnit)
                ExceptionHelper.ThrowArgumentException<AccelerationUnit>(unit, nameof(unit));

            return As(typedUnit);  // CS0165: Use of unassigned local variable 'typedUnit'
        }

One obvious solution would be to revive the Guard and use something like:

• var typedUnit = Guard.Cast<AccelerationUnit>(unit, nameof(unit));
• var typedUnit = Guard.ThrowIfNot<AccelerationUnit>(unit, nameof(unit));
• var typedUnit = Ensure.Is<AccelerationUnit>(unit, nameof(unit));

And what about the case of the IConvertible Methods?

[angularsen]

I don't quite follow on example 1, what doesn't work here?

If [DoesNotReturn] is problematic, then let's just do regular throw CreateX().
It's easier for you with context in the code to see what works or not, I don't really have a strong opinion here, the main benefit is standardizing our exception messages and introducing a few more exception types. Just do what you think works best 👍

[angularsen]

And what about the case of the IConvertible Methods?

I'm not sure I understand the question, what about them? Please mind that I really don't remember our codebase very well without looking at it 🤣

If you are referring to example 1, then I refer to my above question about what doesn't work there.

[lipchev]

And what about the case of the IConvertible Methods?

I'm not sure I understand the question, what about them? Please mind that I really don't remember our codebase very well without looking at it 🤣

If you are referring to example 1, then I refer to my above question about what doesn't work there.

Yes, the code provided in example 1 is the implementation body of the IConvertible methods (repeated a few times per quantity) that works- switching it to something that [DoesNotReturn] doesn't work (what I meant to say is that we have to use the Create approach):

        char IConvertible.ToChar(IFormatProvider? provider)
        {
            ExceptionHelper.ThrowInvalidCastException<Acceleration, char>();
            // still needs to return something here..
        }

However the question I should have probably be asking is - why do we need to implement IConvertible in the first place? I don't really see anything that you don't already have access to via the standard properties. Using it to convert a quantity to a numeric value (without caring about what Unit it was using) is not a "natural conversion". Not to mention that Convert.ToXXX is a huge performance trap for the value types..

I just did a quick test (on the release-v6 version):

• UnitsNet.dll (with the interface): 2.21 MB (2 322 944 bytes)
• UnitsNet.dll (after removing it): 2.05 MB (2 152 448 bytes)

And the number of tests is down from 15373 to 12896..

Now, removing the IConvertable interface would be the worst type of breaking change we can introduce, for somebody who's actually using it for something (as it's going to be a runtime surprise). What do you think, should I create another issue/PR about it?
CC @tmilnthorp

[angularsen]

I don't recall the use case for it, but it seems IConvertible was added in #596.

From skimming the discussion, I don't really see a justification for adding it.
I'm assuming this was intended to be used with some kind of bindings, like WPF UI maybe, where you can automatically convert to compatible types such as the unit, the numeric value, the quantity type enum and more. Maybe this just makes it a little bit easier to bind UI to properties with a quantity value. It could be worth checking if any of our UI-based sample apps use this functionality.

• Is it important to support? I honestly don't know.
• Will it break people? Prrobably some.
• Can we argue that it doesn't provide enough value and should be removed? Maybe, at least if there is an easy workaround for the use cases involved.

@tmilnthorp who originally added it hopefully has some context.

Example:

        object IConvertible.ToType(Type conversionType, IFormatProvider provider)
        {
            if(conversionType == typeof(Acceleration))
                return this;
            else if(conversionType == typeof(AccelerationUnit))
                return Unit;
            else if(conversionType == typeof(QuantityType))
                return Acceleration.QuantityType;
            else if(conversionType == typeof(BaseDimensions))
                return Acceleration.BaseDimensions;
            else
                throw new InvalidCastException($"Converting {typeof(Acceleration)} to {conversionType} is not supported.");
        }

[lipchev]

In v5 this is slightly different w.r.t. the decimal type - the IQuantity.Value returns a double, so you could be losing precision on the BitRate 😄 .
If this was the reason, then it's all good news:

• release-v6 doesn't support the decimals anymore
• fractions-v6 returns the QuantityValue which can be cast to either one (it also has the ToDouble() and ToDecimal() overloads, and it also implements IConvertible and INumber)

[lipchev]

See #1465 as an example of one potential issue related (_not exactly sure how_🤷) to the discussion regarding the IConvertible interface.

I personally am still in favor of removing it, but if not- we should consider adding the following test (in order to complete the coverage):

        [Fact]
        public void Convert_GetTypeCode_Returns_Object()
        {{
            var quantity = {_quantity.Name}.From{_baseUnit.PluralName}(1.0);
            Assert.Equal(TypeCode.Object, Convert.GetTypeCode(quantity));
        }}

[angularsen]

Ok just to circle back a bit. Whether we remove IConvertible or not should not block the exception helpers, let's just assume IConvertible stays for now and make the exception helpers work with that.

Then we can revisit when we decide on the interface.

[lipchev]

Ok, I am leaning towards the return As(Guard.Cast<AccelerationUnit>(unit, nameof(unit))); syntax, with the exception handling as part of the Guard.

This should cover the generic As/ToUnit and the CompareTo cases, and I'll throw in the extra CreateInvalidCastException in it for the IConvertible interface.

This is again slightly off-topic but- here are my two cents on the As(Enum) and the ToUnit(Enum) (explicit) methods:

1. The UnitConverter provides equivalent conversion methods that, when supported, would not throw an exception for conversions between different quantities
2. If the Mass has to implement these two members, then it would be logical to pass in the input to the UnitConverter and let it throw if the conversion is invalid
3. I personally think that it would be better if we only support this type of untyped conversion using the UnitConverter (in my new implementation it's responsible for all conversions anyway)
4. The IQuantity<TUnit, TQuantity>.As(TUnit) and IQuantity<TUnit, TQuantity>.ToUnit(TUnit) can easily be made into extension methods (same as the UnitSystem, the EqualsWithTolerance and everything else that's not bolted to an interface).

[angularsen]

Points 1,2,4 sounds like a good idea 👍

Sentralizing unit conversions in UnitConverter instead of spreading across each quantity implementation sounds like the right direction to move. I also like removing some requirements for IQuantity by moving to extension methods, so HowMuch can become as lean as possible and not have to re-implement things we already provide code for in UnitsNet.

Number 3 I'm a bit unsure about. We do expose Enum Unit {get;} property in IQuantity, and in that sense, it seems reasonable to be able to myQuantity.As(otherUnit) without having to know the enum type. It just flows better when working with IQuantity and generic quantities, I think.

[lipchev]

Number 3 I'm a bit unsure about. We do expose Enum Unit {get;} property in IQuantity, and in that sense, it seems reasonable to be able to myQuantity.As(otherUnit) without having to know the enum type. It just flows better when working with IQuantity and generic quantities, I think.

The property would stay, it's just the method that is moved:

IQuantity someQuantity = Mass.Zero;
Enum unit = _untypedUnitOfMass;
IQuantity newMass = UnitConverter.Convert(someQuantity, unit);

Although, I wouldn't recommend using this approach anyway (there is almost always a generic way of doing this without dropping back to the Enum).. (except maybe WPF/XAML where generics are still not a first class citizen. 😄 )

[angularsen]

I think you are right, moving the implementation out (sentralizing to UnitConverter) is better.
No one is stuck and has a path forward for generic Enum usage, which are very likely a small population of our users anyway.