Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] UFCS disable [[nodiscard]] #305

Closed
filipsajdak opened this issue Mar 28, 2023 · 7 comments
Closed

[BUG] UFCS disable [[nodiscard]] #305

filipsajdak opened this issue Mar 28, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@filipsajdak
Copy link
Contributor

In the current implementation of cppfront (aa70d09), the following code:

t2 : type = {
    public x : *int;

    operator=:(out this, p : *int) = {
        x = p;
    }

    ptr1: (inout this, p : *int) -> *int = std::exchange(x, p);
}

ptr_ext: (inout o : t2, p : *int) -> *int = std::exchange(o.x, p);

main :() = {
    n := 42;
    a : t2 = (n&);
    m := 24;
    a.ptr1(m&); // no warning

    ptr_ext(a, m&); // warning: ignoring return value of function declared with 'nodiscard' attribute

    a;
}

Generates (skipping boilerplate):

class t2   {
    public: int* x; 

    public: explicit t2(cpp2::in<int*> p)
        : x{ p }
#line 4 "../tests/bug_ufcs_disable_nodiscard.cpp2"
                                   {

    }
#line 4 "../tests/bug_ufcs_disable_nodiscard.cpp2"
    public: auto operator=(cpp2::in<int*> p) -> t2& {
        x = p;
        return *this;
#line 6 "../tests/bug_ufcs_disable_nodiscard.cpp2"
    }

    public: [[nodiscard]] auto ptr1(cpp2::in<int*> p) -> int* { return std::exchange(x, p); }
};

[[nodiscard]] auto ptr_ext(t2& o, cpp2::in<int*> p) -> int* { return std::exchange(o.x, p); }

auto main() -> int{
    auto n {42}; 
    t2 a {&n}; 
    auto m {24}; 
    CPP2_UFCS(ptr1, a, &m);// no warning

    ptr_ext(a, &m); // warning

    std::move(a);
}

And when compile with cpp1 compiler (clang++-14 in my case):

../tests/bug_ufcs_disable_nodiscard.cpp2:19:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
    ptr_ext(a, &m); // warning
    ^~~~~~~ ~~~~~
1 warning generated.

Only one unused result is identified - CPP2_UFCS(ptr1, a, &m); generates no warning.

The issue is caused by CPP2_UFCS() macros are eventually lambdas called in-place (Immediately Invoked Function Expressions). The lambda has no [[nodiscard]] attribute - this is already proposed: P2173R0. I have checked on my compiler - I modified the macro to (notice that [[nodiscard]] is added after the capture list):

#define CPP2_UFCS(FUNCNAME,PARAM1,...) \
[&][[nodiscard]](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
    if constexpr (requires{ std::forward<decltype(obj)>(obj).FUNCNAME(std::forward<decltype(params)>(params)...); }) { \
        return std::forward<decltype(obj)>(obj).FUNCNAME(std::forward<decltype(params)>(params)...); \
    } else { \
        return FUNCNAME(std::forward<decltype(obj)>(obj), std::forward<decltype(params)>(params)...); \
    } \
}(PARAM1, __VA_ARGS__)

And the result is:

../tests/bug_ufcs_disable_nodiscard.cpp2:17:5: warning: an attribute specifier sequence in this position is a C++2b extension [-Wc++2b-extensions]
    CPP2_UFCS(ptr1, a, &m);// no warning
    ^
include/cpp2util.h:651:4: note: expanded from macro 'CPP2_UFCS'
[&][[nodiscard]](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
   ^
../tests/bug_ufcs_disable_nodiscard.cpp2:17:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
    CPP2_UFCS(ptr1, a, &m);// no warning
    ^~~~~~~~~~~~~~~~~~~~~~
include/cpp2util.h:657:2: note: expanded from macro 'CPP2_UFCS'
}(PARAM1, __VA_ARGS__)
~^~~~~~~~~~~~~~~~~~~~~
../tests/bug_ufcs_disable_nodiscard.cpp2:19:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
    ptr_ext(a, &m); // warning
    ^~~~~~~ ~~~~~
3 warnings generated.
@JohelEGP
Copy link
Contributor

The lambda has no [[nodiscard]] attribute - this is already proposed: P2173R0.

And implemented because it was accepted, as shown by the result that follows it.
1680044170
1680044182

@JohelEGP
Copy link
Contributor

JohelEGP commented May 1, 2023

There's already macros for annotating functions in cpp2util.h, so I think you can make a PR for this.

@filipsajdak
Copy link
Contributor Author

I guess that it will not be accepted until MSVC does not have support for that.

@JohelEGP
Copy link
Contributor

JohelEGP commented May 6, 2023

It's OK. That's why they're macros.

@hsutter
Copy link
Owner

hsutter commented May 29, 2023

For this one, I'm not sure what the right resolution is. We could make the UFCS macros unconditionally expand to include [[nodiscard]] on compilers that support that. But then a function that has a discardable result (which Cpp2 doesn't yet support authoring) would gain a [[nodiscard]]. That may not be such a bad thing -- it could be viewed as an improvement when using most Cpp1 functions from Cpp2. Thoughts?

(General note: I'm distracted with the upcoming WG21 Varna meeting and will be slow to follow up on issues/PRs until after that.)

@hsutter
Copy link
Owner

hsutter commented Jun 12, 2023

On the eve of the Varna meeting, I've made enough progress on this question over the past couple of weekends that I now think I have an answer, for which I've:

Thanks everyone! I look forwarding to picking up on handling PRs in 2-3 weeks after we all recover from the WG 21 Varna meeting...

@JohelEGP
Copy link
Contributor

The Wiki's Home.md says
"Cpp2 [[nodiscard]]" rather than
"Cpp1 [[nodiscard]]" like the linked article.


  • There is one way to discard: Assign to the _ "don't care" wildcard. For example: _ = func();. This is natural because Cpp2 already uses _ as a "don't-care" wildcard everywhere, and even if I didn't support that, users would try to write it.

This isn't the case for parameters.

I've been wondering how Cpp2 should support unnamed parameters and function type-ids.
The simplest design would have _ to discard a parameter's name.
And maybe require parameters of function type-ids to have discarded names.

See also 3 different design suggestions that support
unnamed parameters (by literally omitting the identifier)
and function type-ids:


Alternatives considered

For some of the interim design discussion, see the Issue #305 comment thread.

Shouldn't this link to #231 instead?

@JohelEGP JohelEGP mentioned this issue Oct 30, 2023
zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
Support `_ =` as a discard operation
	- for example,  `_ = vec.emplace_back(a,b,c);`
	- emitted as `(void)`, not `std::ignore =` because the latter doesn't work with `void` returns and we want to support generic code where there might or might not be a return value in a given template instantiation

Make UFCS calls unconditionally add [[nodiscard]] always
	- note: only added on compilers that support C++23's P2173

Take back `_ :=` as an anonymous deduced variable syntax - the name can be anonymous or the type can be anonymous, but not both
	- yes, it would be easy to support making both anonymous (it did work before this commit, which had to explicitly add a check to disable it)
	- rationale: if that were allowed to keep the returned object alive, that syntax would be dangerously close to '_ = f();' to discard the returned object, which is exactly opposite in a fundamentally important way (lifetime!) and such importantly opposite meanings deserve more than a one-character typo distance
	- explicit discarding gets the nice syntax because it's likely more common
	- but this shouldn't be any hardship because cases that use a type are still just as easy, e.g., `lock_guard`; before this commit `_ := lock_guard(mut);` worked, and after this commit that's disallowed but it's just as easy to write `_: lock_guard = mut;`
	- to make this natural also for `finally`, I just got rid of the `finally` and `finally_success` helper functions, and gave those names to the types... it's a simpler design and I think an improvement in its own right, and `_: finally = code;` is slightly simpler to write (e.g., saves `(` `)` `;`, for example see `pure2-types-order-independence-and-nesting.cpp2`)

Remove return type from `contract_group::set_handler`
	- not needed, since we already have `get_handler` if the current value is of interest

Add reflection `default_to_*` functions for `make_*` functions that ignore the returned bool
	- arguably a clearer API, makes calling code intent more readable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants