Page MenuHomePhabricator

Sema: add support for `__attribute__((__swift_newtype__))`
ClosedPublic

Authored by compnerd on Sep 14 2020, 4:19 PM.

Details

Summary

Add the swift_newtype attribute which allows a type definition to be
imported into Swift as a new type. The imported type must be either an
enumerated type (enum) or an object type (struct).

This is based on the work of the original changes in
https://github.com/llvm/llvm-project-staging/commit/8afaf3aad2af43cfedca7a24cd817848c4e95c0c

Diff Detail

Event Timeline

compnerd created this revision.Sep 14 2020, 4:19 PM
Herald added a project: Restricted Project. · View Herald Transcript
compnerd requested review of this revision.Sep 14 2020, 4:19 PM
aaron.ballman added inline comments.Sep 15 2020, 7:45 AM
clang/include/clang/Basic/Attr.td
2179

We don't ever document swift_wrapper, is that intentional?

(Do you have code elsewhere that's looking at the spelling of the AST attribute object? Should you add an AdditionalMembers to this declaration to make that easier to distinguish?)

2183

You should also set: let HasCustomParsing = 1; since this uses a custom parser.

Is the custom parser actually needed though? Or is it only needed because the enum names selected are keywords? If it's only because of the keyword nature, should the parser be improved in Parser::ParseAttributeArgsCommon() instead?

clang/include/clang/Basic/AttrDocs.td
3585

Code examples of how to use this properly would be appreciated.

clang/include/clang/Basic/DiagnosticSemaKinds.td
4038 ↗(On Diff #291725)

Hrm, we already have existing diagnostics to cover this (warn_attribute_wrong_decl_type_str) but it's in the IgnoredAttributes group. Do you need a new warning group specific to this? Is there a reason that group is not a subset of IgnoredAttributes?

clang/lib/Sema/SemaDeclAttr.cpp
5894

Btw, if you fix up the parser, then you can skip setting HasCustomParsing and then can remove some of this manual error checking as well.

5895

No need to do this (I'm surprised it even compiles given that AL is a const reference), here or elsewhere.

5905

There should be a helper method to do this dance for you. SwiftNewTypeAttr::ConvertStrToNewtypeKind() should be roughly what it's called (it's generated for you by the attr emitter).

clang/test/SemaObjC/attr-swift-newtype.m
2 ↗(On Diff #291725)

Can you split the AST bits out into their own test in the AST directory?

compnerd added inline comments.
clang/include/clang/Basic/Attr.td
2179

Yes, that was intentional. I believe that swift_wrapper predates swift_newtype and is only kept for compatibility. People should gravitate towards swift_newtype.

I don't understand the need for AdditionalMembers.

2183

It's for the keyword handling. I'll take a look and see if I can convert this to the ParseAttributeArgsCommon path.

clang/include/clang/Basic/DiagnosticSemaKinds.td
4038 ↗(On Diff #291725)

Hmm, Im okay with changing the group, but I do wonder if Apple is going to worry about diagnostic compatibility. @doug.gregor, @rjmccall, or @dexonsmith would be better suited to answer the question about command line compatibility.

aaron.ballman added inline comments.Sep 17 2020, 6:23 AM
clang/include/clang/Basic/Attr.td
2179

Ah, thank you for the explanation. I think we should document swift_wrapper as explicitly being deprecated then so it's clear to people (esp Future Aaron) what the intent is. It sounds like there's no need for AdditionalMembers (that's only useful if you need to distinguish between the spellings that share a semantic attribute).

2183

If you can easily modify ParseAttributeArgsCommon, then awesome! If that turns out to be a slog, I don't insist on the change being made.

clang/include/clang/Basic/DiagnosticSemaKinds.td
4038 ↗(On Diff #291725)

If you need specific control over the swift attribute diagnostics for ignored attributes, you could make the group be a part of IgnoredAttributes (so by default, swift ignored attributes are controlled by IgnoredAttributes but can be selectively enabled/disabled).

compnerd updated this revision to Diff 293574.Sep 22 2020, 3:06 PM
compnerd marked 6 inline comments as done.

Update for feedback from @aaron.ballman
The ParseAttrCommonArgs refactoring still needs to be looked at, but this should address the feedback, and doesn't need to be predicated on the refactoring.

aaron.ballman added inline comments.Sep 23 2020, 5:55 AM
clang/include/clang/Basic/Attr.td
2172

Should we make these docs part of the public docs in AttrDocs.td? That's sort of what I had in mind with my comment (so that if someone runs into the attribute in the wild and wonders what it is, they have a hint).

clang/lib/Sema/SemaDeclAttr.cpp
5968

Rather than adding a new enumeration to warn_attribute_wrong_decl_type, I would use warn_attribute_wrong_decl_type_str and pass in the string as part of the diagnostic.

compnerd added inline comments.Sep 23 2020, 8:13 AM
clang/include/clang/Basic/Attr.td
2172

Ah, okay, that seems reasonable enough.

compnerd marked 2 inline comments as done.Sep 23 2020, 8:57 AM
compnerd updated this revision to Diff 293763.Sep 23 2020, 9:01 AM
aaron.ballman accepted this revision.Sep 23 2020, 1:25 PM

The changes LGTM aside from a small wording nit with the documentation. The parser changes I asked about can be done in a follow-up to this patch.

clang/include/clang/Basic/AttrDocs.td
3604–3606

For the new blurb about the deprecated name, how about:

Previously, the attribute was spelled swift_wrapper. While the behavior of the attribute is identical with either spelling, swift_wrapper is deprecated, only exists for compatibility purposes, and should not be used in new code.

This revision is now accepted and ready to land.Sep 23 2020, 1:25 PM
compnerd marked an inline comment as done.Sep 23 2020, 1:46 PM
This revision was landed with ongoing or failed builds.Sep 24 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.