Improving diagnostics with "Reasons:" and "Suggestions:"

[JohelEGP]

Title: Improving diagnostics with "Reasons:" and "Suggestions:".

Description:

Updating my code, I kept hitting:

cppfront/source/to_cpp1.h

Line 2433 in 68b716a

"a 'forward' return type cannot return an 'in', 'copy', or 'move' parameter"

I'm doubting the usefulness of this error as it is.

It's preventing me from writing

export from_window: (v) = v.from(window());

which copies v, a vector, into a point from window to v.

Following the comment a bit above:

cppfront/source/to_cpp1.h

Lines 2411 to 2412 in 68b716a

	// If we're doing a forward return of a single-token name
	// (the entire expression is a postfix-expression)

I parenthesized the expression, which made it it OK.

Can we update these checks?
copy seems to be as safe as any local variable
(https://cpp2.godbolt.org/z/Y8nahPjhh works from Clang 12 and GCC 10 in C++20, to their HEAD versions in C++26).

Kind	Return	Action
in	param	Suggest in_ref.
in	param.X()	Suggest (expr) if param is not part of the return value, and in_ref param otherwise (maybe note that in can optimize to pass by copy).
copy	param	Allow.
copy	param.X()	Allow.
move	param	Suggest -> _, or -> cpp1_rvalue_ref<ParamType>, or allow -> forward_ref _ and also suggest it.
move	param.X()	Suggest (expr) if param is not part of the return value.

Sometimes, I'm left wondering the reason for some of the rejections.
It'd be nice to centralize the documentation of how Cpp2 features obsolete guidance.
That would make it easier to challenge some of decisions, and maybe even help notice further improvements.

For example, given the table above:

Kind	Return	Effect
in	param	Rejected because in can optimize to pass by copy. Use in_ref instead.
in	param.X()	Rejected because the identity of param might live in the resulting value of the expression.
copy	param	Allow for being as safe as using any local variable instead of param.
copy	param.X()	Allow for being as safe as using any local variable instead of param.
move	param	Rejected because the corresponding argument may be a prvalue.
move	param.X()	Rejected because the identity of param might live in the resulting value of the expression.

In fact, this reminds me of some talk(s) where the speaker(s) mention
how the compilers of other programming languages have much nicer diagnostics.
Something like how those compilers are a friend in the development journey rather than a tool.

Taking all of the above, consider the following Cpp2 code fragment and the contents of a desired diagnostic (https://cpp2.godbolt.org/z/n85qhcjbj).

  private validate: (this) -> bool = std::all_of(contents.begin(), contents.end(), :(x) = x.meets_the_requirements());

main.cpp2(4,91): error: a 'forward' return type cannot return an 'in' parameter
- Reason:
    `in param` can optimize to pass by copy (<https://hsutter.github.io/cppfront/cpp2/functions/#parameters>).
    In that case, returning `param` would return a [dangling reference](https://en.cppreference.com/w/cpp/language/reference#Dangling_references).
- Suggestion(s):
    - If the returned expression doesn't reference `param`,
      wrap it in parentheses to indicate so by convention.
    - If the returned expression references `param`,
      change its declaration to `in_ref param`.
- Note(s): `-> forward _` is implicit because the statement of the function is an expression.
           `:(   x   )              =          x.meets_the_requirements()`    expands to
           `:(in x: _) -> forward _ = { return x.meets_the_requirements(); }`
                       ~~~^~~~~~~~~

PS: I don't think this is a [SUGGESTION].

[JohelEGP]

Some more code:

      if member_objects.contains(first*.arguments, :(x) = x.name()) { // Safe but rejected.
      if member_objects.contains(first*.arguments, :(x) = (x.name())) { // Drowning in parentheses.
      if member_objects.contains(first*.arguments, :(x) -> _ = x.name()) { // The unterse.

  for std::views::iota(0, 5) do (i) test(count_n(i), :(x) = x.std::min(i$)); // Safe but rejected.
  for std::views::iota(0, 5) do (i) test(count_n(i), :(x) = (x.std::min(i$))); // Drowning in parentheses.
  for std::views::iota(0, 5) do (i) test(count_n(i), :(x) = std::min(x, i$)); // Mathematically correct.

I personally do not like further overloading parentheses (#542 (comment)).

I'm afraid I have mixed two issues in one.
The suggestion to improve what's rejected.
And the presentation of diagnostics in general.

[JohelEGP]

Following the comment a bit above:

cppfront/source/to_cpp1.h

Lines 2411 to 2412 in 68b716a

	// If we're doing a forward return of a single-token name
	// (the entire expression is a postfix-expression)

I parenthesized the expression, which made it it OK.

Oh, but a parenthesized expression is a postfix-expression.

//G primary-expression:
//G     [...]
//G     '(' expression-list ','? ')'
//G     [...]
//G
//G postfix-expression:
//G     primary-expression
//G     [...]

So maybe my workaround works unintendedly.

[MaxSagebaum]

If you do not use UFCS, it starts to work. https://cpp2.godbolt.org/z/vePsbM7bj

It seems that for a regular function call the analysis is not as elaborate. In order to get a better understanding, I would suggest an improved reproducer. E.g. a pure cpp2 case, an object with different functions with different returns, and multiple lines that demonstrate what works and what not works.