diff --git a/googlemock/include/gmock/gmock-actions.h b/googlemock/include/gmock/gmock-actions.h index 208295f7..df2effd7 100644 --- a/googlemock/include/gmock/gmock-actions.h +++ b/googlemock/include/gmock/gmock-actions.h @@ -322,6 +322,12 @@ struct is_callable_r_impl>, R, F, Args...> template using is_callable_r = is_callable_r_impl; +// Like std::as_const from C++17. +template +typename std::add_const::type& as_const(T& t) { + return t; +} + } // namespace internal // Specialized for function types below. @@ -872,17 +878,14 @@ class ReturnAction final { public: explicit ReturnAction(R value) : value_(std::move(value)) {} - // Support conversion to function types with compatible return types. See the - // documentation on Return for the definition of compatible. - template + template >, // + negation>, // + std::is_convertible, // + std::is_copy_constructible>::value>::type> operator Action() const { // NOLINT - // Check our requirements on the return type. - static_assert(!std::is_reference::value, - "use ReturnRef instead of Return to return a reference"); - - static_assert(!std::is_void::value, - "Can't use Return() on an action expected to return `void`."); - return Impl(value_); } @@ -918,27 +921,7 @@ class ReturnAction final { // that does `return R()` requires R to be implicitly convertible to // U, and uses that path for the conversion, even U Result has an // explicit constructor from R. - // - // We provide non-const access to input_value to the conversion - // code. It's not clear whether this makes semantic sense -- what - // would it mean for the conversion to modify the input value? This - // appears to be an accident of history: - // - // 1. Before the first public commit the input value was simply an - // object of type R embedded directly in the Impl object. The - // result value wasn't yet eagerly created, and the Impl class's - // Perform method was const, so the implicit conversion when it - // returned the value was from const R&. - // - // 2. Google changelist 6490411 changed ActionInterface::Perform to - // be non-const, citing the fact that an action can have side - // effects and be stateful. Impl::Perform was updated like all - // other actions, probably without consideration of the fact - // that side effects and statefulness don't make sense for - // Return. From this point on the conversion had non-const - // access to the input value. - // - value(ImplicitCast_(input_value)) {} + value(ImplicitCast_(internal::as_const(input_value))) {} // A copy of the value originally provided by the user. We retain this in // addition to the value of the mock function's result type below in case @@ -1763,7 +1746,7 @@ internal::WithArgsAction::type> WithoutArgs( // * U is not void. // * U is not a reference type. (Use ReturnRef instead.) // * U is copy-constructible. -// * R& is convertible to U. +// * const R& is convertible to U. // // The Action object contains the R value from which the U return // value is constructed (a copy of the argument to Return). This means that the diff --git a/googlemock/test/gmock-actions_test.cc b/googlemock/test/gmock-actions_test.cc index 59ee86b9..414c615a 100644 --- a/googlemock/test/gmock-actions_test.cc +++ b/googlemock/test/gmock-actions_test.cc @@ -654,8 +654,8 @@ TEST(ReturnTest, AcceptsStringLiteral) { TEST(ReturnTest, SupportsReferenceLikeReturnType) { // A reference wrapper for std::vector, implicitly convertible from it. struct Result { - std::vector* v; - Result(std::vector& v) : v(&v) {} // NOLINT + const std::vector* v; + Result(const std::vector& v) : v(&v) {} // NOLINT }; // Set up an action for a mock function that returns the reference wrapper @@ -709,6 +709,56 @@ TEST(ReturnTest, PrefersConversionOperator) { EXPECT_THAT(mock.AsStdFunction()(), Field(&Out::x, 19)); } +// Return(x) should not be usable with a mock function result type that's +// implicitly convertible from decltype(x) but requires a non-const lvalue +// reference to the input. It doesn't make sense for the conversion operator to +// modify the input. +TEST(ReturnTest, ConversionRequiresMutableLvalueReference) { + // Set up a type that is implicitly convertible from std::string&, but not + // std::string&& or `const std::string&`. + // + // Avoid asserting about conversion from std::string on MSVC, which seems to + // implement std::is_convertible incorrectly in this case. + struct S { + S(std::string&) {} // NOLINT + }; + + static_assert(std::is_convertible::value, ""); +#ifndef _MSC_VER + static_assert(!std::is_convertible::value, ""); +#endif + static_assert(!std::is_convertible::value, ""); + + // It shouldn't be possible to use the result of Return(std::string) in a + // context where an S is needed. + using RA = decltype(Return(std::string())); + + static_assert(!std::is_convertible>::value, ""); + static_assert(!std::is_convertible>::value, ""); +} + +// Return(x) should not be usable with a mock function result type that's +// implicitly convertible from decltype(x) but requires an rvalue reference to +// the input. We don't yet support handing over the value for consumption. +TEST(ReturnTest, ConversionRequiresRvalueReference) { + // Set up a type that is implicitly convertible from std::string&& and + // `const std::string&&`, but not `const std::string&`. + struct S { + S(std::string&&) {} // NOLINT + S(const std::string&&) {} // NOLINT + }; + + static_assert(std::is_convertible::value, ""); + static_assert(!std::is_convertible::value, ""); + + // It shouldn't be possible to use the result of Return(std::string) in a + // context where an S is needed. + using RA = decltype(Return(std::string())); + + static_assert(!std::is_convertible>::value, ""); + static_assert(!std::is_convertible>::value, ""); +} + // Tests that Return(v) is covaraint. struct Base { @@ -760,19 +810,6 @@ TEST(ReturnTest, ConvertsArgumentWhenConverted) { << "when performed."; } -class DestinationType {}; - -class SourceType { - public: - // Note: a non-const typecast operator. - operator DestinationType() { return DestinationType(); } -}; - -TEST(ReturnTest, CanConvertArgumentUsingNonConstTypeCastOperator) { - SourceType s; - Action action(Return(s)); -} - // Tests that ReturnNull() returns NULL in a pointer-returning function. TEST(ReturnNullTest, WorksInPointerReturningFunction) { const Action a1 = ReturnNull();