[clang] Use decltype((E)) for compound requirement type constraint

[mizvekov]

Summary

See PR45088.

Compound requirement type constraints were using decltype(E) instead of
decltype((E)), as per [expr.prim.req]p1.3.3.

Since neither instantiation nor type dependence should matter for
the constraints, this uses an approach where a decltype type is not built,
and just the canonical type of the expression after template instantiation
is used on the requirement.

Signed-off-by: Matheus Izvekov <[e-mail]>

[• Quuxplusone]

clang/lib/Sema/SemaType.cpp

8837

I think a better name for this might be getDecltypeForNonIdentifierExpr and/or getDecltypeForParenthesizedExpr.
Depending on the mental model you want people to have, ImplicitParens might be better named AsNonIdentifier.
The thing we're enabling/disabling here is "whether to treat an id-expression differently from any other kind of expression."

Also, FYI and FWIW, I have a proposal in the pipeline for C++23 that will make "the decltype of" the id-expression in return x into an rvalue (for the purposes of decltype(auto) return types). So it might be worth thinking about what that codepath would look like, since it's another example of "treating id-expressions differently from other kinds of expressions (but in a different way from how we already do)."
http://quuxplusone.github.io/draft/d2266-implicit-move-rvalue-ref.html
If I ran the zoo, I'd have a primitive function for "get the decltype of this expression, ignoring all crazy id-expression rules," and then I'd layer various helper functions on top of it that did, like, "if (is id-expression) craziness; else call the primitive function."

Finally, your comment on line 8837 ought to indicate what rules it's not accounting for. "The rules" is pretty vague. ;)

clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp

89–90

I think this is too large of a change. How about just keeping the old test code but changing it to

template<typename T>
concept Large = sizeof(std::decay_t<T>) >= 4; // expected-note{{because... etc}}

[rsmith]

clang/lib/AST/ASTContext.cpp

5446 ↗

(On Diff #328918)

If we don't track the extra parens here too, we can end up giving the same canonical type to dependent decltype types with and without implicit parens, even though they can instantiate to different types. (But I think the simplest way to handle this would be to include the parens in the expression; see other comment.)

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

Instead of adding complexity to the type system to deal with this special case, can you directly create a ParenExpr here?

[mizvekov]

Addressing Arthur's concerns:

• Changed getBaseDecltypeForExpr to getDecltypeForParenthesizedExpr.
• Changed ImplicitParens to Parenthesized.
• Clarified comment.

[mizvekov]

clang/lib/AST/ASTContext.cpp

5446 ↗

(On Diff #328918)

Good catch, but yeah I'll see about the other solution.

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

Hmm I thought about that. Since I am a bit unfamiliar with the code there, I was not sure it was going to end up being more or less complicated than the proposed solution. But I'll give it a shot if that looks simpler to you.

clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp

89–90

Yeah I thought about that, but on the other hand changing the test to make it a bit more strict did not look like a problem to me.
Hmm, we cover this case in the test below anyway, might as well reduce the amount of changes I guess.

[mizvekov]

• Uses yet another simpler approach: Since dependence should not matter (we were using the type of the expression after template instantiation anyway), we don't even need to build a decltype type here, and just get the canonical type from the (expr) instead. Kudos to rsmith for this suggestion in private.
• Adds some more tests, testing more kinds of values.

[mizvekov]

lint

[mizvekov]

broken bot rebuild repush

[mizvekov]

I don't know what is going on with the clang-format linux failure here.
The patch it suggests looks awful...

[mizvekov]

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

So yeah, now we just get the type of the expression directly, by using the now exposed underlying code from decltype.
This is OK since the expressions in the requires clause were never dependent in the first place.
Even though just creating the parensexpr would likely lead to a smaller change as in number of lines of code modified, I think not using it makes the fact that dependency is not an issue here more clear in the code than just reusing the full decltype machinery.

[rsmith]

Thanks, I like this approach.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2761

I don't think we need to talk about the mechanics of how we formed the type here. Also, adding a manual 'aka' like this will result in a double-aka (decltype((foo))' (aka 'bar' (aka 'baz'))) in some cases.

Aside [not something to address in this patch]: this (and other diagnostics nearby) also violate our diagnostics best practices by redundantly including in the diagnostic the source expression that the caret will be pointing to, but perhaps that's justifiable in this case because we expect these expressions to be short and they're so central to what's being diagnosed.

clang/lib/Sema/SemaConcept.cpp

448

Please don't canonicalize the type here. If we've retained type sugar for the type of e, it'll be more helpful to the user if we present that type sugar.

[mizvekov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2761

Though the original code just built a decltype and printed it here. This format string, as far as I remember, matches exactly what the original code printed, so that a sweeping change to update all the tests would not have to be made.
I vaguely remember following the code of the type printer here and seeing that it would stop from printing an aka chain by just canonicalizing the type on the first aka.

So yeah, the original code ironically violated this best practice by just printing a type, but which in this case contains the expression.

But I agree we do not necessarily need to explain the mechanics of this type here, though I think the standard does explain how the type is obtained here in terms of decltype(()).

But since you mentioned this, I think the best approach here might be to just print the non canonical type, and not print the expression.
Will have to update all the tests but no big deal. Agreed?

[mizvekov]

Changes per rsmith review:

Just print the non-canonical type in the diagnostics.
Instead of printing the expression, make the caret point to it, instead of
pointing to the constraint.