Recover more gracefully when __declspec is not supported as a keyword

[aaron.ballman]

Summary

In r238238, we removed declspec as a universally-accepted keyword -- instead, it is only enabled as a supported keyword when -fms-extensions or -fdeclspec is passed to the driver. However, this had an unfortunate side-effect in that it made for bad diagnostics in the presence of a declspec when neither of those flags are passed. For instance:

__declspec(naked) void f(void) {}

clang -fsyntax-only -fno-ms-extensions -Wall -pedantic main.c

main.cpp:1:24: error: parameter named 'f' is missing
__declspec(naked) void f(void) {}

^

main.cpp:1:31: error: expected ';' at end of declaration
__declspec(naked) void f(void) {}

^
;

main.cpp:1:12: warning: parameter 'naked' was not declared, defaulting to type 'int' [-Wpedantic]
__declspec(naked) void f(void) {}

^

main.cpp:1:1: warning: type specifier missing, defaults to 'int' [-Wimplicit-int]
__declspec(naked) void f(void) {}

This patch addresses the poor diagnostics by noticing when a __declspec has been used but is not enabled. It eats the attribute, diagnoses the use, and then attempts to continue parsing.

[compnerd]

lib/Parse/ParseDecl.cpp

2973

Shouldn't this be getLangOpts().DeclSpecKeyword since you don't need the Microsoft extensions as much as you need the declspec keyword to be processed. Having MicrosoftExt implicitly enable DeclSpecKeyword (just as borland mode enables it as well). I suppose that it would fail anyways as Tok.is(tok::identifier) would fail.

2989

I think that we want to emit the diagnostic even if there is no parenthesis as __declspec is a reserved identifier, and we would normally diagnose the bad __declspec (expected '(' after '__declspec').

[aaron.ballman]

Correcting review feedback.

lib/Parse/ParseDecl.cpp

2973

That's a good point, I've corrected.

2989

Yes, but it could also lead to a rejecting code that we used to accept and properly handle when __declspec is an identifier rather than a keyword. e.g.,

struct __declspec {};

__declspec some_func(void);

By looking for the paren, we run less risk of breaking working code, even if that code abuses the implementation namespace (after all, __declspec it not a keyword in this scenario).

[compnerd]

lib/Parse/ParseDecl.cpp

2989

But we would reject that code under -fdeclspec anyways. I think having the code be more portable is a bit nicer.

[aaron.ballman]

Fixed review feedback

[aaron.ballman]

lib/Parse/ParseDecl.cpp

2989

After discussing in IRC, I decided that I agree with @compnerd on this and have changed the patch accordingly.

[majnemer]

lib/Parse/ParseDecl.cpp

2989

What if somebody wants to use __declspec and are using the compiler in a freestanding mode? Also, we aren't the only member of the implementor's namespace.

[aaron.ballman]

lib/Parse/ParseDecl.cpp

2989

Users using __declspec in a freestanding mode is a concern that I share. However, I imagine there are *far* more users who accidentally forget to pass -fdeclspec than there are users who are writing code in freestanding mode that wish to use __declspec as an identifier in a situation that proves problematic. That being said, do you think the approach in the patch would work with a warning rather than an error? I went with an error because I felt that a warning would require tentative parsing to be properly implemented, which felt like a heavy solution for a problem I didn't think anyone would run into in practice.

I'm not overly sympathetic to others in the implementor's namespace who use __declspec but not to implement attributes under that name. However, I could be convinced to be sympathetic if there was some conflict in practice. Do you have a case in mind?

[majnemer]

lib/Parse/ParseDecl.cpp

2989

I suppose what you've got should be fine in practice. Heck, we used to have __declspec attributes available all the time...

[aaron.ballman]

Commit in r295114.

include/clang/Basic/DiagnosticParseKinds.td

 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup<IgnoredAttributes>;
+def err_ms_attributes_not_enabled : Error<
+  "'__declspec' attributes are not enabled; use '-fdeclspec' or "
+  "'-fms-extensions' to enable support for __declspec attributes">;
 def err_expected_method_body : Error<"expected method body">;
 def err_declspec_after_virtspec : Error<
   "'%0' qualifier may not appear after the virtual specifier '%1'">;

lib/Parse/ParseDecl.cpp

       if (DS.hasTypeSpecifier())
         goto DoneWithDeclSpec;
 
+      // If the token is an identifier named "__declspec" and Microsoft
+      // extensions are not enabled, it is likely that there will be cascading
+      // parse errors if this really is a __declspec attribute. Attempt to
+      // recognize that scenario and recover gracefully.
+      if (!getLangOpts().DeclSpecKeyword && Tok.is(tok::identifier) &&
+          Tok.getIdentifierInfo()->getName().equals("__declspec")) {
+        Diag(Loc, diag::err_ms_attributes_not_enabled);
+
+        // The next token should be an open paren. If it is, eat the entire
+        // attribute declaration and continue.
+        if (NextToken().is(tok::l_paren)) {
+          // Consume the __declspec identifier.
+          SourceLocation Loc = ConsumeToken();
+
+          // Eat the parens and everything between them.
+          BalancedDelimiterTracker T(*this, tok::l_paren);
+          if (T.consumeOpen()) {
+            assert(false && "Not a left paren?");
+            return;
+          }
+          T.skipToEnd();
+          continue;
+        }
+      }
+
       // In C++, check to see if this is a scope specifier like foo::bar::, if
       // so handle it as such.  This is important for ctor parsing.
       if (getLangOpts().CPlusPlus) {

test/Parser/declspec-recovery.c

+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+
+struct S {
+  __declspec(property(get=Getter, put=Setter)) int X; // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+  int Y;
+};

test/Parser/declspec-supported.c

+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions -verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+  __declspec(property(get=Getter, put=Setter)) int X;
+  int Y;
+};