Page MenuHomePhabricator

Sema: introduce `__attribute__((__swift_name__))`
ClosedPublic

Authored by compnerd on Sep 11 2020, 11:05 AM.

Details

Summary

This introduces the new swift_name attribute that allows annotating
interfaces with an alternate spelling for Swift. This is used as part
of the importing mechanism to allow interfaces to be imported with a new
name into Swift. It takes a parameter which is the Swift function name.
This parameter is validated to check if it matches the possible
transformed signature in Swift.

Diff Detail

Unit TestsFailed

TimeTest
80 mswindows > LLVM.Other::change-printer.ll
Script: -- : 'RUN: at line 6'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\opt.exe -S -print-changed -passes=instsimplify 2>&1 -o /dev/null < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll --check-prefix=CHECK_SIMPLE

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2020, 11:05 AM
compnerd requested review of this revision.Sep 11 2020, 11:05 AM
compnerd added inline comments.Sep 11 2020, 1:12 PM
clang/include/clang/Basic/Attr.td
2173

Note for @rjmccall and @doug.gregor - this version actually enumerates the subjects to which this attribute appertains unlike what was in the original swift version. Are there other subjects which this should list?

aaron.ballman added inline comments.Sep 14 2020, 5:51 AM
clang/include/clang/Basic/Attr.td
2174

I don't see anything adding ExpectedSwiftNameSubjects to the list of diagnostic enumerations, are you sure this is needed?

clang/include/clang/Basic/DiagnosticSemaKinds.td
3985

I think several of these can be consolidated into a single diagnostic with %select:

%0 attribute has invalid identifier for %select{context name|base name|parameter name}1

clang/include/clang/Sema/Sema.h
1839

The docs should probably mention what AL is used for.

clang/lib/Sema/SemaDeclAttr.cpp
4282

const auto *

4289

I think it would be more helpful if the diagnostic said why the attribute is being ignored (because the arguments don't match).

5658

FWIW, I'm not particularly qualified to review the logic here. Someone with more Swift experience should look at this bit.

5660

Set IsSingleParamInit as well (nothing sets it before the early return on line 5673)?

5804–5805

Do you have to worry about functions with ... variadic parameters and how those impact counts (for either ObjC or regular methods)?

clang/test/SemaObjC/attr-swift-name.m
1 ↗(On Diff #291277)

The fblocks makes me wonder -- should this attribute be one you can write on a block?

Thank you for doing this!

clang/include/clang/Basic/Attr.td
2173

Hmm. If we enumerate the subjects, we're going to have to update Clang whenever Swift's Clang importer learns a new trick. For example, this is probably missing CXXMethod and FunctionTmpl based on work that's going on in Swift. I suspect we're also missing ObjCCompatibilityAlias. I'm inclined to treat this more like AsmLabelAttr and not try to enumerate subjects at all.

compnerd added inline comments.Sep 15 2020, 1:52 PM
clang/include/clang/Basic/Attr.td
2173

That seems fair to me. I'll try to remove the subject list and try to clean up the fallout.

clang/test/SemaObjC/attr-swift-name.m
1 ↗(On Diff #291277)

This is not something that can be applied to blocks. Removing the -fblocks is probably a good idea since it isn't actually used. copy-paste failure on my part :-(.

compnerd marked 8 inline comments as done.Sep 16 2020, 8:43 AM
compnerd added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
4289

Does the note below not accomplish that?

5804–5805

No, they are currently not auto-imported AFAIK.

compnerd updated this revision to Diff 292253.Sep 16 2020, 9:29 AM
compnerd marked an inline comment as done.

Address feedback from @aaron.ballman

aaron.ballman added inline comments.Sep 16 2020, 10:55 AM
clang/include/clang/Basic/Attr.td
104

This change is no longer needed.

clang/include/clang/Basic/AttrDocs.td
3584

A code example showing proper usage would be appreciated.

clang/lib/Sema/SemaDeclAttr.cpp
4289

Not really, no. The warning diagnostic itself just says that the attribute is ignored, which is the effect but not the rationale. The note (which is easier for folks to ignore) says the attribute is conflicting, but conflicting with *what* (there could be a half dozen attributes on the same declaration, for instance).

5804–5805

Should such function signatures be rejected here though? (If so, can you add a test for that case as well as a test using a K&R C declaration like void f()?)

compnerd marked 3 inline comments as done.Sep 16 2020, 12:02 PM
compnerd added inline comments.
clang/include/clang/Basic/Attr.td
104

Ah, right, I'll move it to the swift_private change.

clang/lib/Sema/SemaDeclAttr.cpp
4289

Okay, I don't think that there is an existing warning, but I should be able to add one.

5804–5805

I'll add a test case demonstrating that, it is indeed missing.

compnerd updated this revision to Diff 292299.Sep 16 2020, 12:04 PM
compnerd marked 2 inline comments as done.

Address everything but warning

compnerd added inline comments.Sep 16 2020, 1:47 PM
clang/lib/Sema/SemaDeclAttr.cpp
4289

The existing diagnostic is actually more inline with the existing attribute handling for __attribute__((__optnone__)), __attribute__((__minsize__)), and __attribute__((__always_inline__)) which merge similarly.

compnerd updated this revision to Diff 292368.Sep 16 2020, 4:18 PM

Change diagnostics for conflicting swift_name, add a test case

compnerd updated this revision to Diff 292369.Sep 16 2020, 4:19 PM
compnerd marked 2 inline comments as done.
aaron.ballman added inline comments.Sep 17 2020, 6:44 AM
clang/include/clang/Basic/AttrDocs.td
3582

The parameter takes -> The argument is

clang/include/clang/Basic/DiagnosticSemaKinds.td
3977

I want to make sure we're clear with the terminology used in the diagnostics, so there's a fair number of "take -> have" suggestions here, but I want to be sure I'm correct before you apply those suggestions.

I'm used to function declarations having parameters which take arguments from a function call expression. Are the diagnostics about "taking a parameter" talking about "having a parameter" or about "taking an argument"? I believe the string arguments are function signatures rather than function names (perhaps?) and so we should be talking about having a parameter, but I wasn't 100% sure.

3979

How about: %0 attribute argument must be a string literal specifying a Swift function name?

3988

%0 attribute for 'subscript' must %select{be a getter or setter|have at least one parameter|have a 'self' parameter}1 ?

3994

take -> have

3997

take -> have

elsewhere new value is spelled 'newValue:, should that be the same here?

4006

take -> have

4009

take -> have

4012

take -> have

4015

take -> have

4017

I don't think you need this diagnostic -- you should be able to use warn_attribute_wrong_decl_type (it has a non-K&R-style functions option which is a bit more clear).

compnerd marked 11 inline comments as done.Sep 17 2020, 9:30 AM
compnerd added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3977

Discussed offline.

3997

newValue: is the argument label spelling and "new value" is the argument value, which is the reason for the difference.

compnerd updated this revision to Diff 292548.Sep 17 2020, 9:52 AM
compnerd marked 2 inline comments as done.

Address feedback

aaron.ballman accepted this revision.Sep 18 2020, 9:00 AM

LGTM, though you may want to get a second set of eyes on the swift name validation bits for a more thorough review of that part.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3997

That makes sense to me, thanks!

This revision is now accepted and ready to land.Sep 18 2020, 9:00 AM
gribozavr2 accepted this revision.Sep 18 2020, 2:10 PM

This introduces the new swift_name attribute that allows annotating
interfaces with an alternate spelling for Swift. This is used as part
of the importing mechanism to allow interfaces to be imported with a new

s/interfaces/APIs/ (to not confuse with Objective-C @interface)

clang/include/clang/Basic/AttrDocs.td
3579–3580

WDYT about:

The `swift_name` attribute specifies the name of the declaration in Swift. If this attribute is absent, the name is transformed according to the algorithm built into the Swift compiler.

3582

The argument is a string literal that contains the Swift name of the function, variable, or type. When renaming a function, the name may be a compound Swift name.

3593

Could we try a more realistic example?

@interface URL
- (void) initWithString:(NSString*)s __attribute__((__swift_name__("URL.init(_:)")))
@end

double __attribute__((__swift_name__("squareRoot()"))) sqrt(double) {
}
clang/include/clang/Basic/DiagnosticSemaKinds.td
3982

for => for the

3988

80 columns.

clang/include/clang/Sema/Sema.h
1837–1838

"a legal argument for the swift_name attribute applied to decl \p D."

1843–1844

WDYM by "the function will output"? Which function, DiagnoseSwiftName? I think this part of the comment is about validateSwiftFunctionName.

clang/lib/Sema/SemaDeclAttr.cpp
4282

Why is the variable called Inline? Unless there's a reason, I'd suggest something that has a stronger connection, like PreviousSNA.

5716

Could you add an assert that the character being dropped is indeed ')'? There is an error above, but it is sort of far away and the connection between that line and this one is not obvious.

compnerd marked 8 inline comments as done.Sep 21 2020, 3:01 PM
compnerd added inline comments.
clang/include/clang/Basic/AttrDocs.td
3579–3580

I think that is more approachable and less technical. I'll take it.

3593

I like that!

clang/include/clang/Sema/Sema.h
1843–1844

Ah, right, that is about validateSwiftFunctionName. I'll move the description.

clang/lib/Sema/SemaDeclAttr.cpp
4282

The previous one was declared inline in the declaration, but sure, I can rename it to PrevSNA.

This revision was landed with ongoing or failed builds.Sep 22 2020, 8:40 AM
This revision was automatically updated to reflect the committed changes.
compnerd marked 4 inline comments as done.