[PrintAsObjC] print __attribute__((noescape)) in Obj-C headers

[jtbandes]

Print out (A) -> B as SWIFT_NOESCAPE B ([*^] «nullability»)(A) if it's the type of a function param, unless marked with @escaping.

Resolves SR-2406 filed by @jrose-apple.

---

Before merging this pull request to apple/swift repository:

• Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

1. Builds the compiler incrementally.
2. Builds the standard library only for macOS. Simulator standard libraries and
   device standard libraries are not built.
3. lldb is not built.
4. The test and validation-test targets are run only for macOS. The optimized
   version of these tests are not run.

A smoke test on Linux does the following:

1. Builds the compiler incrementally.
2. Builds the standard library incrementally.
3. lldb is built incrementally.
4. The swift test and validation-test targets are run. The optimized version of these
   tests are not run.
5. lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

[jtbandes]

@jrose-apple — based on CODE_OWNERS I think you're probably the best one to review this, too. 🙇

[jrose-apple]

I'm not happy with adding the extra "is parameter position" flag, but maybe it's unavoidable. What about typealiases, though?

[jrose-apple]

It's probably also good to have a test for nested @escaping.

[jrose-apple]

(And thank you for working on this!)

[jtbandes]

What about typealiases? I can't test right now, but I imagine they're not allowed to be noescape. Only when used in a function param would it have any meaning, so you'd write func foo(x: @escaping MyFuncTy). I guess I'd better make sure such a case is in the tests.

[jrose-apple]

Yeah, that's all I meant. Both Swift typealiases and imported Objective-C typedefs now allow @escaping, which means the lack of @escaping should produce a noescape.

Maybe instead of passing down the "are we in parameter position" flag, the logic that iterates over parameters can handle it? That does mean putting the logic in two places (once for function/block types, once for method parameters) but that's not the end of the world.

[jrose-apple]

(and would handle typealiases and other alternate spellings automatically)

[jtbandes]

That's kinda how it's already done: the logic iterating over function parameters and method parameters are the only places where IsFunctionParam is passed in; everywhere else uses IsNotFunctionParam or passes down the value it was given.

It's messy because in printing out param types, the code just calls print() from the top, so I had to thread this new IsFunctionParam_t through the whole visitor pattern. If you can think of a better alternative, let me know. In a little bit I'll add a test for typealiases.

[jtbandes]

(It would've been nicer if TypeVisitor could be given default values for its additional args, but I don't think that's supported.)

[jtbandes]

Friendly bump 😄

[jrose-apple]

I was remembering that it was okay to put __attribute__((noescape)) before the type as well (__attribute__((noescape)) int(*)(void)), and it turns out that's correct. That means the information wouldn't have to be passed down through the type printer; you could just do this:

if (auto fnTy = argTy->getAs<AnyFunctionType>())
  if (fnTy->getExtInfo().isNoEscape())
    print("SWIFT_NOESCAPE ");

I feel like that's cleaner, but what do you think?

[jtbandes]

Hmm, that's interesting, and would certainly be less code change. But would
getAs properly look through typealiases, paren types, etc.?
On Fri, Aug 26, 2016 at 9:45 AM Jordan Rose [email protected]
wrote:

I was remembering that it was okay to put attribute((noescape))
before the type as well (attribute((noescape)) int(*)(void)), and
it turns out that's correct. That means the information wouldn't have to be
passed down through the type printer; you could just do this:

if (auto fnTy = argTy->getAs())
if (fnTy->getExtInfo().isNoEscape())
print("SWIFT_NOESCAPE ");

I feel like that's cleaner, but what do you think?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4438 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAA3nX14RWs5FZLr_EnZLl8grjipinzUks5qjxgkgaJpZM4JpJTF
.

[jrose-apple]

Ah, yep, sorry. getAs will go all the way to a canonical type like that unless the type kind you are trying to getAs is itself a "sugar" type. So it should be fine.

[jtbandes]

OK, thanks for the tip. I'll update the PR tonight.

[jtbandes]

Thanks a lot for the feedback. I'm much happier with this version.

@swift-ci please smoke test

[jtbandes]

missed changing one file to the new style. @swift-ci please smoke test

[jrose-apple]

Thanks! One more thing to test, unless I missed it: an imported typedef for a block. That should come out as SWIFT_NOESCAPE TheBlockType.

Oh, and what do you think about using a two-element enum instead of a bool for "isFunctionParameter"? Then we don't need the comments.

[jtbandes]

It looks like typealiases are just fully expanded right now: https://github.com/apple/swift/pull/4438/files#diff-ab50a31c33e58d3c979a7680a7dec033R92 Is that unexpected?

[jrose-apple]

Swift typealiases are fully expanded, but those defined in Objective-C shouldn't be.

[jtbandes]

@jrose-apple Hmm, actually this is broken right now:
https://gist.github.com/jtbandes/8820a8fab7e0ed9668247ae3ba06b026#file-output-h-L1-L5

Mostly, I think, due to an existing limitation https://bugs.swift.org/browse/SR-2520 which @slavapestov mentioned in a FIXME

[jtbandes]

Also, it looks like SwiftDeclConverter::VisitTypedefNameDecl doesn't even try to respect the noescape attribute. The conversion is passed down to SwiftTypeConverter::VisitFunctionProtoType, which doesn't have access to the parameter decls (just the types) so it doesn't know whether they're supposed to be noescape. So making this actually work for imported typedefs seems to be a tall order.

[jtbandes]

I suppose the possible courses of action are:

• Discuss how to change SwiftDeclConverter/SwiftTypeConverter to handle noescape attributes in typedefs properly (from my reading of the code, this seems like it'd require either an architectural change or a hack, but I might be wrong)
• Put in a passing test with a FIXME
• Put in an XFAIL test (checking against the desired output) with a FIXME
• Back out the whole change, if it's not worth having a partial solution

Thoughts?

[jrose-apple]

If I'm understanding you correctly, I think you've found two different issues here:

• @escaping MyBlockTy loses sugar, so PrintAsObjC prints it as a block type instead of a typedef.
• typedef void (^MyBlockTy)(__attribute__((noescape)) void (^callback)(void)) doesn't preserve the noescape on the callback parameter.

For the first issue, I think a passing test with a FIXME is good enough, as long as we test imported typedefs with and without @escaping. I'm a little surprised by the second one, because the escaping-ness is part of the type, but maybe we didn't model it that way in Clang? Or maybe I'm misunderstanding the issue.

[jtbandes]

Yeah, that's accurate, at least as far as I could figure out reading the code last night.

Here's where typedef decls are imported; it calls getUnderlyingType() and imports that type:
https://github.com/apple/swift/blob/1a88c86f39b2552fbef5e05ea8ba95d3161e23b7/lib/ClangImporter/ImportDecl.cpp#L1685-L1690

To import the block pointer type it just imports the underlying function pointer:
https://github.com/apple/swift/blob/1a88c86f39b2552fbef5e05ea8ba95d3161e23b7/lib/ClangImporter/ImportType.cpp#L379-L382

Then, here's where parameters of function pointer types are imported:
https://github.com/apple/swift/blob/1a88c86f39b2552fbef5e05ea8ba95d3161e23b7/lib/ClangImporter/ImportType.cpp#L482-L496

At this point it seems like the noescape information is not available. Other places which use it are checking hasAttr<clang::NoEscapeAttr>() on a param decl, but this only has access to the param types.

[jrose-apple]

Okay, so I guess for now we should be treating all parameters in imported block typedefs as escaping.

[jtbandes]

OK, I've added a new test file to capture this, with FIXMEs and notes about what I think the real expected output should be, once the issues get fixed. #gloriousfuture 🌈

(I filed SR-2529 and SR-2520 to track them, and linked to these from the source code / tests.)

I also found that I wasn't handling optional params properly, so I added a lookThroughAllAnyOptionalTypes() call when deciding to print SWIFT_NOESCAPE. Of course, now I'm worried that more cases might be missing — do you think lookThroughAllAnyOptionalTypes()->getAs<AnyFunctionType>() will really cover all the bases?

@swift-ci please smoke test

[jtbandes]

@shahmishal Is this a known issue? The OS X smoke test seemed to start and immediately fail with --- EXIT WRONG SHA ---. But then it started itself again. https://ci.swift.org/job/swift-PR-osx-smoke-test/1445/ https://ci.swift.org/job/swift-PR-osx-smoke-test/1446/

[shahmishal]

@jtbandes yes this is known issue. It re-triggers with the new and correct sha.

[jtbandes]

Not sure what I'm doing wrong here. For some reason, other test files with practically the same lit directives aren't generating this @import.

fatal error: module '__ObjC' not found
@import __ObjC;
 ~~~~~~~^~~~~~

[jtbandes]

Found it; I was missing -import-objc-header %S/../Inputs/empty.h. Not sure what that's for, but I'll take it. 😄 @swift-ci please smoke test

[jtbandes]

@swift-ci smoke test linux

[jrose-apple]

I think we're good. Thanks, Jacob!

[jtbandes]

Thanks for all your help & patience! 😃