[clang][github] Added checking for completeness of lvalue in conditional operator [#59718]

[Aditya-pixel]

Summary

[ISSUE] https://github.com/llvm/llvm-project/issues/59718

struct x y;
int main(void)
{
    (void)(1 ? y : y);
}
struct x {int i;};

The conditional operator(?:) requires the second and third operands to be of compatible types. To be compatible, they also need to be complete (however, both can be void). Therefore, the expected response from clang after running the above code as a C program should be error dialogue pointing out that both the types are incomplete hence incompatible, but the code compiled without any errors.
The patch ensures the completeness in the CheckCondtionalOperand function present in llvm-project/clang/lib/Sema/SemaChecking.cpp

[junaire]

Hi, thanks for the patch, but AFAICT it at least has issues below:

1. You need to add a regression test, probably in clang/test/Sema.
2. You may need to update the release note, in clang/docs/ReleaseNotes.rst.
3. You may need to format your patch, see more details at https://llvm.wiki/llvm/overview/#submit-a-patch
4. Would it be better if you could add a comment on it?

[shafik]

Thank you for taking the time to submit a patch.

In addition to the previous comments, please also add a summary of the problem and why this is a fix for it.

It would also be good to reference the github bug, I usually add a link some folks just add the issue number.

[Aditya-pixel]

Thank you for all the suggestions and guidance. I will make the changes in the revision and also update the clang tests for Sema.

[Aditya-pixel]

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

[aaron.ballman]

In D144358#4145514, @Aditya-pixel wrote:

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

I don't think we have a good tutorial for adding tests to Clang (the tests kind of "go where they belong" which is a bit hard to define, as you might imagine!). In this case, I would add the test coverage to test/Sema/incomplete-decl.c because that test is about incomplete declarations in general. You might also want to add a second RUN line to that file which passes -x c++ to test the behavior in C++ as well as C, given that there's a difference in behavior between the two.

clang/lib/Sema/SemaChecking.cpp

14372–14375

You might as well use RequireCompleteExprType(E, diag::err_incomplete_type) as it's slightly simpler but does the same thing.

[Aditya-pixel]

In D144358#4147287, @aaron.ballman wrote:

In D144358#4145514, @Aditya-pixel wrote:

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

I don't think we have a good tutorial for adding tests to Clang (the tests kind of "go where they belong" which is a bit hard to define, as you might imagine!). In this case, I would add the test coverage to test/Sema/incomplete-decl.c because that test is about incomplete declarations in general. You might also want to add a second RUN line to that file which passes -x c++ to test the behavior in C++ as well as C, given that there's a difference in behavior between the two.

Hi! Thanks for the help. I found a nice documentation about regression tests which is quite detailed. I'll edit the code to RequireCompleteExprType(E, diag::err_incomplete_type) and run the tests.

[Aditya-pixel]

Hello! I have updated the patch and added tests for C and C++. The tests are compatible and code is working as expected. Please do let me know if any changes are to be done or tests needs to be increased. Thank you.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

14373–14375

Our coding style would have you drop the curly braces here.

clang/test/Sema/incomplete-decl.c

2–4 ↗

(On Diff #502384)

We typically try to keep #if conditions like this out of test code because it makes it too easy to not know what part of the test is actually being tested. There's a different approach that I think will work better here: use -verify=cxx on the C++ RUN line.

-verify turns on the diagnostic verifier, which looks for those expected-whatever comments. When you use -verify=foo, the diagnostic verifier looks for foo-whatever instead of expected-whatever. So if you use -verify=cxx, you can specify the C++-specific diagnostics with cxx-error {{words}} and the C-specific ones with expected-error {{different words}} which lets you get rid of the #if.

[Aditya-pixel]

Hello! I tried this but I am getting some errors. I think the c++ RUN line is also checking the earlier defined test cases and throwing ['error' diagnostics seen but not expected.] Would should I do? Thanks.

[aaron.ballman]

In D144358#4176998, @Aditya-pixel wrote:

Hello! I tried this but I am getting some errors. I think the c++ RUN line is also checking the earlier defined test cases and throwing ['error' diagnostics seen but not expected.] Would should I do? Thanks.

You can use multiple prefixes with -verify, so if you have some diagnostics in C++, some in C, and some in both, I usually do:

// RUN: %clang_cc1 -fsyntax-only -verify=expected, c %s
// RUN: %clang_cc1 -fsyntax-only -x c++ -verify=expected, cxx %s

void bad_code(); // c-warning {{this is bad in C}}
void more_bad_code(void); // cxx-warning {{this is bad in C++}}
void wow_this_is_horrible(float); // expected-warning {{this is bad in both C and C++}}

[Aditya-pixel]

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

[Aditya-pixel]

PS: How do I take care of code where both C++ and C throw error but error messages are different? Here's an example:
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: tentative definition has type 'struct foo' that is never completed - expected in C
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: variable has incomplete type 'struct foo' - error thrown by C++.
Thank you.

[aaron.ballman]

In D144358#4177899, @Aditya-pixel wrote:

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

[aaron.ballman]

In D144358#4177937, @Aditya-pixel wrote:

PS: How do I take care of code where both C++ and C throw error but error messages are different? Here's an example:
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: tentative definition has type 'struct foo' that is never completed - expected in C
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: variable has incomplete type 'struct foo' - error thrown by C++.
Thank you.

Good question! I handle that like this:

struct foo bad_code; // c-warning {{tentative definition has type 'struct foo' that is never completed}} \
                        cxx-warning {{variable has incomplete type 'struct foo'}}

(Note, I'm using a line continuation character (\) to continue the comment from one line to the next so the comments are both associated with the same source line.)

[Aditya-pixel]

In D144358#4177950, @aaron.ballman wrote:

In D144358#4177899, @Aditya-pixel wrote:

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

[aaron.ballman]

In D144358#4177982, @Aditya-pixel wrote:

In D144358#4177950, @aaron.ballman wrote:

In D144358#4177899, @Aditya-pixel wrote:

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

The trouble is that the comments for verifying the diagnostics aren't using the new prefix you added (and you missed adding the c prefix to that RUN line).

The run lines should be:

// RUN: %clang_cc1 -fsyntax-only -verify=c,expected %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=cxx,expected %s

and the comments should be more like:

static void c; // expected-error {{variable has incomplete type 'void'}}
static struct foo g;  // cxx-warning {{tentative definition of variable with internal linkage has incomplete non-array type 'struct foo'}} \
    c-error{{tentative definition has type 'struct foo' that is never completed}}

Where expected-whatever is being checked for both RUN lines, c-whatever is being checked only for the first RUN line, and cxx-whatever is being checked only for the second RUN line.

[Aditya-pixel]

In D144358#4177998, @aaron.ballman wrote:

In D144358#4177982, @Aditya-pixel wrote:

In D144358#4177950, @aaron.ballman wrote:

In D144358#4177899, @Aditya-pixel wrote:

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

The trouble is that the comments for verifying the diagnostics aren't using the new prefix you added (and you missed adding the c prefix to that RUN line).

The run lines should be:

// RUN: %clang_cc1 -fsyntax-only -verify=c,expected %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=cxx,expected %s

and the comments should be more like:

static void c; // expected-error {{variable has incomplete type 'void'}}
static struct foo g;  // cxx-warning {{tentative definition of variable with internal linkage has incomplete non-array type 'struct foo'}} \
    c-error{{tentative definition has type 'struct foo' that is never completed}}

Where expected-whatever is being checked for both RUN lines, c-whatever is being checked only for the first RUN line, and cxx-whatever is being checked only for the second RUN line.

Thanks I understand now, I'll make the appropriate changes in the testing file.

[Aditya-pixel]

This comment was removed by Aditya-pixel.

[Aditya-pixel]

UPDATED: Removed the initial use of #ifdef and added separate tests for C and C++. Here c-<diagnostic> means diagnostic is specific to C, cxx-<diagnostic> means diagnostic is specific to c++ and expected-<diagnostic> means diagnostic is common to both.

[junaire]

Can you run git clang-format HEAD~1 to format your patch? (Put https://github.com/llvm/llvm-project/blob/main/clang/tools/clang-format/git-clang-format in your PATH, like ~/.local/bin the command will work)

[Aditya-pixel]

Hello! I have added the diff file after running the clang-format command. However, it outputs no files changed. Is the patch I updated formatted?

I am getting this message.

[aaron.ballman]

In D144358#4208735, @Aditya-pixel wrote:

Hello! I have added the diff file after running the clang-format command. However, it outputs no files changed. Is the patch I updated formatted?

I am getting this message.

I don't see any formatting issues in the patch you've uploaded, and precommit CI didn't find any concerns either, so I think you're fine.

clang/test/Sema/incomplete-decl.c

7–9 ↗

(On Diff #506853)

cxx-note 3 {{forward declaration of 'foo'}} instead of duplicating the same diagnostic across multiple lines?

Also, to help make it easier to read, can you indent the second line so that cxx-note aligns with c-note from the line above? (Same suggestion throughout the file.)

[Aditya-pixel]

UPDATED: Formatted the file, added indents and condensed comments.

[aaron.ballman]

LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.

[Aditya-pixel]

In D144358#4219311, @aaron.ballman wrote:

LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.

Hello! If it's not an inconvenience you could commit the changes and add the release notes.
Name: Aditya Singh
Email Address: [e-mail]
Thanks a lot.

[Aditya-pixel]

UPDATE: Added release note, please have a look and suggest if anything more needs to be mentioned. Thanks.

[aaron.ballman]

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

clang/docs/ReleaseNotes.rst

137 ↗

(On Diff #508433)

This should be:

(`#59718 <https://github.com/llvm/llvm-project/issues/59718>`_)

with the trailing _.

[Aditya-pixel]

In D144358#4225034, @aaron.ballman wrote:

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

[aaron.ballman]

In D144358#4225123, @Aditya-pixel wrote:

In D144358#4225034, @aaron.ballman wrote:

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I'll fix that one up and land it as an NFC change, so you don't need to worry about that. Thanks for pointing it out though!

[aaron.ballman]

In D144358#4225124, @aaron.ballman wrote:

In D144358#4225123, @Aditya-pixel wrote:

In D144358#4225034, @aaron.ballman wrote:

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I'll fix that one up and land it as an NFC change, so you don't need to worry about that. Thanks for pointing it out though!

Ah that one was already fixed it seems: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst?plain=1#L171

[Aditya-pixel]

Oh, okay, Ill change the patch now. Also, it seems my local directory is out of sync. Should I always update it before submitting a patch, or is it alright without updating it?

[aaron.ballman]

In D144358#4225139, @Aditya-pixel wrote:

Oh, okay, Ill change the patch now. Also, it seems my local directory is out of sync. Should I always update it before submitting a patch, or is it alright without updating it?

It's usually good to rebase each time you update the patch, but it's not a requirement either (sometimes it's easier to rebase at the end instead of while the patch is under review). I've landed the changes for you in dedd7b6548f4a37f4f691aa0cd3a709756b7e794

clang/lib/Sema/SemaChecking.cpp

 static void CheckConditionalOperand(Sema &S, Expr *E, QualType T,
                                     SourceLocation CC, bool &ICContext) {
   E = E->IgnoreParenImpCasts();
+  if (!S.getLangOpts().CPlusPlus && E->getType()->isRecordType()) {
+    S.RequireCompleteType(E->getBeginLoc(), E->getType(),
+                          diag::err_incomplete_type);
+  }
 
   if (auto *CO = dyn_cast<AbstractConditionalOperator>(E))
     return CheckConditionalOperator(S, CO, CC, T);