[C2X] N3007 Type inference for object definitions

[to268]

Summary

This patches implements the auto keyword from the N3007 standard specification.
This allows deducing the type of the variable like in C++

auto nb = 1;
auto chr = 'A';
auto str = "String";

The list of statements which allows the usage of auto:

• Basic variables declarations (int, float, double, char, char*...)
• Macros declaring a variable with the auto type

The list of statements which will not work with the auto keyword:

• auto arrays
• sizeof(), alignas()
• auto parameters, auto return type
• auto as a struct/typedef member
• uninitialized auto variables
• auto in an union
• auto as a enum type specifier
• auto casts
• auto in an compound literals

[aaron.ballman]

In D133289#4037180, @tschuett wrote:

It surprised me that there are no type inference messages? The type of this auto is double. I only found warnings of misuse of auto and codegen tests.

Can you give a code example of what you expected to see a diagnostic for but aren't getting one?

[tschuett]

In D133289#4096588, @aaron.ballman wrote:

In D133289#4037180, @tschuett wrote:

It surprised me that there are no type inference messages? The type of this auto is double. I only found warnings of misuse of auto and codegen tests.

Can you give a code example of what you expected to see a diagnostic for but aren't getting one?

This is probably only for testing and not for Clang users:

auto Float = 3.0; // note that Float is a float

[to268]

Patch update: Reverting auto as a type specifier.
I am currently figuring out what behavior the auto keyword need to match.

[to268]

I have improved some diagnostics and i am now testing nullptr with auto.

[to268]

I have *hopefully* fixed my broken patch.
This is all the things in need to address:

auto str2[] = "string";
[[clang::overloadable]] auto test(auto x) { return x; }

and maybe retry to support auto in compound literals,
becaue i have seen heavy macro users that want compound literals to work.

I think it's safe to say that we will not support all features from N3076 in this patch.

[to268]

Fixed auto char arrays with explicit brackets

c
auto foo[] = "bar";

[aaron.ballman]

Thank you for the continued work on this! In general, I think the test coverage should verify the deduced type information more often. e.g., if we accept the code, there should probably be a static_assert nearby that ensures the deduced type is what we'd expect.

I applied the patch locally and tried out various things, and here's some test coverage I think we're lacking.

// Important: these two declarations are at file scope, we're testing that the storage
// class specifier is ignored because we're deducing the type.
auto i = 12; // Ok, deduces int
auto j = 12ull; // Ok, deduces unsigned long long

// This is testing whether storage specifier order matters:
void func() {
  const auto static i = 12; // Ok, static variable of type const int
}

In D133289#4279054, @to268 wrote:

I have *hopefully* fixed my broken patch.
This is all the things in need to address:

auto str2[] = "string";
[[clang::overloadable]] auto test(auto x) { return x; }

and maybe retry to support auto in compound literals,
becaue i have seen heavy macro users that want compound literals to work.

I think it's safe to say that we will not support all features from N3076 in this patch.

My testing of your patch suggests those aren't issues left to address (perhaps). e.g.,

auto test[] = "foo";
static_assert(_Generic(test, char * : 1, default : 0));

this matches what I'd expect (no warning, no failed assertion; string literals have type char [] in C, not const char[].

My testing of __attribute__((overloadable)) with your patch is that we currently reject use of auto in those functions. I think that's good initial behavior -- if we want to allow auto there, that's probably better left to a follow-up patch anyway because I'd expect there to be some debate around whether that's a good idea or not.

As for compound literals, I think we've determined they're not supported by C currently.

clang/test/C/C2x/n3007.c

21

This diagnostic is incorrect -- I believe this code should be accepted.

22

This diagnostic is correct.

32–33

We should test that the types match our expectations:

double A[3] = { 0 };
auto pA = A; // pA is double * due to array decay on A
static_assert(_Generic(pA, double * : 1, default : 0));
auto qA = &A; // qA is double (*)[3]
static_assert(_Generic(qA, double (*)[3] : 1, default : 0));

47

I think this would make it more clear that what's being tested is the _Alignas behavior.

57–58

This should now be resolved in N3096 so the FIXME comment can be removed.

67

This is awfully close to the examples from the paper (6.7.9p3):

auto p1 = (struct { int a; } *)0; // error expected

struct s;
auto p2 = (struct s { int a; } *)0; // error expected

They are expected to be diagnosed due to changes in N3006 (underspecified object definitions), so I think it's fine to add a FIXME comment with the test cases instead of trying to also implement N3006 at the same time. We currently list that as unknown in our status page, but I think it's clearly a no now.

101

No longer accurate (see comments below), so I think this FIXME comment can be removed.

107–108

I'd like to see a test that passes -pedantic which diagnoses these declarations with a warning about accepting this code being a Clang extension. (The standard requires we support auto ident = expr; but leaves other declarator pieces up to the implementation.)

Note, 6.7.9p2 made this implementation-defined behavior, so we also need to add some documentation to meet that requirement.

clang/test/Parser/c2x-auto.c

47–52

The C2x diagnostics here are kind of mean because if you give an initializer, we then reject the code anyway. If possible, it'd be nice to reject this for use of auto with an array declarator rather than the lack of an initializer.

79–82

This is tested below with a more clear test.

clang/test/Sema/c2x-auto.c

27–28

This FIXME is no longer accurate, I think it's not clear whether we should accept or reject this code from reading the standard, but I'm now thinking we should reject it for two reasons: 1) 6.7.9p1 says "A declaration for which the type is inferred shall contain the storage-class specifier auto", but a compound literal is not a declaration, it's an expression. What's more, it doesn't have a direct declarator or an equals sign as specified in p2. 2) GCC rejects: https://godbolt.org/z/K8sqr9qTT

[to268]

I have updated and fixed all minors comments about the test cases.
I have added the diagnostic to warn users about using a Clang extension when declaring an explicit auto* in pedantic mode.
I need to debug more about the two remaining diagnostics to fix:

• Wrong diagnostic when using the _Atomic attribute
• Misleading diagnostic where it should say that auto is not allowed but says that [...] requires an initializer.

[to268]

I have fixed the misleading diagnostic with auto array declarations.
The diagnostics now says that 'auto' is not allowed in array declarations.

I am not sure with the way i have currently implemented it in SemaDecl.cpp, maybe it can be done in a cleaner way.

[to268]

Minor code reformatting

[aaron.ballman]

Thank you for your patience while WG14 finalized the feature and I wrapped my head around the moving parts involved. I think this is shaping up nicely, but there's still some diagnostic issues. The biggest ones are:

• We should have an extension warning for array use with string literals auto str[] = "testing";
• We should be careful about testing underspecified declarations in a way that give the impression they're an extension, so we should sprinkle more fixme comments around the tests
• _Atomic auto x = 12; is now something we should be accepting and deduce as an _Atomic int

I think there's a typo in the patch summary:

auto in an compound statement

I think you mean "as the type of a compound literal"?

clang/include/clang/Basic/DiagnosticSemaKinds.td

243–245

I think we'll need this diagnostic for more than just * in a declaration. Consider a case like:

auto val = (struct X { int y; }){ 0 };

accepting this is also an extension because it declares y, which is not an "ordinary" identifier. See 6.7.9p2-3

Also, because this is about the syntax used, I think this might be a diagnostic that should live in DiagnosticParseKinds.td (but maybe it's correct here, I've not reviewed enough yet to see).

2469–2470

I'm surprised to see decltype(auto) here as decltype isn't a keyword in C so it'd be invalid syntax; let's switch it to something that makes it obvious that we should never issue that particular part of the diagnostic in C.

clang/lib/Sema/SemaDecl.cpp

12684–12685

Comment can be re-flowed to 80 columns and the comment should end in a period. (Weirdly, Phabricator isn't letting me suggest edits just within this file.)

12687–12688

This can switch to using !isa_and_present<StringLiteral, InitListExpr>(Init) (the _and_nonnull variant is quietly deprecated).

12690

Please add a /* */ comment before 23 detailing what that magic number will be printing.

clang/lib/Sema/SemaTemplateDeduction.cpp

4845

4854–4857

I think this diagnostic should happen from the parser rather then when determining the concrete type; that ensures consistent diagnosis of the syntax regardless of whether we needed to deduce the type or not.

clang/test/C/C2x/n3007.c

7–23

Adding one more test case for restrict-qualified pointers.

26

What we finally landed on in WG14 is that this should be accepted. In this form, _Atomic is being used as a type qualifier (not a type specifier), and 6.7.9p2 explicitly talks about qualifiers: "The inferred type of the declared object is the type of the assignment expression after lvalue, array to pointer or function to pointer conversion, additionally qualified by qualifiers and amended by attributes as they appear in the declaration specifiers, if any."

So this should form an _Atomic int initialized to the value 12, and that matches GCC's behavior: https://godbolt.org/z/s468s86z6

33

The first error on this line isn't helpful; the second error is the only one I'd expect to see here.

64

To make it clear that use of auto within sizeof is the issue, not the initialization itself.

82–85

Might need to re-flow the comment as well.

103

This should also get the "is a clang extension" warning because the declarators include [].

117

clang/test/CodeGen/auto.c

16

20–23

I'd like to see a sema test for this:

auto t = ({
  auto b = 12;
  b;
)};
_Generic(t, int : 1);

clang/test/Parser/c2x-auto.c

72–73

98

123

This one should be fixed and accepted.

125

I'd like to see parsing tests for:

auto ident [[clang::annotate("this works")]] = 12; // No diagnostic expected

We should also add a test that this C++'ism doesn't work in C:

int a = auto(0);

clang/test/Sema/c2x-auto.c

75

This should also get an extension warning.

120

Some additional test cases to consider:

_Complex auto i = 12.0; // Should be rejected because _Complex is a type specifier; however, 
                        // when auto becomes a type specifier, this should be accepted. GCC
                        // rejects but if we end up accepting, I won't be sad -- we'll need an
                        // extension warning in that case though.

void foo(void) {
  extern void foo(int a, int array[({ auto x = 12; x;})]); // This is a use of `auto` within function prototype
                                                           // scope, but should still be accepted.
}

clang/www/c_status.html

1189

This can be committed as an NFC change separate from this review, given that it's slightly orthogonal.

[aaron.ballman]

In D133289#4489883, @aaron.ballman wrote:

• _Atomic auto x = 12; is now something we should be accepting and deduce as an _Atomic int

The committee is discussing this again on the reflectors. Thus far, almost everyone reads the standard the same way as GCC did with their implementation, which matches what I suggest above. However, there are folks who are claiming we should not be able to deduce the derived type because _Atomic forms an entirely new type and thus isn't actually a qualifier (and there are some words in the standard that could maybe be read as supporting that). The most conservative approach is to reject using _Atomic auto for right now so users don't build a reliance on it. Eventually WG14 will make a decision and we can relax that diagnostic then if we need to. Sorry for the confusion on this topic!

clang/test/Sema/c2x-auto.c

120

The suggested comment I have isn't fully correct. It should be rejected because _Complex is a type specifier, but when auto becomes a type specifier, I think _Complex perhaps should still not deduce. Consider this analogous case (which could be a fun test case as well):

signed auto a = 1L;

signed is a type specifier as well, and this is not accepted in C++ (so we don't want to accept it in C either).

[to268]

Thank you for your feedback @aaron.ballman!
I really appreciate that you are pointing out all my formatting mistakes, and giving me more guidelines.
The direction of the patch is clearer now.

In D133289#4489883, @aaron.ballman wrote:

I think there's a typo in the patch summary:

auto in an compound statement

I think you mean "as the type of a compound literal"?

Yes i was a mistake, I have edited the summary to fix that typo.

In D133289#4489883, @aaron.ballman wrote:

• We should have an extension warning for array use with string literals auto str[] = "testing";

This makes sense, since auto arrays are prohibited in the standard.

In D133289#4497901, @aaron.ballman wrote:

The committee is discussing this again on the reflectors. Thus far, almost everyone reads the standard the same way as GCC did with their implementation, which matches what I suggest above. However, there are folks who are claiming we should not be able to deduce the derived type because _Atomic forms an entirely new type and thus isn't actually a qualifier (and there are some words in the standard that could maybe be read as supporting that). The most conservative approach is to reject using _Atomic auto for right now so users don't build a reliance on it. Eventually WG14 will make a decision and we can relax that diagnostic then if we need to. Sorry for the confusion on this topic!

I was wondering about the support of _Atomic auto, i will add new error diagnostic to prohibit _Atomic auto, thank you for addressing that topic!

[to268]

Added recommendations from @aaron.ballman.
Adjusting diagnostic messages still need to be done.

clang/test/Sema/c2x-auto.c

120

void foo(void) {
  extern void foo(int a, int array[({ auto x = 12; x;})]); // This is a use of `auto` within function prototype
                                                           // scope, but should still be accepted.
}

I think you made a mistake there by using the same function name as the outer one.

[to268]

This update is mostly a followup of a change upstream that consist of renaming all C2x references to C23.

[to268]

This updates fixes the _Atomic auto diagnostic, the previous diagnostic was confusing since it was referencing a C++ feature (std::is_trivially_copyable).
I have a few diagnostic issues remaining, some parts need to take place as earlier in the Parsing stage and be diagnosed in the Sema stage.
PS: I'll migrate this patch to GH at the end of September if I need to.

[to268]

Fixed the initializer list diagnotic

[to268]

I need thoughts about this.

clang/lib/Sema/SemaTemplateDeduction.cpp

4862–4864

It feels wrong to check if it's not C++ and changing the diagnostic if we are in C, I only emit the fixed diagnostic in C23 mode for now.

[aaron.ballman]

I spotted some minor issues, but this LGTM for the initial implementation. Thank you for all your effort on this project!!

There are some outstanding issues that should be handled in a follow up, however:

void foo() {
   auto l = (struct S { int x, y; }){ 1, 2 }; // Underspecified declaration, should diagnose

  _Atomic int i;
  _Static_assert(_Generic(&i, _Atomic auto *: 1)); // Gets two errors instead of just 1

  auto int i = 12; // At local and file scope, now gives: warning: 'auto' storage class specifier is not permitted in C++11, and will not be supported in future releases

  auto __attribute__((mode(HI))) i = 12; // Gives confused diagnostics, but GCC also doesn't accept currently either
  _Static_assert(_Generic(i, short : 1)); // But if we accepted the above, this should pass. 
}

but these all seem minor enough to be handled in follow-up work.

Do you need me to land this on your behalf? If so, I can fix up the NFC changes I found for you, but what email address would you like me to use for patch attribution?

clang/lib/Sema/DeclSpec.cpp

1379–1380

clang/lib/Sema/SemaDecl.cpp

12869

/*in array decl*/ instead of here

clang/test/C/C2x/n3007.c

33

To be clear, I mean this example should have one diagnostic instead of two:

_Static_assert(_Generic(&i, _Atomic auto *: 1)); // expected-error {{_Atomic cannot be applied to type 'auto' in C23}} \
                                                    expected-error {{'auto' not allowed here}}

clang/test/Sema/c2x-auto.c

111–112

[to268]

Do you need me to land this on your behalf? If so, I can fix up the NFC changes I found for you, but what email address would you like me to use for patch attribution?

It is probably the best thing to do, and yes I need you to land this on your behalf. Guillot Tony <[e-mail]>

There are some outstanding issues that should be handled in a follow up

I will start to work on these issues, when the patch has been landed.

Thank you for the review!

[to268]

Fix arm buildbot issues