Wignored-qualifiers emits a false positive warning for user defined types returned by const auto

[Kurkin]

Bugzilla Link	43709
Version	trunk
OS	All
CC	@aaronpuchert,@Quuxplusone,@DougGregor,@yeputons,@zygoloid

Extended Description

Consider the example:

struct S{};

const auto bar() { return S{}; } // false positive warning
const S bar1() { return S{}; }

https://godbolt.org/z/XE3F69

[yeputons]

Affects me as well. I first encountered it in Emscripten: emscripten-core/emscripten#11123

Usecase: developing a DSL, I want to return an object which cannot be moved from, but can be copied from. In the meantime, the base class should support both movies and copies.

[Kurkin]

https://reviews.llvm.org/D80082 will probably fix this issue.

[aaronpuchert]

Affects me as well. I first encountered it in Emscripten:
emscripten-core/emscripten#11123
Not sure this issue is related to the warning. If I add to Dmitry's code

void f() {
S a = bar(), b = bar1();
}

and compile with -std=c++17, I get the following AST (minus clutter):

VarDecl a 'S' cinit
-ImplicitCastExpr 'S' <NoOp> -CallExpr 'const S':'const S'
-ImplicitCastExpr 'const S (*)()' <FunctionToPointerDecay> -DeclRefExpr 'const S ()' lvalue Function 'bar' 'const S ()'
VarDecl b 'S' cinit
-ImplicitCastExpr 'S' <NoOp> -CallExpr 'const S'
-ImplicitCastExpr 'const S (*)()' <FunctionToPointerDecay> -DeclRefExpr 'const S ()' lvalue Function 'bar1' 'const S ()'

So there is copy elision and the const is indeed without effect. If you think that's a bug you should probably file separately. The warning is emitted on function declaration, whereas the decision about copy elision is obviously made on calls to the function.

Usecase: developing a DSL, I want to return an object which cannot be moved
from, but can be copied from. In the meantime, the base class should support
both movies and copies.
Copy elision basically means that the caller owns the return object, so why should the caller not be able to move from that?

[yeputons]

Copy elision basically means that the caller owns the return object, so why should the caller not be able to move from that?

I have a base class with a non-movable derived class. The function returns a non-movable derived object via copy elision, which is expected to be materialized first and destroyed in the end of a full expression later. It, however, really does not want to be moved (it marks an external function which calls a C++ lambda by address, some making it movable will add another level of indirection).

Simplified code for the usecase:

struct Base {}; // Can be safely copied and moved.
struct Derived : Base {
Derived(Derived&&) = delete; // Cannot be moved or copied.
~Derived(); // Some non-trivial destructor which does not want Base to be moved-from.
};
Derived create_derived();
void expected_use_1(Derived &d);
void expected_use_2(Base &d);
void bad_use(Base b);

expected_use_1(create_derived()); // Materializes temporary which lives until semicolon, no moves of Derived.
expected_use_2(create_derived()); // Materializes temporary which lives until semicolon, no moves of Derived.
bad_use(create_derived()); // Materializes temporary which lives until semicolon, but it's moved from by Base(Base&&) into a parameter. So, ~Derived() fails.

As far as I understand, it's valid C++ and does not rely on lack of copy elision: all materializations are required here.

Another solution for the same problem would probably be declaring operator const Base&() in Derived instead of Base.

[yeputons]

Another solution for the same problem would probably be declaring operator const Base&() in Derived instead of Base.

After giving it more thought, it won't do, because Base and expected_use_* are actually templates, and I want template deduction to work.

Anyhow, that seems off-topic, sorry.

[Quuxplusone]

I don't know/remember why I'm cc'ed on this bug report, but FWIW:

struct S{};
const auto bar() { return S{}; } // warns
const S bar1() { return S{}; } // doesn't warn

I consider this a false negative. Clang warns correctly that "'const' type qualifier on return type has no effect [-Wignored-qualifiers]". In fact, for user-defined types it has WORSE than no effect; it's a pessimization!

See https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/#const-on-a-return-type-is-not-a-meaningful-contract-and-is-sometimes-a-pessimization

In short, this is a false negative, not a false positive: Clang should equally be warning on const S foo() { return S(); }. And arguably the warning's wording should be updated to indicate that return-by-const-value is not always technically "no effect"; sometimes it HAS a HARMFUL effect.

[yeputons]

I think that "returning const value makes little sense" should be a completely separate warning.

const for user-defined does have an effect, it's pessimization most of the time (as Arthur has described), but sometimes the effect is actually desired. I have described one such narrow usecase above.

However, a separate warning like -Wreturn-const-value would be useful. Narrow usecases like above may then explicitly silence it with appropriate comment around.

[aaronpuchert]

Derived create_derived();
I presume you want to make this return "const Derived", right?

void expected_use_1(Derived &d);
void expected_use_2(Base &d);
These must take a const reference, because non-const references don't bind to temporaries, so the expected_use_* invocations won't work.

bad_use(create_derived()); // Materializes temporary which lives until
semicolon, but it's moved from by Base(Base&&) into a parameter. So,
~Derived() fails.
Hmm, that's difficult. You have a class that you can't move from, but you can move from a public base, and apparently you are otherwise fine with slicing. To me that's contradictory, you want to disallow an operation but its constituent parts (casting to base class, move constructing base class) are allowed. After adding the const above, the bad_use call indeed invokes the copy constructor, but you can still write

Derived d = create_derived();
bad_use(std::move(d));

which invokes the move constructor. So I don't think the const really helps here. You don't even need to circumvent it, this is completely idiomatic code.

I don't know/remember why I'm cc'ed on this bug report, but FWIW:
I just added you because I remembered that you wrote about this, but feel free to remove yourself if you're not interested.

I consider this a false negative. Clang warns correctly that "'const' type
qualifier on return type has no effect [-Wignored-qualifiers]". In fact, for
user-defined types it has WORSE than no effect; it's a pessimization!
Agreed, I don't think there are any circumstances where it makes sense.

[the] wording should be updated to indicate that return-by-const-value
is not always technically "no effect"; sometimes it HAS a HARMFUL effect.
We could have different messages depending on the type.

I think that "returning const value makes little sense" should be a
completely separate warning.
Indeed the warning name -Wignored-qualifiers isn't entirely correct, but since for example initializing a variable ignores it, it is not effective, or "effectively ignored". So I think it's fine under that flag, but as Arthur said the message should be different for non-builtin types.

[yeputons]

Thank you Aaron, your corrections are all true.

Agreed, I don't think there are any circumstances where it makes sense.

I've described bigger picture in the original issue: emscripten-core/emscripten#11123 (comment) . It's kind of messy and highly specific, so maybe there is a better solution to that. Or, actually, maybe no reason to do anything with the warning except better wording.

Basically, the reason why I consider

Derived d = create_derived();
bad_use(std::move(d));

to be non-issue in my usecase, while still frowning upon

bad_use(create_derived());

is because in my usecase user is never expected to actually store anything of type Derived, so storing result of create_derived() immediately stands out. Or calling std::move. Or doing basically anything except passing it directly to bad_use()/good_use_1.

Derived and create_derived() are actually a hack around a lambda-like object which is only created "inline". Proper C++ syntax would look more verbose:

{
DerivedWithProperRaii d = create_derived_with_proper_raii();
bad_use(d); // No std::move, copy+expected slicing, no issues.
bad_use(std::move(d)); // std::move stands out in the code, because
// a user does not expect any moves of d.
// Compiles, but smells heavily at code review.
} // DerivedWithProperRaii is destroyed

However, that specific Derived is a lambda-like object which is very often created, so creating a variable for each operation is verbose. So I resorted to the fact that temporaries live until the end of a full expression.