[PATCH] D74130: [clang] fix consteval call in default arguments

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 382702.
Tyker added a comment.

In D74130#3085313 <https://reviews.llvm.org/D74130#3085313>, @aaron.ballman wrote:

> FWIW, I am not seeing double errors on that code. Here's the output I get with this patch applied locally:
>
>   F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
>   consteval int f1() { return 0; }
>   consteval auto g() { return f1; }
>   
>   constexpr auto e = g();
>   constexpr auto e1 = f1;
>   
>   F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only -std=c++2b "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: error: call to consteval function 'g' is not a
>         constant expression
>   constexpr auto e = g();
>                      ^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:20: note: pointer to a consteval declaration is not a
>         constant expression
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
>   consteval int f1() { return 0; }
>                 ^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: error: constexpr variable 'e' must be initialized
>         by a constant expression
>   constexpr auto e = g();
>                  ^   ~~~
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:16: note: pointer to a consteval declaration is not a
>         constant expression
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
>   consteval int f1() { return 0; }
>                 ^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:21: error: cannot take address of consteval function
>         'f1' outside of an immediate invocation
>   constexpr auto e1 = f1;
>                       ^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
>   consteval int f1() { return 0; }
>                 ^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: error: constexpr variable 'e1' must be initialized
>         by a constant expression
>   constexpr auto e1 = f1;
>                  ^    ~~
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:5:16: note: pointer to a consteval declaration is not a
>         constant expression
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:15: note: declared here
>   consteval int f1() { return 0; }
>                 ^
>   4 errors generated.
>
> These look like valid errors to me, so am I misunderstanding something? Is the concern that we're emitting `error: constexpr variable '<whatever>' must be initialized by a constant expression` after we already issued a diagnostic on the same line?

Yes, i think that
`error: constexpr variable '<whatever>' must be initialized by a constant expression` + `call to consteval function '<whatever>' is not a constant expression` or 
`error: constexpr variable '<whatever>' must be initialized by a constant expression` + `cannot take address of consteval function <whatever>' outside of an immediate invocation`
 is emitting 2 errors for the same root cause. the correct solution in my mind would be to swap too constant context when processing the initializer of a constexpr (or constinit) variable. this would disable consteval checking.

> From my testing, there's more work to be done, but I think it can be done in a follow-up. Consider:
>
>   template <typename Ty>
>   struct S {
>     consteval S() = default;
>     Ty Val;
>   };
>   
>   struct foo {
>     foo();
>   };
>   
>   S<int> s1; // Correctly rejected
>   S<foo> s2; // Incorrectly accepted
>
> With this patch applied, `s1` is correctly diagnosed, but `s2` is not. The reason it's not is because `Sema::CheckForImmediateInvocation()` tests `!Decl->isConsteval()` to early return, and for the instantiation of `S<foo>`, the constructor is marked as "unspecified" rather than `consteval`. The reason for that is because we set `DefaultedDefaultConstructorIsConstexpr` to false at https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclCXX.cpp#L1309 when instantiating the class. I think we may have some confusion where we mean "*IsConstexpr" to mean "is a constexpr function" or "is usable as a constexpr function", because my reading of https://eel.is/c++draft/dcl.constexpr#7 is that the instantiated template constructor IS a constexpr function but it is not usable in a constant expression. However, that might be orthogonal to the fixes here and should be done separately.

having a function that is explicitly specified consteval not returning true for isConsteval seems wrong.

IsConstexpr means the function has a either a consteval or constexpr(maybe implicit) specifier.
so i guess that "is a constexpr function" would be isConstexprSpecified
and "is usable as a constexpr function" would be isConstexpr

BTW I found an other case were we accept invalid code:

  struct A { consteval A(); } a;


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74130/new/

https://reviews.llvm.org/D74130

Files:
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74130.382702.patch
Type: text/x-patch
Size: 14626 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211027/96f72a7a/attachment-0001.bin>