[cpp2util] as: add constexpr branch for any

sookach commented 2 months ago

This patches #961. The issue lies in the fact that the as cast for any is defined at https://github.com/hsutter/cppfront/blob/5663493860a5558a3d64c59ac9ee6f0e26dedf99/include/cpp2util.h#L1625-L1630 so the any call hits https://github.com/hsutter/cppfront/blob/5663493860a5558a3d64c59ac9ee6f0e26dedf99/include/cpp2util.h#L1351-L1353 even though the code provided in the issue is well within the specifications outlined at https://hsutter.github.io/cppfront/cpp2/expressions/

This pr adds another constexpr branch with the same requirements as the function (I think it is the simplest solution). Let me know if you have any suggestions. Thanks!

sookach commented 2 months ago

@wolfseifert

sookach commented 2 months ago

@filipsajdak tagging you because I'm not sure who the code owner is

filipsajdak commented 2 months ago

I will take a look at this issue during the weekend.

sookach commented 2 months ago

Thanks!

wolfseifert commented 2 months ago

I can confirm that this PR resolves #961.

As a sole user (tester) of Cpp2 I cannot tell whether the proposed patch fits into cppfront's code design.

hsutter commented 1 month ago

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've sent the Contributor License Agreement (CLA) to your email, and once it's signed I can look at your pull request. Thanks again for your contribution.

hsutter commented 1 month ago

@filipsajdak I also want to make sure this doesn't conflict with your #1053?

sookach commented 1 month ago

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've sent the Contributor License Agreement (CLA) to your email, and once it's signed I can look at your pull request. Thanks again for your contribution.

Awesome! Done.

hsutter commented 1 month ago

Thanks! This PR would fix the problem, but it does it by repeating the as that already exists for std::any inside the general as. My aim was to demonstrate extensibility for user-defined is and as by having the standard library types' is and as be separate, and not built-in to the "language" in the language-support default implementations.

But it provides a clue to the problem: There is already an as for std::any later, which ought to be being called, but isn't.

We have these two functions, which differ mainly in the constness of the parameter:

template< typename C, typename X >
auto as(X const& x CPP2_SOURCE_LOCATION_PARAM_WITH_DEFAULT) -> decltype(auto) {
    if constexpr (
        std::is_floating_point_v<C> &&
        std::is_floating_point_v<CPP2_TYPEOF(x)> &&
        sizeof(CPP2_TYPEOF(x)) > sizeof(C)
    )
    {
        return CPP2_COPY(nonesuch);
    }
    //  Signed/unsigned conversions to a not-smaller type are handled as a precondition,
    //  and trying to cast from a value that is in the half of the value space that isn't
    //  representable in the target type C is flagged as a type_safety contract violation
    else if constexpr (
        std::is_integral_v<C> &&
        std::is_integral_v<CPP2_TYPEOF(x)> &&
        std::is_signed_v<CPP2_TYPEOF(x)> != std::is_signed_v<C> &&
        sizeof(CPP2_TYPEOF(x)) <= sizeof(C)
    )
    {
        const C c = static_cast<C>(x);
        type_safety.enforce(   // precondition check: must be round-trippable => not lossy
            static_cast<CPP2_TYPEOF(x)>(c) == x && (c < C{}) == (x < CPP2_TYPEOF(x){}),
            "dynamic lossy narrowing conversion attempt detected" CPP2_SOURCE_LOCATION_ARG
        );
        return CPP2_COPY(c);
    }
    else if constexpr (std::is_same_v<C, std::string> && std::is_integral_v<X>) {
        return cpp2::to_string(x);
    }
    else if constexpr (std::is_same_v<C, X>) {
        return x;
    }
    else if constexpr (std::is_base_of_v<C, X>) {
        return static_cast<C const&>(x);
    }
    else if constexpr (std::is_base_of_v<X, C>) {
        return Dynamic_cast<C const&>(x);
    }
    else if constexpr (
        std::is_pointer_v<C>
        && std::is_pointer_v<X>
        && requires { requires std::is_base_of_v<deref_t<X>, deref_t<C>>; }
    )
    {
        return Dynamic_cast<C>(x);
    }
    else if constexpr (requires { C{x}; }) {
        //  Experiment: Recognize the nested `::value_type` pattern for some dynamic library types
        //  like std::optional, and try to prevent accidental narrowing conversions even when
        //  those types themselves don't defend against them
        if constexpr( requires { requires std::is_convertible_v<X, typename C::value_type>; } ) {
            if constexpr( is_narrowing_v<typename C::value_type, X>) {
                return nonesuch;
            }
        }
        return C{x};
    }
    else {
        return nonesuch;
    }
}

template< typename C, typename X >
auto as( X& x ) -> decltype(auto) {
    if constexpr (std::is_same_v<C, X>) {
        return x;
    }
    else if constexpr (std::is_base_of_v<C, X>) {
        return static_cast<C&>(x);
    }
    else if constexpr (std::is_base_of_v<X, C>) {
        return Dynamic_cast<C&>(x);
    }
    else {
        return as<C>(std::as_const(x));
    }
}

Questions I'm investigating... @JohelEGP and @filipsajdak I'd value your observations please:

These two function bodies are similar but different, even though their interfaces seem to mainly differ only in constness. Why does the second one have only a few of the cases of the first one? I think @JohelEGP most recently touched this in #956.
If I move just the second (shorter) function to after the as for std::any, the example in #961 works fine and calls the as for std::any. But shouldn't it already be doing that in its current source location, given that as<C>(std::as_const(x)) is a dependent call (or is the point of instantiation somewhere in between somehow)?

sookach commented 1 month ago

This PR would fix the problem, but it does it by repeating the as that already exists for std::any inside the general as.

100% agree. The change I made is not the nicest approach, and for something as critical to the language as casting and type information it makes sense to take the time to come up with as clean of a solution as possible.

My aim was to demonstrate extensibility for user-defined is and as by having the standard library types' is and as be separate, and not built-in to the "language" in the language-support default implementations.

Ah, thanks for the info. I think your reasoning and aim make complete sense and the modularity/extensibility is something I would definitely prefer as a contributor and especially as a user.

But it provides a clue to the problem: There is already an as for std::any later, which ought to be being called, but isn't.

Honestly, even if the code change is way off the mark and ends up being replaced, figuring out the source of the issue would make this PR worthwhile in my opinion.

Curious to see what the resolution ends up being since we might be able to kill two birds with one stone (#1062 follows a similar pattern) by fixing this bug.

sookach commented 1 month ago

I've been playing around a bit and found that removing

https://github.com/hsutter/cppfront/blob/36046264f49d8f58cf318643dfb235f0db696909/include/cpp2util.h#L1533-L1547

fixes both this issue and #1061

hsutter commented 1 month ago

Ah, right... it seems that the second as( X& x ) for non-const lvalues is a better match and hides the later overload that's intended for any which takes const&:

template<typename T, typename X>
    requires (!std::is_reference_v<T> && std::is_same_v<X,std::any> && !std::is_same_v<T,std::any>)
constexpr auto as( X const& x ) -> T
    { return std::any_cast<T>( x ); }

Archaeological spelunking in the pointer/reference casts

Disclaimer: The following reconstruction may not be exactly right. This is mostly notes-in-progress for myself to remember how we got here.

Aug 2022

Originally I added the const and non-const overloads for the pointer/reference cases to make the casts return the correctly-constified type:

template< typename C, typename X >
    requires (std::is_base_of_v<X, C> && !std::is_same_v<C,X>)
auto as( X& x ) -> C& {
    return dynamic_cast<C&>(x);
}

template< typename C, typename X >
    requires (std::is_base_of_v<X, C> && !std::is_same_v<C,X>)
auto as( X const& x ) -> C const& {
    return dynamic_cast<C const&>(x);
}

template< typename C, typename X >
    requires (std::is_base_of_v<X, C> && !std::is_same_v<C,X>)
auto as( X* x ) -> C* {
    return dynamic_cast<C*>(x);
}

template< typename C, typename X >
    requires (std::is_base_of_v<X, C> && !std::is_same_v<C,X>)
auto as( X const* x ) -> C const* {
    return dynamic_cast<C const*>(x);
}

Jan 2024, PR 956

Then #956 refactored this, but ended up with two functions, pretty much as it is now that I pasted earlier above:

the one for const& that had all the pointer/reference casts but also more cases/logic (e.g., merged in signedness logic)
the one for & that only has reference cases (not pointer casts or the other)

So we want to finish #956's work in unifying these.

I think we don't want to just duplicate all the other logic from the const& overload in the & overload, because then those two will be consistent but the & overload will still hide customizations like the one for any.

I think instead we want to drop the & overload (as suggested above) and change the const& one to be a T&& forwarder and then do the correct const propagation in the cast sections of that function. That might work... it would make these two consistent (by unifying them) and shouldn't hide the customizations which will still be a better match I think (unless forwarders are even more greedy than I remember... I guess I'm about to find out).

That's a snapshot of in-progress thinking... now to actually try the latter , will report back...

hsutter commented 1 month ago

Thanks! Even though I didn't take this PR as written, it illuminated the problem and helped more quickly narrow down what the problem was. The problems this PR was targeting should now be fixed with the above commit, please check.

Again, thanks.

hsutter / cppfront