Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[shivanshu3]

Summary

Goal: Clang should be able to parse the following code with '-fms-compatibility' because MSVC allows using the 'enum' specifier for typedef'd enums:

typedef enum
{
    First,
    Second
} MyEnum;

void func()
{
    enum MyEnum foo;
}

Today the code above produces the following compile error:

<source>:9:7: error: typedef 'MyEnum' cannot be referenced with a enum specifier
        enum MyEnum foo;
             ^
<source>:5:3: note: declared here
} MyEnum;
  ^

The reason why this change is desired in Clang is because the MSVC tools 'mktyplib' and 'midl' produce C++ code which uses the 'enum' specifier in such a way. Here is a small repro:

MyEnum.idl:

[
      uuid(da2889bf-6173-4c1c-a23d-52ad85fc91ca)
]
library MyEnumLib
{
      typedef enum { X } MyEnum;
};

Application.idl:

[
      uuid(4d0cc96f-3258-46f7-98bf-6654c1d027c5)
]
library ApplicationLib
{
      importlib("MyEnum.tlb");

      [
            uuid(6ac4c3d7-15c7-40c7-8b64-60459e29680b)
      ]
      interface ApplicationInterface : IDispatch
      {
            [
                  id(66),
                  propget
            ]
            HRESULT Foo([out, retval] enum MyEnum* Foo);

            [
                  id(66),
                  propput
            ]
            HRESULT Foo([in] enum MyEnum Foo);
      };
};

Use the following commands to build:

mktyplib /win32 /cpp_cmd cl.exe /cpp_opt /E /tlb MyEnum.tlb /h MyEnum.h MyEnum.idl
midl -h Application.h -iid Application.c -tlb Application.tlb /cpp_cmd cl.exe Application.idl

This produces Application.h with the following snippet (note the usage of the 'enum' specifier):

MIDL_INTERFACE("6ac4c3d7-15c7-40c7-8b64-60459e29680b")
ApplicationInterface : public IDispatch
{
public:
    virtual /* [propget][id] */ HRESULT STDMETHODCALLTYPE get_Foo( 
        /* [retval][out] */ enum /* external definition not present */ MyEnum *Foo) = 0;
    
    virtual /* [propput][id] */ HRESULT STDMETHODCALLTYPE put_Foo( 
        /* [in] */ enum /* external definition not present */ MyEnum Foo) = 0;
    
};

Note that the 'mktyplib' tool is deprecated now but it's used for building some code here at Microsoft, and probably other legacy build scenarios in the industry as well.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

15545–15546

Our coding style does not parenthesize == within &&.

15551–15567

This can be simplified substantially.

15739–15745

In the anonymous union case, we also need to check for qualifiers on the typedef type, and more broadly perhaps we should be using getAnonDeclWithTypedefName here too. Presumably the new case should also only apply for TUK_Reference.

15740

clang/test/Sema/enum-typedef-msvc.c

16

Please also add a testcase for the situation where the use of enum Foo occurs in the same scope as the typedef and anonymous enum declaration. (MSVC has weird behavior here that we don't necessarily need to follow, but we should make sure we at least do something defensible.)

[shivanshu3]

Addressing some of rsmith's comments:

• Add a test case where an enum variable is declared in the same scope as the enum definition
• Fix a style issue with parentheses
• Simplify the if condition for the C case
• The C++ case should only apply for TUK_Reference

[shivanshu3]

Thank you very much for the review @rsmith!

clang/lib/Sema/SemaDecl.cpp

15739–15745

I added a check for TUK_Reference. But I don't fully understand your other comment about checking the qualifiers on the typedef type. Could you please explain a little more?

clang/test/Sema/enum-typedef-msvc.c

16

Oh interesting! I didn't realize MSVC produces an error in that case. Clang does not produce an error with these changes in that case. Is that OK?

[shivanshu3]

Previously for some reason I thought that MSVC only allowed this behavior for anonymous enums. But now I realize that the enum doesn't have to be anonymous. So the following code should be legal too with '-fms-compatibility':

typedef enum FooEnum {
  A
} Foo;

void func() {
  enum Foo foo;
}

[shivanshu3]

clang/lib/Sema/SemaDecl.cpp

15739–15745

@rsmith Since my newest iteration removes the restriction for the enum to be anonymous, I'm guessing I can mark this comment as resolved?

[shivanshu3]

Ping!

[rsmith]

@rnk Your thoughts on this would be appreciated.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

15739–15745

Regarding qualifiers:

enum A { e };
typedef const A B;
void f() {
    enum ::B b = e; // OK?
    b = e; // OK?
}

MSVC's behavior here is very strange -- they behave exactly as if the enum keyword were not present. The first commented line above is accepted, and the second is rejected because we're assigning to an object of type const A.

If we want to be compatible with this kind of thing, then I think we need a larger-scale restructuring: ActOnTag would need a way to say "no, wait, this isn't a tag at all, use this non-tag type instead". I don't know how common this kind of code is, but perhaps we could instead reject the first line above (because the typedef isn't for *exactly* the enum type).

[rnk]

In D91659#2458639, @rsmith wrote:

@rnk Your thoughts on this would be appreciated.

I think it's important to make MIDL happy if we can. As to how to restructure the code to get there, I took a look, but wasn't able to come up with any helpful suggestions without putting a lot more time into this.

What is the issue with the current structure, ActOnTag produces the wrong AST node, an ElaboratedType, but we want to produce a TypedefType?

[rsmith]

In D91659#2458872, @rnk wrote:

In D91659#2458639, @rsmith wrote:

What is the issue with the current structure, ActOnTag produces the wrong AST node, an ElaboratedType, but we want to produce a TypedefType?

I don't think we want to introduce a new declaration at all in this compatibility case; I think we want to act as if the enum keyword were omitted entirely (the caller should set a different kind of TypeSpecType on the DeclSpec in this case). Maybe the simplest thing would be to add a new optional TypeResult* out parameter to ActOnTag that's only set by the call in ParseEnumSpecifier (and then only for the TUK_Reference case). Then ParseEnumSpecifier can set a TST_typename type specifier in this case instead of a TST_enum.

clang/lib/Sema/SemaDecl.cpp

if (Name && SS.isNotEmpty()) {
 
     // If this is a named struct, check to see if there was a previous forward
     // declaration or definition.
     // FIXME: We're looking into outer scopes here, even when we
     // shouldn't be. Doing so can result in ambiguities that we
     // shouldn't be diagnosing.
     LookupName(Previous, S);
 
+    // Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+    // Note that lookup only fails in C, not C++, so this if condition
+    // is only used for C code.
+    if (getLangOpts().MSVCCompat && Kind == TTK_Enum && Previous.empty() &&
+        TUK == TUK_Reference) {
+      LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+                                     Redecl);
+      LookupName(TypedefEnumLookup, S);
+
+      if (auto *TD = TypedefEnumLookup.getAsSingle<TypedefNameDecl>())
+        if (auto *TT = TD->getUnderlyingType()->getAs<TagType>())
+          if (auto *ED = dyn_cast_or_null<EnumDecl>(TT->getDecl()))
+            Previous.addDecl(ED);
+    }
+
     // When declaring or defining a tag, ignore ambiguities introduced
     // by types using'ed into this scope.
     if (Previous.isAmbiguous() &&
         (TUK == TUK_Definition || TUK == TUK_Declaration)) {
       LookupResult::Filter F = Previous.makeFilter();
       while (F.hasNext()) {
         NamedDecl *ND = F.next();
         if (!ND->getDeclContext()->getRedeclContext()->Equals(
                 SearchDC->getRedeclContext()))
           F.erase();
       }
       F.done();
     }
 
     // C++11 [namespace.memdef]p3:
     //   If the name in a friend declaration is neither qualified nor
     //   a template-id and the declaration is a function or an
     //   elaborated-type-specifier, the lookup to determine whether
     //   the entity has been previously declared shall not consider
     //   any scopes outside the innermost enclosing namespace.
if (!Previous.empty()) {
     // This is also okay for elaborated-type-specifiers, which is
     // technically forbidden by the current standard but which is
     // okay according to the likely resolution of an open issue;
     // see http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#407
     if (getLangOpts().CPlusPlus) {
       if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(PrevDecl)) {
         if (const TagType *TT = TD->getUnderlyingType()->getAs<TagType>()) {
           TagDecl *Tag = TT->getDecl();
-          if (Tag->getDeclName() == Name &&
-              Tag->getDeclContext()->getRedeclContext()
-                          ->Equals(TD->getDeclContext()->getRedeclContext())) {
+          bool AllowLookupUsingTypedefName = getLangOpts().MSVCCompat &&
+                                             TUK == TUK_Reference &&
+                                             Kind == TTK_Enum;
+          if ((Tag->getDeclName() == Name || AllowLookupUsingTypedefName) &&
+              Tag->getDeclContext()->getRedeclContext()->Equals(
+                  TD->getDeclContext()->getRedeclContext())) {
             PrevDecl = Tag;
             Previous.clear();
             Previous.addDecl(Tag);
             Previous.resolveKind();
           }
         }
       }
     }
 

clang/test/Sema/enum-typedef-msvc.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT %s
+
+typedef enum FooEnum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} // expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
+
+#ifdef USE_MSVC_COMPAT
+enum Foo foo2; // expected-no-diagnostics
+#else
+enum Foo foo2;  // expected-error {{tentative definition has type 'enum Foo' that is never completed}}
+                // expected-note@-1 {{forward declaration of 'enum Foo'}}
+#endif

clang/test/Sema/enum-typedef-msvc.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum FooEnum {
+  A,
+} Foo;
+
+class AMyInterface {
+  virtual void MyFunc(enum Foo *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum Foo *param) override {}
+};
+
+enum Foo myEnum;
+
+// expected-no-diagnostics