au icon indicating copy to clipboard operation
au copied to clipboard

Improve error messages when a constructed Quantity truncates

Open hoffbrinkle opened this issue 8 months ago • 4 comments

Given some units defined as such^1:

struct PerMille : decltype(Percent{} / mag<10>());
struct PerMyriad : decltype(PerMille{} / mag<10>());
struct PerCentMille : decltype(PerMyriad{} / mag<10>());
struct PartsPerMillion : decltype(PerCentMille{} / mag<10>());
struct PartsPerBillion : decltype(PartsPerMillion{} / mag<1'000>());
struct PartsPerTrillion : decltype(PartsPerBillion{} / mag<1'000>());
struct PartsPerQuadrillion : decltype(PartsPerTrillion{} / mag<1'000>());
struct PartsPerQuintillion : decltype(PartsPerQuadrillion{} / mag<1'000>());

Trying to assign from a QuantityI64<PartsPerQuintillion> to a QuantityI64<PartsPerBillion> results in an error message that says there isn't a constructor available:

note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'Quantity<av::embedded::firmware::protocol::ptp::PartsPerQuintillion, [...]>' to 'Quantity<av::embedded::firmware::protocol::ptp::PartsPerBillion, [...]>' for 1st argument

Manually requesting the conversion using .as(PartsPerBillion{}) yields a more useful error:

error: static assertion failed due to requirement 'IMPLICIT_OK': Dangerous conversion for integer Rep!  See: https://aurora-opensource.github.io/au/main/troubleshooting/#dangerous-conversion

Which can then be resolved with a .coerce_as. It would be useful to get the conversion error in the first instance.

hoffbrinkle avatar Apr 22 '25 14:04 hoffbrinkle

One thing I was thinking about here: if we simplify this and just delegate to .as, then it will affect the results of std::is_constructible for various Quantity types. The better error messages are nice, but I'm not sure we want to give up the ability to query these traits. Something to think about.

chiphogg avatar May 05 '25 20:05 chiphogg

I tried simplifying the constructors and running our tests, and uncovered some concrete use cases that depend on the status quo.

  • Basically all of chrono_policy_validation_test.cc uses this to test that we support/omit the same operations as the chrono library.
  • Some calls to clamp in math_test.cc --- specifically, those that include exactly one shapeshifter type as one of the three arguments --- become ambiugous if every Quantity appears to be constructible from every other.
  • There are a couple of QuantityPoint tests that explicitly check this trait.

This isn't necessarily a hard deal-breaker, but it shows some of the work we'll need to do and some of the things we'll need to consider if we want to make this happen.

chiphogg avatar May 12 '25 14:05 chiphogg

Ah! I understand better now.

I was confused because I had explicitly deleted the constructor for disallowed conversions, which should give a nice, clean error message.

The real problem was that I made that deleted constructor explicit, which prevents it from being considered in conversions like this. If we take that out, we get something more like this:

Image

Directing the user to the line should be good enough, I hope. We can tweak the accompanying comment. We can also add a section to the troubleshooting doc.

Before I put up a PR for review, I'll try to find time to double check that there's no compile time impact.

chiphogg avatar May 12 '25 20:05 chiphogg

Hmm... this still isn't a complete solution, because it still has the problem with making the clamp calls ambiguous. 🤔

Not yet sure where to go next.

chiphogg avatar May 12 '25 22:05 chiphogg