[ClangImporter] Import macros with cast to builtin type

[IngmarStein]

What's in this pull request?

This PR builds on #2538 by adding support for type casts to some builtin types (mostly those consisting of a single token, e.g. 'short', 'long', etc.).

---

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

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.

[jrose-apple]

Looks pretty good to me!

@swift-ci Please test

[swiftix]

@IngmarStein Hi, I discovered that some of the imported macros from Foundation cannot be properly type checked. For example:

#define MACH_VOUCHER_NAME_ARRAY_NULL ((mach_voucher_name_array_t) 0)
#define KAUTH_FILESEC_WANTED    ((kauth_filesec_t)1)

I discussed it with @jrose-apple and it looks like the desired behavior should be:

1. do not import macros if they cannot be type checked
2. extend casts support to support casting to non-builtin types. May be UnsafePointer.init(bitPattern:) could be used to represent such casted values.

Would you be interested in trying to implement this?

[jrose-apple]

To be clear, we just want (1) for now. (2) can come later.

[IngmarStein]

I'll have a look at the typechecker and see what I can do.

In the meantime, I suggest the following patch:

diff --git a/lib/ClangImporter/ImportMacro.cpp b/lib/ClangImporter/ImportMacro.cpp
index 97a23b5..b71fdab 100644
--- a/lib/ClangImporter/ImportMacro.cpp
+++ b/lib/ClangImporter/ImportMacro.cpp
@@ -346,11 +346,16 @@ static ValueDecl *importMacro(ClangImporter::Implementation &impl,
       } else {
         return nullptr;
       }
+      if (!castType->getTypePtr()->isBuiltinType() && !castTypeIsId) {
+        return nullptr;
+      }
     } else {
       auto builtinType = builtinTypeForToken(tokenI[1],
                                              impl.getClangASTContext());
       if (builtinType) {
         castType = &builtinType.getValue();
+      } else {
+        return nullptr;
       }
     }
     tokenI += 3;

I didn't want to go so far as to import constants which are used as special pointer addresses such as (void *)1. With the patch above, we would restrict the importer to only consider types whose canonical type is a builtin type.

[jrose-apple]

Just a note: you can write the above as just castType->isBuiltinType().

id should already be going down a separate path for nil, right? Anything where we use nil should be okay here, including void *. (But if we don't get that right, then yes, this looks OK.)

[IngmarStein]

castType is a clang::QualType, so I can't write this as castType->isBuiltinType():

error: no member named 'isBuiltinType' in 'clang::QualType'
      if (!castType->isBuiltinType() && !castTypeIsId) {
           ~~~~~~~~  ^
1 error generated.

The path for id follows if numTokens == 1, so we have to make sure not to bail out early if the cast type is not a builtin type.

I'll create a new PR for the patch. Thanks for the review!

[jrose-apple]

clang::QualType has an operator->, so I'm a little confused why this wouldn't work.