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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D74130#3079630 <https://reviews.llvm.org/D74130#3079630>, @Tyker wrote:

> In D74130#3073271 <https://reviews.llvm.org/D74130#3073271>, @aaron.ballman wrote:
>
>> @Tyker -- are you planning to pick this review back up again sometime in the near future? If not, do you care if the review gets commandeered?
>
> here is an update that i think is close to committable. if it is not i am fine if it gets commandeered.
>
> Changes
>
> - rebased
> - fix the default parameter situation for functions and lambdas.
> - fix the top level variables not being properly checked.
>
> the double errors are not fixed on code like:
>
>   consteval int f1() { return 0; }
>   consteval auto g() { return f1; }
>   
>   constexpr auto e = g();
>   constexpr auto e1 = f1;

Thank you for the update and letting me know you're fine if this is commandeered.

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?

>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.



================
Comment at: clang/lib/Parse/ParseDecl.cpp:2393
 
       InitScope.pop();
 
----------------
Is there a reason we're not moving this one to below `AddInititializerToDecl()` as we did elsewhere?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14510
+    auto LSI = getCurLambda();
+    /// If the functcion is not consteval the paramters need to be checked for
+    /// immediate invocations.
----------------



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

https://reviews.llvm.org/D74130