Page MenuHomePhabricator

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

Authored by shivanshu3 on Nov 17 2020, 2:09 PM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
430 msx64 windows > lld.MachO::thin-archive.s
Script: -- : 'RUN: at line 3'; split-file C:\ws\w1\llvm-project\premerge-checks\lld\test\MachO\thin-archive.s C:\ws\w1\llvm-project\premerge-checks\build\tools\lld\test\MachO\Output\thin-archive.s.tmp

Event Timeline

shivanshu3 created this revision.Nov 17 2020, 2:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2020, 2:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
shivanshu3 requested review of this revision.Nov 17 2020, 2:09 PM
rsmith added a subscriber: rsmith.Nov 17 2020, 5:15 PM
rsmith added inline comments.
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 added a reviewer: rsmith.

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 marked 3 inline comments as done.Nov 17 2020, 9:52 PM

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 updated this revision to Diff 308803.Dec 1 2020, 3:55 PM
shivanshu3 retitled this revision from Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode to Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode.
shivanshu3 edited the summary of this revision. (Show Details)

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 added inline comments.Dec 1 2020, 3:58 PM
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?

@rnk Your thoughts on this would be appreciated.

rsmith added inline comments.Dec 16 2020, 11:58 AM
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 added a comment.Dec 16 2020, 2:11 PM

@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?

In D91659#2458872, @rnk 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.