This is an archive of the discontinued LLVM Phabricator instance.

[Clang] CWG 1394: Incomplete types as parameters of deleted functions
ClosedPublic

Authored by rZhBoYao on Apr 2 2022, 9:11 AM.

Details

Summary

According to CWG 1394, dcl.fct.def.general p2 and this StackOverflow answer, Clang should not diagnose incomplete types if function body is "= delete;".

For example:

struct Incomplete;
Incomplete f(Incomplete) = delete; // well-formed

Inspired by this tweet.

Also close PR52802.

Diff Detail

Event Timeline

rZhBoYao created this revision.Apr 2 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2022, 9:11 AM
rZhBoYao requested review of this revision.Apr 2 2022, 9:11 AM
rZhBoYao edited the summary of this revision. (Show Details)Apr 2 2022, 6:15 PM
ChuanqiXu added inline comments.Apr 5 2022, 7:16 PM
clang/lib/Sema/SemaDecl.cpp
14473–14474
14477

I think we could remove the use of FnDeleted by the use of FD->isDeleted(). Also we should constrain the behavior only if std >= 11.

ChuanqiXu added a reviewer: Restricted Project.Apr 5 2022, 7:17 PM
rZhBoYao marked 2 inline comments as done.Apr 5 2022, 8:14 PM
rZhBoYao added inline comments.
clang/lib/Parse/Parser.cpp
1336–1337

I'm thinking should we merge FnDeleted and Deleted into one variable or leave it as is? On the one hand, Deleted's scope is tight so very readable. On the other hand, it seems redundant.

1343

FunctionDecl::setDeletedAsWritten not called until this SetDeclDeleted call.

clang/lib/Sema/SemaDecl.cpp
14473–14474

I think the sentence is not yet complete there so I mimic the old one on the left.

14477

I tried FD->isDeleted() at first only to find out it always returns false. Looks like it's not handled until Parser.cpp:1342

Also, looks like deleted functions are implemented as an extension. A warning is issued at Parser.cpp:1338 if language mode < C++11

ChuanqiXu added inline comments.Apr 5 2022, 8:39 PM
clang/lib/Sema/SemaDecl.cpp
14477

I see. My suggestion might be to defer the diagnose message after we set FD->setDeletedAsWritten(). I understand it might be harder. But the current implementation looks a little bit not good... (redundant variables and passing information by argument)

rZhBoYao updated this revision to Diff 420764.Apr 6 2022, 3:28 AM
rZhBoYao marked 2 inline comments as done.
rZhBoYao added a reviewer: erichkeane.

I think an extra parameter is inevitable without complicating things too much.

erichkeane added inline comments.Apr 6 2022, 6:14 AM
clang/include/clang/Sema/Sema.h
2906

I tend to prefer not doing bool params, and instead some type of 'enum class', as that makes it more clear at the call site.

clang/lib/Parse/Parser.cpp
1306

I'm not sure about doing this 'look ahead' here, this feels dangerous to me. First, does this work with comments? Second, it seems we wouldn't normally look at 'deleted' if SkipBody.ShouldSkip (see below with the early exit)?

Next I'm not a fan of double-parsing these tokens with this lookahead. I wonder, if we should move hte logic from ~1334 and 1338 up here and calculate the 'deleted'/'defaulted' 'earlier', before we 'actOnStartOfFunctionDef`.

This would result in us being able to instead change the signature of ActOnStartOfFunctionDef to take some enum as to whether it is deleted/defaulted, AND create the function decl as deleted/defaulted 'in place' (or, at least, call SetDeclDeleted or SetDeclDefaulted immediately).

clang/lib/Sema/SemaDecl.cpp
14477

I disagree on doing this for C++11 mode. As we allow them as an extension, we want this to work in the extension mode as well.

I think an extra parameter is inevitable without complicating things too much.

My thought is that it might not be worth to fix the problem for the workaround. I feel like the problem is less countered. Let's try to fix it in a cleaner way.

rZhBoYao marked 3 inline comments as done.Apr 7 2022, 11:15 AM
rZhBoYao added inline comments.
clang/lib/Parse/Parser.cpp
1306

I'm not sure about doing this 'look ahead' here, this feels dangerous to me. First, does this work with comments?

Yes, it returns a normal token after phase 5, so comments are long gone.

Second, it seems we wouldn't normally look at 'deleted' if SkipBody.ShouldSkip (see below with the early exit)?

SkipBody.ShouldSkip is an output parameter of ActOnStartOfFunctionDef. We need to either look ahead or consume "delete" before entering ActOnStartOfFunctionDef anyway.

Next I'm not a fan of double-parsing these tokens with this lookahead.

It does look weird. Consume them I will. Updated diff coming.

AND create the function decl as deleted/defaulted 'in place' (or, at least, call SetDeclDeleted or SetDeclDefaulted immediately).

SetDecl{Deleted | Defaulted} needs KWLoc tho. I haven't thought of a way of doing that "in place" inside ActOnStartOfFunctionDef.

clang/lib/Sema/SemaDecl.cpp
14477

Thanks for the endorsement.

erichkeane added inline comments.Apr 7 2022, 11:21 AM
clang/lib/Parse/Parser.cpp
1306

My point is: do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?

Alternatively, we could add a function to Sema to 'ActOnFunctionDefinition(DefType)' and move this diagnostic THERE instead of ActOnStartofFunctionDef, and call it AFTER we have handled the '= delete/=default/etc'.

rZhBoYao marked 2 inline comments as done.Apr 7 2022, 11:41 AM
rZhBoYao added inline comments.
clang/lib/Parse/Parser.cpp
1306

do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?

But we need Decl *Res returned by ActOnStartOfFunctionDef.

I did try to factor out the diagnostic right after ActOnFunctionDefinition and 27 tests were failing.
Furthermore, we are not the only caller of ActOnFunctionDefinition.

rZhBoYao updated this revision to Diff 421305.EditedApr 7 2022, 11:44 AM

Handling of eagerly parsed deleted or defaulted function must happen AFTER D.complete(Res);.

A nicer looking diff: https://github.com/poyaoc97/llvm-project/commit/dc37a262582f6a2d8143c6f1f586dc657b4b5311

rZhBoYao marked an inline comment as done.Apr 7 2022, 11:46 AM
rZhBoYao added inline comments.
clang/lib/Parse/Parser.cpp
1306

I meant to say ActOnStartOfFunctionDef.

This definitely looks like it is 'nicer' than before, a few smaller/nit-ish comments.

Additionally, Phab made a REAL mess of the diff, if you could give a quick summary of what changes were actually made (vs things that were moved slightly and causing massive red/green diffs), it would be helpful.

clang/include/clang/Sema/Sema.h
2906
2910

Might suggest 'Other'?

Also, the EqDelete and EqDefault could probably just be Delete/Default and be equally as clear.

clang/lib/Parse/Parser.cpp
1327

Since the FnBodyKind is in Sema, I would suggest just adding a new function to replace all of this, "SetFunctionBodyKind(Res, BodyKind)", and make Sema do the 'switch' here.

clang/lib/Sema/SemaDecl.cpp
14473–14474

This is C++ specific, so could we be specific about that here?

rZhBoYao updated this revision to Diff 421328.EditedApr 7 2022, 1:31 PM
rZhBoYao marked 5 inline comments as done.

Removed member function test cases and addressed comments,
which includes:

  1. Sema::SetFunctionBodyKind
  2. Change enum names
  3. Be clear about delete being C++ specific.

A nicer looking diff: https://github.com/poyaoc97/llvm-project/commit/f594442f9614
It seems that GitHub has a VERY different way of displaying diff.

Tests passed on my machine (x86_64 Ubuntu)

rZhBoYao updated this revision to Diff 421338.Apr 7 2022, 2:33 PM
rZhBoYao retitled this revision from [Clang] Diagnose incomplete return/param types only when function is not deleted to [Clang] CWG 1394: Incomplete types as parameters of deleted functions.
rZhBoYao edited the summary of this revision. (Show Details)
rZhBoYao removed a reviewer: rtrieu.
rZhBoYao set the repository for this revision to rG LLVM Github Monorepo.

Updating the status of CWG 1394 is the only change.

rZhBoYao updated this revision to Diff 421345.Apr 7 2022, 3:02 PM

Added release note.

Also, push to my repo to make sure the release note is correctly rendered:
https://github.com/poyaoc97/llvm-project/commit/1167f0fc6b91

I agree this one is better.

clang/include/clang/Sema/Sema.h
2899–2909

Agree to @erichkeane

rZhBoYao marked an inline comment as done.Apr 7 2022, 10:27 PM
rZhBoYao added inline comments.
clang/include/clang/Sema/Sema.h
2899–2909

With all due respect, this code suggestion doesn't make any sense to me. My best guess is @ChuanqiXu was thinking the order specified by the grammar as noted in dcl.fct.def.general p1. Even if that was the case, CompoundStmt is not quite right either. Also, differentiating ctor-initializer[opt] compound-statement and function-try-block is meaningless here, hence the name Other.

I adopted the same order as to how Parser::ParseFunctionDefinition has always been parsing function-body. The order is not significant in any meaningful way as each of the 4 grammar productions of function-body is VERY different and mutually exclusive. Putting Delete and Default upfront not only emphasizes the "specialness" of them but also conveys how we handle function-body.

What say you, @erichkeane ?

ChuanqiXu added inline comments.Apr 7 2022, 10:45 PM
clang/include/clang/Sema/Sema.h
2899–2909

Yeah, the order comes from the standard. I think the comment should be consistent with the spec. And for the name, I agree CompoundStmt is not accurate indeed... I thought Default should be a good name but there is Default already. But I don't feel Other is good since the compound-statement is much more common.

Putting Delete and Default upfront not only emphasizes the "specialness" of them but also conveys how we handle function-body.

This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.

rZhBoYao marked 2 inline comments as done.Apr 7 2022, 10:54 PM
rZhBoYao added inline comments.
clang/include/clang/Sema/Sema.h
2899–2909

But I don't feel Other is good since the compound-statement is much more common.

Other reads "not special like Delete and Default".

This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.

Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.

I'll await @erichkeane 's final verdict.

erichkeane accepted this revision.Apr 8 2022, 6:21 AM

I'm going to accept, I am ok with it as-is, though I would like to see @ChuanqiXu be accepting of whatever we do in Sema's enum as well. So please wait for his feedback too.

clang/include/clang/Sema/Sema.h
2899–2909

Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.

This was my understanding. This is a single value that means 'not default and not deleted'. I think the idea of doing it in standards order is a really good one, but have the top 2 just both result in Other, Unknown, etc. So something like:

//ctor-initializeropt compound-statement
//function-try-block
Unknown,
//= default ;
Default,
//= delete ;
Delete

BUT as this is not an 'ast' level flag, it is just for the purposes of simplifying this call, I don't think it needs to conform that closely.

This revision is now accepted and ready to land.Apr 8 2022, 6:21 AM
rZhBoYao updated this revision to Diff 421589.EditedApr 8 2022, 10:59 AM
rZhBoYao marked 2 inline comments as done.
rZhBoYao set the repository for this revision to rG LLVM Github Monorepo.

Reordered enumerators.

Does this look good to you, @ChuanqiXu ?

I will land this later next week if no feedback is received.

Revisiting @ChuanqiXu 's code suggestion...

clang/include/clang/Sema/Sema.h
2899–2909

I actually don't know what you were trying to say here:

We don't yet part function-try-block yet.
FunctionTryBlock,

especially:

We don't yet part function-try-block yet.

Nonetheless, I did my best to cater to your comment within reason, such as reordering enumerators to align with the C++ standard text.

ChuanqiXu accepted this revision.Apr 11 2022, 6:54 PM

LGTM then.

This revision was landed with ongoing or failed builds.Apr 11 2022, 8:18 PM
This revision was automatically updated to reflect the committed changes.