feature request - allow disabling floating point implicit conversion

[ghorn]

au permits implicit floating point conversions by design, including conversions which reduce precision.

GCC and Clang have a -Wfloat-conversion flag (implied by -Wconversion) which prevents implicit conversions from double to float. Clang-tidy has the cppcoreguidelines-narrowing-conversions check (aliased by bugprone-narrowing-conversions) which also enforces this.

In a codebase with these warnings/checks enable, it was surprising to me when a au::Quantity<Unit, double> was implicitly cast to au::Quantity<Unit, float>. It led to a subtle bug.

I'm guessing that there is no desire to remove the implicit conversions feature. On the other hand, in a codebase with -Wfloat-conversion / cppcoreguidelines-narrowing-conversions, the au::Quantity implicit conversions are essentially a "leak". What are your thoughts on this situation? How could it be possible for a codebase to disallow these implicit conversions? What would you think of something like a -DAU_NO_IMPLICIT_FLOAT_CONVERSIONS flag?

[chiphogg]

What a great question! You're right that this behavior was by design. And your argument about -Wfloat-conversion is a very compelling reason to revisit that design.

Au 0.5.0 added some new truncation docs that discuss our philosophy in more detail. I think the flaw in the doc's argument is that it only really works when you go from integers to floating point. Going from one floating point type, to another with wider tolerance (lower precision), probably is morally equivalent to truncation.

As to a flag based approach: that's something we strongly want to avoid. We don't have any configurable preprocessor macros in the library, and we'd have a pretty high bar to change that, as it feels like a slippery slope. So, assuming we don't add a flag, what are our options?

1. Stick to the status quo.
2. Treat narrowing float-to-float conversions as "truncating" everywhere.
3. Find some way to "hang" a customizable default policy on either the U or R template parameter.
4. Add a new operation for "implicit conversion", as an alternative to the "static cast" operation

(1) is definitely a live possibility, but I would rather find a way to satisfy your request if at all feasible. (2) would incur a tremendous upgrade burden, and probably also an ongoing usability burden: it's very common to initialize a float from a literal without an f suffix, for example. (3) is a possibility I've thought of for a variety of reasons over time, as a possible evolution direction for the library. mp-units has a "quantity spec" instead of our "unit", and it's a great design! Probably what I would do if I were starting all over. But even if we did this, it wouldn't be a "project wide" setting; you'd have to pick specific "strict" versions of units, or wrapped rep types. So it probably entails too much manual work to be appealing to you.

I think (4), which I just thought of now, may be the most interesting idea, if we can get it to work. We've got this brand new mechanism in 0.5.0 where every unit conversion is a sequence of abstract operations: static casts, multiplications, and dividing by integers. The "static cast" operation was basically standing in for all type conversions, but it doesn't have to be like this. Maybe we could add an "implicit conversion" operation, if we could reliably tell the difference from the invoking context (which I think we could).

The ideal end state would be that for users with -Wconversion enabled, we would raise compiler warnings/errors in exactly the same situations for Quantity as for the underlying raw numeric types. And for users without this warning enabled, the compiler would continue to accept them without complaint.

I'll put it in the 0.6.0 milestone not because I expect to be able to deliver it, but so that I can explore our options more fully. Of course, if we do deliver it, that'll be great. 🙂

[geoffviola]

Good discussion 😄 There is some overlap with MISRA C++ 2023 guidelines.

Rule 7.0.6 Assignment between numeric types shall be appropriate
...
... the source size shall be smaller than the target size...
...

flt2 = 0.0;  // Non-compliant - different sizes and not an
             // integral constant expression
flt3 = 0.0f; // Compliant

[ghorn]

The ideal end state would be that for users with -Wconversion enabled, we would raise compiler warnings/errors in exactly the same situations for Quantity as for the underlying raw numeric types.

I agree, that would be ideal. I assume the approach would be to have the conversion happen implicitly as opposed to with static_cast, is that right? I'm unclear exactly how it this works, but third party libraries are often included with -isystem and #include <>, which masks compiler warnings in those headers. Would -isystem mask the warning in this case? Additionally, I've noticed that some conversion operations in template functions are treated differently and more permissively by static analysis, I'm not sure if this case is affected.

If this approach wouldn't work for these or other reasons, I don't know of another approach to get this behavior, do you?

Question - if we took the approach in (2), would it be forcing stricter -Wconversion behavior on users who are not using that warning flag?

(2) would incur a tremendous upgrade burden, and probably also an ongoing usability burden: it's very common to initialize a float from a literal without an f suffix, for example.

I agree, though anecdotally the big codebase I work on prevents this by linting.

As to a flag based approach: that's something we strongly want to avoid

I think it's wise to do as little as possible in the pre-processor, though I wonder if it's the least evil way to support "opt-in" behavior. The correct opt-in approach would be (3), right? That seems like a major piece of work, no?

If we wanted to hack in this behavior today, I wonder if there is a relatively simple patch we could apply. Does the floating truncation happen in a single place, for instance as a template specialization, where we could just patch it out? That would be similar to a flag-based approach, except completely unsupported. It would be useful for an audit of our codebase if not for general usage.

[chiphogg]

Thanks for your patience! I now have a working proof of concept to resolve this issue. My plan is for the implicit constructor to use implicit conversions under the hood, while .in and .as, being more explicit, will still use static_cast. The implementation looks very promising: it caused a fair number of existing Au test cases to fail when run under -Wconversion, and I had to move them to "no-wconversion"-tagged tests.

Right now, we've got an Aurora-internal issue preventing us from using the new bzlmod-based version of Au. Once we sort that out, I want to run this change on Aurora's entire codebase, just to double check that there aren't any nasty surprises --- since this affects such a fundamental part of the library. But I'm not anticipating that we'll find any show stoppers, so I expect we can resolve this soon. I'll put the change out for review once it's passed this test.

Another outcome of this issue should be a doc that clearly lays out Au's philosophy: Au will not add or subtract compiler warnings or errors. That is what we aspire to. We already had a decent handle on the "don't add" part, but this issue showed that "don't subtract" is the critical other side of the coin. Ultimately, we want to control our warning policies by the same mechanisms (compiler flags) as for raw numeric types.

The problem with this is exactly what you pointed out: the -isystem setting. We will have to instruct users to configure Au not to use this setting in their project. If they follow that advice, but Au adds or subtracts errors, then that is an Au bug, which we would need to fix. I can understand why -isystem makes sense, given that many third party libraries didn't build with a lot of warning flags, or didn't see warnings as a big deal. But I hope we can be held to a higher standard.

[chiphogg]

Aurora-internal testing is done; no surprises. I think this is ready for review, and I'll start putting the PRs out.