This is an archive of the discontinued LLVM Phabricator instance.

Implement P1401R5
ClosedPublic

Authored by cor3ntin on Jun 29 2021, 8:33 AM.

Details

Summary

Support Narrowing conversions to bool in if constexpr condition
under C++23 language mode.

Only if constexpr is implemented as the behavior of static_assert
is already conforming.

Diff Detail

Event Timeline

cor3ntin requested review of this revision.Jun 29 2021, 8:33 AM
cor3ntin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 8:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
3940

Minor nit: I think it would be a tiny bit clearer if you factored out this condition, since you repeat it:

bool IsConstexprNonValueDependent = IsConstexpr  && !CondExpr->isValueDependent();
if (!LangOpts.CPlusPlus2b && IsConstexprNonValueDependent) {
....
if (!IsConstexprNonValueDependent)
3945

The value returned from PerformContextuallyConvertToBool can have a possibly failed state (We follow this convention for FooResult named types).
But here there is a get on it on a path where there could be an error.
Can you handle this, and also add test case to cover it?

cor3ntin updated this revision to Diff 355462.Jun 30 2021, 12:31 AM

Fix and add tests for the case where the condition is not convertible to bool
Simplify code.
Add reference to c++23 wording

cor3ntin updated this revision to Diff 355477.Jun 30 2021, 1:49 AM

Fix formatting

Thanks!
Just some nits and some minor points on the tests.

clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

Looks a bit easier to parse the english there.

clang/lib/Sema/SemaExprCXX.cpp
3932–3933

This FIXME got pushed out of the thing it used to refer to.

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
1–3

I think attaching the expected diagnostics to the lines that generate them looks clearer and is easier to maintain.

The problem is that it seems you are having to work around that some of these errors are conditional on the std version, and it would indeed be quite a bite noisier with the preprocessor there, but there is a simple solution: with the suggested edit above, you can do things like:

`
// cxx17-error {{value of type 'ccce::S' is not implicitly convertible to 'bool'}}
// cxx2b-error {{value of type 'ccce::S' is not contextually convertible to 'bool'}}
`

But you can still do expected-note {{cannot be used in a constant expression}} in order to expect diagnostics for all versions.

41

This is not consistently indented.

70–71

Hmm what is this testing exactly? That constexprS is not getting confused with s?

cor3ntin updated this revision to Diff 355505.Jun 30 2021, 4:41 AM
cor3ntin marked an inline comment as done.

Improve and fix test,
Move the fixme in a more sensible place

clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

I would rather not change that, to remain consistent with existing diagnostics involving constexpr if
But I agree it might be good to change them all

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
1–3

Thanks, much better, I did not know it was an option

41

Unfortunately, it was put there by clang-format, should I move it manually?

70–71

Indeed, will fix

mizvekov requested changes to this revision.Jun 30 2021, 11:54 AM

So I read the paper, downloaded this patch, played around with it a little bit, tried some different tests, like expressions with dependent types,
classes with regular/explicit user-defined conversions, function names and some other examples that are mentioned in the paper.
It works fine on these.

However, I confirm that the failures in CXX/except/except.spec/p1.cpp detected by the buildbots are real.
It fails for me with this DR, works on parent revision.

clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

I see, yeah agreed.

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
41

In general you should respect the formatting tips you get from the non-test code, as these will fail the pre-merge checks.
But in the test code, our buildbot produces the clang-format patch but this is not really enforced when merging.

I think what you are doing here, keeping the existing style, is reasonable so you should disregard this particular tip from the format patch.
The other option would be to format everything in a pre-work NFC commit. This should be fine for pure semantic tests like these, but you have to be careful with parser tests.

This revision now requires changes to proceed.Jun 30 2021, 11:54 AM

So I read the paper, downloaded this patch, played around with it a little bit, tried some different tests, like expressions with dependent types,
classes with regular/explicit user-defined conversions, function names and some other examples that are mentioned in the paper.
It works fine on these.

However, I confirm that the failures in CXX/except/except.spec/p1.cpp detected by the buildbots are real.
It fails for me with this DR, works on parent revision.

Yep, I need to look at that. I've ran the entire test suite locally without issue initially but maybe I broke something!

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
41

This is unfortunate, but I'll fix it, thanks!

Its unfortunate the buildbot does not have debug symbols.

If you have not managed to reproduce it locally, here is the backtrace I got:

FAIL: Clang :: CXX/except/except.spec/p1.cpp (1 of 1)
******************** TEST 'Clang :: CXX/except/except.spec/p1.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\users\mizve\source\llvm\build\dbg\bin\clang.exe -cc1 -internal-isystem c:\users\mizve\source\llvm\build\dbg\lib\clang\13.0.0\include -nostdsysteminc -std=c++11 -fexceptions -fcxx-exceptions -fsyntax-only -verify C:\Users\mizve\source\llvm\clang\test\CXX\except\except.spec\p1.cpp
--
Exit Code: 2147483651

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\users\mizve\source\llvm\build\dbg\bin\clang.exe" "-cc1" "-internal-isystem" "c:\users\mizve\source\llvm\build\dbg\lib\clang\13.0.0\include" "-nostdsysteminc" "-std=c++11" "-fexceptions" "-fcxx-exceptions" "-fsyntax-only" "-verify" "C:\Users\mizve\source\llvm\clang\test\CXX\except\except.spec\p1.cpp"
# command stderr:
Assertion failed: (NoexceptExpr->isTypeDependent() || NoexceptExpr->getType()->getCanonicalTypeUnqualified() == Context.BoolTy) && "Parser should have made sure that the expression is boolean", file C:\Users\mizve\source\llvm\clang\lib\Sema\SemaDeclCXX.cpp, line 17947
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: c:\\users\\mizve\\source\\llvm\\build\\dbg\\bin\\clang.exe -cc1 -internal-isystem c:\\users\\mizve\\source\\llvm\\build\\dbg\\lib\\clang\\13.0.0\\include -nostdsysteminc -std=c++11 -fexceptions -fcxx-exceptions -fsyntax-only -verify C:\\Users\\mizve\\source\\llvm\\clang\\test\\CXX\\except\\except.spec\\p1.cpp
1.      <eof> parser at end of file
2.      C:\Users\mizve\source\llvm\clang\test\CXX\except\except.spec\p1.cpp:75:1: parsing namespace 'PR11084'
3.      C:\Users\mizve\source\llvm\clang\test\CXX\except\except.spec\p1.cpp:76:19: parsing struct/union/class body 'PR11084::A'
 #0 0x00007ff72b447a6c HandleAbort C:\Users\mizve\source\llvm\llvm\lib\Support\Windows\Signals.inc:408:0
 #1 0x00007ffff838bc31 (C:\Windows\SYSTEM32\ucrtbased.dll+0x6bc31)
 #2 0x00007ffff838d889 (C:\Windows\SYSTEM32\ucrtbased.dll+0x6d889)
 #3 0x00007ffff83930bf (C:\Windows\SYSTEM32\ucrtbased.dll+0x730bf)
 #4 0x00007ffff8391091 (C:\Windows\SYSTEM32\ucrtbased.dll+0x71091)
 #5 0x00007ffff8393a1f (C:\Windows\SYSTEM32\ucrtbased.dll+0x73a1f)
 #6 0x00007ff72fe2bf2a clang::Sema::checkExceptionSpecification(bool, enum clang::ExceptionSpecificationType, class llvm::ArrayRef<class clang::OpaquePtr<class clang::QualType>>, class llvm::ArrayRef<class clang::SourceRange>, class clang::Expr *, class llvm::SmallVectorImpl<class clang::QualType> &, struct clang::FunctionProtoType::ExceptionSpecInfo &) C:\Users\mizve\source\llvm\clang\lib\Sem #7 0x00007ff72fe2c0ee clang::Sema::actOnDelayedExceptionSpecification(class clang::Decl *, enum clang::ExceptionSpecificationType, class clang::SourceRange, class llvm::ArrayRef<class clang::OpaquePtr<class clang::QualType>>, class llvm::ArrayRef<class clang::SourceRange>, class clang::Expr *) C:\Users\mizve\source\llvm\clang\lib\Sema\SemaDeclCXX.cpp:17983:0
 #8 0x00007ff72fb16bd9 clang::Parser::ParseLexedMethodDeclaration(struct clang::Parser::LateParsedMethodDeclaration &) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseCXXInlineMethods.cpp:489:0
ng\lib\Parse\ParseCXXInlineMethods.cpp:257:0
#10 0x00007ff72fb15eb6 clang::Parser::ParseLexedMethodDeclarations(struct clang::Parser::ParsingClass &) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseCXXInlineMethods.cpp:325:0
#11 0x00007ff72fb96995 clang::Parser::ParseCXXMemberSpecification(class clang::SourceLocation, class clang::SourceLocation, struct clang::ParsedAttributesWithRange &, unsigned int, class clang::Decl *) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDeclCXX.cpp:35#12 0x00007ff72fb95153 clang::Parser::ParseClassSpecifier(enum clang::tok::TokenKind, class clang::SourceLocation, class clang::DeclSpec &, struct clang::Parser::ParsedTemplateInfo const &, enum clang::AccessSpecifier, bool, enum clang::Parser::DeclSpecContext, struct clang::ParsedAttributesWithRange &) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDeclCXX.cpp:2030:0
#13 0x00007ff72fad24d2 clang::Parser::ParseDeclarationSpecifiers(class clang::DeclSpec &, struct clang::Parser::ParsedTemplateInfo const &, enum clang::AccessSpecifier, enum clang::Parser::DeclSpecContext, class clang::Parser::LateParsedAttrList *) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDecl.cpp:3989:0
#14 0x00007ff72fb07c7f clang::Parser::ParseSingleDeclarationAfterTemplate(enum clang::DeclaratorContext, struct clang::Parser::ParsedTemplateInfo const &, class clang::ParsingDeclRAIIObject &, class clang::SourceLocation &, class clang::ParsedAttributes &, enum clang::AccessSpecifier) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseTemplate.cpp:225:0
#15 0x00007ff72fb078fa clang::Parser::ParseTemplateDeclarationOrSpecialization(enum clang::DeclaratorContext, class clang::SourceLocation &, class clang::ParsedAttributes &, enum clang::AccessSpecifier) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseTemplate.cpp:169:0
#16 0x00007ff72fb07297 clang::Parser::ParseDeclarationStartingWithTemplate(enum clang::DeclaratorContext, class clang::SourceLocation &, class clang::ParsedAttributes &, enum clang::AccessSpecifier) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseTemplate.cpp:46:0
#17 0x00007ff72faca50a clang::Parser::ParseDeclaration(enum clang::DeclaratorContext, class clang::SourceLocation &, struct clang::ParsedAttributesWithRange &, class clang::SourceLocation *) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDecl.cpp:1718:0
#18 0x00007ff72fa84fc9 clang::Parser::ParseExternalDeclaration(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec *) C:\Users\mizve\source\llvm\clang\lib\Parse\Parser.cpp:907:0
#19 0x00007ff72fb8eff8 clang::Parser::ParseInnerNamespace(class llvm::SmallVector<struct clang::Parser::InnerNamespaceInfo, 4> const &, unsigned int, class clang::SourceLocation &, class clang::ParsedAttributes &, class clang::BalancedDelimiterTracker &) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDeclCXX.cpp:247:0
#20 0x00007ff72fb8ede0 clang::Parser::ParseNamespace(enum clang::DeclaratorContext, class clang::SourceLocation &, class clang::SourceLocation) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDeclCXX.cpp:227:0
#21 0x00007ff72faca6ea clang::Parser::ParseDeclaration(enum clang::DeclaratorContext, class clang::SourceLocation &, struct clang::ParsedAttributesWithRange &, class clang::SourceLocation *) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseDecl.cpp:1731:0
#22 0x00007ff72fa84fc9 clang::Parser::ParseExternalDeclaration(struct clang::ParsedAttributesWithRange &, class clang::ParsingDeclSpec *) C:\Users\mizve\source\llvm\clang\lib\Parse\Parser.cpp:907:0
#23 0x00007ff72fa7fd34 clang::Parser::ParseTopLevelDecl(class clang::OpaquePtr<class clang::DeclGroupRef> &, bool) C:\Users\mizve\source\llvm\clang\lib\Parse\Parser.cpp:719:0
#24 0x00007ff72fa7c078 clang::ParseAST(class clang::Sema &, bool, bool) C:\Users\mizve\source\llvm\clang\lib\Parse\ParseAST.cpp:158:0
#25 0x00007ff72c913df7 clang::ASTFrontendAction::ExecuteAction(void) C:\Users\mizve\source\llvm\clang\lib\Frontend\FrontendAction.cpp:1060:0
#26 0x00007ff72c9137ae clang::FrontendAction::Execute(void) C:\Users\mizve\source\llvm\clang\lib\Frontend\FrontendAction.cpp:955:0
#27 0x00007ff72c8944a6 clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) C:\Users\mizve\source\llvm\clang\lib\Frontend\CompilerInstance.cpp:974:0
#28 0x00007ff72cafc037 clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) C:\Users\mizve\source\llvm\clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:278:0
#29 0x00007ff726c93e04 cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) C:\Users\mizve\source\llvm\clang\tools\driver\cc1_main.cpp:246:0
#30 0x00007ff726c7fa40 ExecuteCC1Tool C:\Users\mizve\source\llvm\clang\tools\driver\driver.cpp:338:0
#31 0x00007ff726c8032b main C:\Users\mizve\source\llvm\clang\tools\driver\driver.cpp:415:0
#32 0x00007ff7325e7a99 invoke_main d:\agent\_work\36\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:79:0
#33 0x00007ff7325e793e __scrt_common_main_seh d:\agent\_work\36\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#34 0x00007ff7325e77fe __scrt_common_main d:\agent\_work\36\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:331:0
#35 0x00007ff7325e7b2e mainCRTStartup d:\agent\_work\36\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:17:0
#36 0x00007ff87d467034 (C:\Windows\System32\KERNEL32.DLL+0x17034)
#37 0x00007ff87d5a2651 (C:\Windows\SYSTEM32\ntdll.dll+0x52651)

error: command failed with exit status: 2147483651

So I read the paper, downloaded this patch, played around with it a little bit, tried some different tests, like expressions with dependent types,
classes with regular/explicit user-defined conversions, function names and some other examples that are mentioned in the paper.
It works fine on these.

However, I confirm that the failures in CXX/except/except.spec/p1.cpp detected by the buildbots are real.
It fails for me with this DR, works on parent revision.

Yep, I need to look at that. I've ran the entire test suite locally without issue initially but maybe I broke something!

Its unfortunate the buildbot does not have debug symbols.

If you have not managed to reproduce it locally, here is the backtrace I got:

Thanks.
I think my local build had NDEBUG defined, so an assert was not firing
My current guess is that early exiting on value-dependent expressions bypasses check expected down the line, so I'll confirm that, fix it and update the PR!
Thanks for your help!

cor3ntin updated this revision to Diff 355666.Jun 30 2021, 12:44 PM

Fix Formatting in tests.

Do not return early for value dependent expressions
as PerformContextuallyConvertToBool performs checks that
are expected to occur down the line

LGTM!

Since you submitted this quite recently, I would wait some more time before landing, for any other people who might want to review this.
Maybe wait for it to be a week old, ping anyone else you might want to get a review from, and wait a couple of days for them to respond.

clang/lib/Sema/SemaExprCXX.cpp
3940

By the way you fixed this by reverting back to the original form pre-refactoring, so my tip above applies again.

mizvekov accepted this revision.Jun 30 2021, 2:41 PM
This revision is now accepted and ready to land.Jun 30 2021, 2:41 PM
rsmith added a subscriber: rsmith.Jun 30 2021, 2:53 PM

P1401R5 points out that we get noexcept(bool) wrong in the opposite direction, permitting conversions to bool that we are supposed to reject. While that rule didn't change as part of P1401R5, it'd be nice to handle it as part of dealing with that paper.

clang/lib/Sema/SemaExprCXX.cpp
3934

I think we should apply this retroactively, even though it wasn't moved as a DR. All other implementations already behave this way across all language modes, and it's just not reasonable to reject conversions in the condition of an if constexpr that are accepted in the condition of an if -- this seems like an obvious language defect even if the C++ committee haven't officially voted it to be one yet.

Separately I've asked on the committee reflectors if we can officially treat this as a DR, given the implementation consensus.

3940

Please use E.isInvalid() instead of !E.isUsable() unless you anticipate and intend to handle the valid-but-null case (which is unusable but not invalid).

Would we benefit from any more test coverage for the static_assert side of this paper, or is our (formerly wrong and now correct) behavior already well-covered by existing tests? (Similarly for explicit(bool), do we already have good test coverage for the distinction between "contextually converted constant expression of type bool" and the various other possible rules?)

P1401R5 points out that we get noexcept(bool) wrong in the opposite direction, permitting conversions to bool that we are supposed to reject. While that rule didn't change as part of P1401R5, it'd be nice to handle it as part of dealing with that paper.

To be clear: doing that as a separate change is completely fine, but we should make sure we don't forget about it!

rsmith added inline comments.Jun 30 2021, 2:59 PM
clang/www/cxx_status.html
1299

This should be class unreleased (yellow) for now so that people can easily tell what's in the most recent Clang release versus what's implemented but not released; we convert all the class="unreleased" to class="full" when we cut a release.

cor3ntin updated this revision to Diff 355697.Jun 30 2021, 3:03 PM

Mark the feature as unreleased

cor3ntin updated this revision to Diff 355698.Jun 30 2021, 3:05 PM

Replace !isUsable() by isInvalid()

cor3ntin marked 2 inline comments as done.Jun 30 2021, 3:07 PM
cor3ntin added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
3934

I'll do that - it might take me a few days to get to it though

cor3ntin added inline comments.Jun 30 2021, 3:24 PM
clang/www/cxx_status.html
1299

Would you prefer I mark it partial for the explicit bool case?

cor3ntin updated this revision to Diff 355706.Jun 30 2021, 3:45 PM
  • Apply the change to all C++ version
  • Add more tests for static_assert
cor3ntin updated this revision to Diff 355720.Jun 30 2021, 4:29 PM

clang-format

rsmith added inline comments.Jun 30 2021, 5:22 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

Would it be reasonable to drop the "convertible to bool" part here? We know the problem is that the (converted) expression is not a constant expression, not that the expression can't be converted to bool, because we handle the conversion to bool separately before we get to this diagnostic; I think the diagnostic would be clearer if it didn't mention the conversion.

clang/www/cxx_status.html
1299

If you're not planning on working on explicit(bool) yourself, then I think marking it as "partial" might be useful as a reminder that we should go back and look at that paper again. Otherwise, I have no strong preference.

cor3ntin updated this revision to Diff 355861.Jul 1 2021, 6:47 AM

Mark the feature completion as partial
as this PR does not fix explicit(bool)

mizvekov added inline comments.Jul 1 2021, 1:21 PM
clang/test/SemaCXX/static-assert.cpp
202

Missing newline here. You might need to configure your editor to always add this.

cor3ntin updated this revision to Diff 356022.Jul 1 2021, 2:54 PM

Missing EOL

aaron.ballman added inline comments.Jul 9 2021, 6:38 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

+1 -- I think the "convertible to bool" may trip some users up on how to correct the issue.

clang/lib/Sema/SemaExprCXX.cpp
3925
3932–3933

+1, I think this comment should move down to the APSInt declaration so it's clear which value we want returned to the caller someday.

3935
aaron.ballman requested changes to this revision.Jul 9 2021, 6:38 AM

Making it clear that there are still some minor changes left to be made.

This revision now requires changes to proceed.Jul 9 2021, 6:38 AM
cor3ntin updated this revision to Diff 357501.Jul 9 2021, 7:17 AM
cor3ntin marked 3 inline comments as done.

Improve diagnostic message, fix comments, use isInvalid instead of isUsable

cor3ntin marked 5 inline comments as done.Jul 9 2021, 7:19 AM
cor3ntin added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
1491

Sorry, I missed that feedback earlier, fixed now

This LGTM aside from a small nit, but you should wait a bit to land in case other reviewers have comments.

clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
42–43

This looks like it still needs some indentation to match the nearby local style. Also, do you need to declare the variable s?

aaron.ballman accepted this revision.Jul 9 2021, 8:07 AM

Oops, I chanted the magic incantation but forgot to press the button. LGTM again. ;-)

This revision is now accepted and ready to land.Jul 9 2021, 8:07 AM
cor3ntin updated this revision to Diff 357536.Jul 9 2021, 9:07 AM

Fix test formatting

cor3ntin marked an inline comment as done.Jul 9 2021, 9:08 AM
aaron.ballman closed this revision.Jul 12 2021, 5:07 AM

Thanks for the patch! I've committed on your behalf in 8747234032c9c6270a6198ab3cca14ce2bd18721.