port.set(0) in Cpp target generates compile error "ambiguous call of overloaded function"

[elgeeko1]

Issue: Setting an output port with integral type to the value 0 produces a compile error, unable to disambiguate the call to set.

LF version: 0.7.0 stable

Workaround 1: Statically cast .set(static_cast<uint32_t>(0))

Workaround 2: Comment out the line void set(std::nullptr_t) = delete; from port.hh and run make.

Steps to recreate:

target Cpp;
reactor A {
    output a: uint32_t;

    reaction(startup) -> a {=
        a.set(75);
    =}

    reaction(shutdown) -> a {=
        a.set(0);
    =}
}

The line a.set(0) produces the following compile error:

A.cc: In member function ‘void A::Inner::reaction_2(const reactor::ShutdownTrigger&, reactor::Output<unsigned int>&)’:
A.cc:86:14: error: call of overloaded ‘set(int)’ is ambiguous
   86 |   a.set(0);
      |   ~~~~~~~~~~~^~~
In file included from src-gen/reactor-cpp-default/include/reactor-cpp/connection.hh:18,
                 from src-gen/reactor-cpp-default/include/reactor-cpp/reactor-cpp.hh:14,
                 from A.hh:9,
                 from A.cc:7:
src-gen/reactor-cpp-default/include/reactor-cpp/port.hh:134:8: note: candidate: ‘void reactor::Port<T>::set(const T&) [with T = unsigned int]’
  134 |   void set(const T& value) { set(make_immutable_value<T>(value)); }
      |        ^~~
src-gen/reactor-cpp-default/include/reactor-cpp/port.hh:135:8: note: candidate: ‘void reactor::Port<T>::set(T&&) [with T = unsigned int]’
  135 |   void set(T&& value) { set(make_immutable_value<T>(std::forward<T>(value))); }
      |        ^~~
src-gen/reactor-cpp-default/include/reactor-cpp/port.hh:137:8: note: candidate: ‘void reactor::Port<T>::set(std::nullptr_t) [with T = unsigned int; std::nullptr_t = std::nullptr_t]’ (deleted)
  137 |   void set(std::nullptr_t) = delete;
      |        ^~~
make[2]: *** [App/CMakeFiles/App.dir/build.make:76: App/CMakeFiles/App.dir/App/A.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:160: App/CMakeFiles/App.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

Note that the port set in the startup reaction does not produce an error, so it is value-dependent.

In the reactor-cpp file port.hh, the following overloads for set are defined:

void set(MutableValuePtr<T>&& value_ptr) { set(ImmutableValuePtr<T>(std::forward<MutableValuePtr<T>>(value_ptr))); }
  void set(const T& value) { set(make_immutable_value<T>(value)); }
  void set(T&& value) { set(make_immutable_value<T>(std::forward<T>(value))); }
// Setting a port to nullptr is not permitted.
  void set(std::nullptr_t) = delete;

It appears that the value 0 may be castable to std::nullptr_t, which makes sense as legacy C code defines NULL as 0. This creates the ambiguous overload.

[cmnrd]

This is a very interesting find. Thanks for reporting! I am still puzzeled that this wasn't noticed earlier... and I am not sure how to handle this, or if this is a bug or a feature. It comes down to how we define the semantics of seting or unsetting a port. Currently, in the C++ runtime, we detect if a port is present based on whether it carries a value or not (in which case the value pointer is nullptr). If we allow setting a port to nullptr, then it it is possible to first set a value to the port (which triggers downstream reactions) and then unset the port. In this case, downstream reactions would be triggered on a port that has no present value. We could allow this corner case, but I am not sure if that is what we want. I guess a middle ground could be to remove the static compiler check for nullptr_t and instead add an assertion to the implementation of set that checks for nullptr and produces a runtime error.

Any thoughts on this @lhstrh, @edwardalee?

[cmnrd]

Another issue is, that I don't know which type the C++ compiler would assign to the 0 literal if we allow set(nullptr_t). We definitely do not want set(0) to be equivalent to set(nullptr). set(0) is expected to create a smart pointer that points to the value 0 and then assign the smart pointer to the port. In contrast, set(nullptr) would mean, null the smart pointer and thus remove the current value (if any) from the port. So I am not sure if there is a way for us to safely resolve the type ambiguity.

[edwardalee]

I would like to see the Cpp target support persistent data, in which case present or absent needs to be encoded some other way. I find this a very useful feature in the C target.

[cmnrd]

Yes, implementing persistent ports would actually be relatively simple. However, we should do this conciousiously and have some wider discussion about it. It also comes at the expense of potentially freeing memory for large data structures that are relayed via ports only late (when the value is overwritten vs at the end of the tag).

However, persistent ports actually do not solve this particular problem. The problem is that a smart pointer to the value 0 is semantically different from a nullptr. I am beginning to doubt that this issue is solvable, and it seems to be a quirk of the C++ language that we need to accept. The root cause of the ambiguity is that you can provide both a value and a smart pointer to the value to the set method. The following calls are equivalent:

  a.set(make_immutable_value<uint32_t>(42));  // assign smart pointer to 42
  a.set(42);  // implicitly create a smart pointer that points to the value 42

However, in the case of 0 the compiler does not know whether we want to use the auto-wrapping or if it should interpret 0 as a null pointer.
A potential solution would be to remove the overload that accepts plain values (which automatically wraps the value in a smart pointer) and instead only accept smart pointers. But that would make setting values a lot more verbose.

[lhstrh]

Or we could have a shorthand setZero function to do the static cast?

[cmnrd]

Yes, that could work. Perhaps it wasn't the best idea to overload set. An alternative to the special set_zero would be to distinguish set_ptr and set_value (and don't use set).

[elgeeko1]

A very C++ way to solve this is template SFINAE,

#include <iostream>

#include <type_traits>

template <typename T>
class Port {
public:
    Port() = default; 

    void set(const T& value) {
        std::cout << "called (1)" << std::endl;
    }

    void set(T&& value) {
        std::cout << "called (2)" << std::endl;
    }

    template <typename V, typename = std::enable_if_t< std::is_same_v<V, std::nullptr_t> >>
    void set(V) = delete;
};

int main(void) {
    Port<int> p_int;
    Port<std::string> p_str;

    int value = 0;
    p_int.set(value);   // called (1)
    p_int.set(0);       // called (2)

    // uncommenting below produces compile error
    // 'use of deleted function'
    // p_int.set(nullptr);

    std::string str("hello");
    p_str.set(str);         // called (1)
    p_str.set("hello");     // called (2)

    // uncommenting below produces compile error
    // 'use of deleted function'
    // p_str.set(nullptr);
    
    return 0;
}

[cmnrd]

Oh, interesting. That would indeed be a workable solution. I didn't know that enable_if could be used in this way.