Pitch: Improved ClangImporter Diagnostics

Hi everyone,

We would like to propose improvements to the ClangImporter to provide more detailed diagnostics when an entity cannot be imported from C or Objective-C. As it stands, when the ClangImporter fails to import an entity (function, type, macro, etc.) either entirely or partially, failure is largely silent. The current Swift compilation errors present largely as if the entity was never defined at all.

We are proposing adding two different kinds of signals to provide more context to the users:

  • Warnings for the different constructs from headers that fail to import.
  • Errors for when those constructs are used in Swift code by generating stubs marked unavailable in place of the not imported entities.

For the time being, we propose to hide this functionality behind a new opt-in flag.

Proof of Concept

Here GitHub - NuriAmari/swift at diagnostics-pitch we have prepared a small proof of concept that centers around a couple of builtin C types with no Swift representation. As it stands, if such a type is used in a C function signature, the function will not be imported by the ClangImporter. Similarly, if an unsupported type is used in a field declaration, the ClangImporter will import the struct, but it will appear as if the field was never declared.

Demonstration

Below we present a minimal example demonstrating the improvements provided by the proof of concept and the kinds of improvements we would like to continue making. Here are the files involved, all in the same directory.

module.map

module complex {
    header "complex.h"
    export *
}

complex.h

struct MyStruct {
float _Complex x;
int y;
};
double _Complex myFunc();

demo.swift

import complex
let _ = myFunc()

demo2.swift

import complex
var s : MyStruct
s.x = 5.0

For the first demonstration, we show the current behaviour before enabling the new diagnostics:

swift-frontend -typecheck -I . demo.swift
demo.swift:3:9: error: cannot find 'myFunc' in scope
let _ = myFunc();
^~~~~~

As mentioned, the function simply can’t be accessed, though the user hasn’t really made any mistakes. After diagnostics are enabled, we get more helpful information:

swift-frontend -typecheck -enable-experimental-clang-importer-diagnostics -I . demo.swift
/Users/nuriamari/git/swift-project/demo/complex.h:6:1: warning: function 'myFunc' not imported: type 'Complex' is unavailable in Swift
double _Complex myFunc();
^
demo.swift:3:9: error: 'myFunc()' is unavailable: type 'Complex' is unavailable in Swift
let _ = myFunc();
^~~~~~
complex.myFunc:2:13: note: 'myFunc()' has been explicitly marked unavailable here
public func myFunc() -> Any
^

Though not shown in this example, the proof of concept also handles invalid parameter types, not just return types.

We provide similar improvements for struct fields:

swift-frontend -typecheck -I . demo2.swift
demo2.swift:4:3: error: value of type 'MyStruct' has no member 'x'
s.x = 5.0
~ ^

Again, the user is left wondering what happened to the x field. After enabling diagnostics, the situation is a little more clear:

swift-frontend -typecheck -enable-experimental-clang-importer-diagnostics -I . demo2.swift
/Users/nuriamari/git/swift-project/demo/complex.h:2:20: warning: field 'x' not imported: type 'Complex' is unavailable in Swift
float _Complex x;
^
demo2.swift:4:3: error: 'x' is unavailable: 'Complex' is unavailable in Swift
s.x = 5.0
^
complex.MyStruct:3:16: note: 'x' has been explicitly marked unavailable here
public var x: Any
^

Admittedly, these examples are quite niche, but they serve to communicate the type of improvements we wish to make.

General Design

The proof of concept above contains two main flows that we propose be repeated where needed to add support for more diagnostics. A pull request has been opened here to highlight the changes: Clang Importer Diagnostics Pitch by NuriAmari · Pull Request #39655 · apple/swift · GitHub.

Diagnostic Reporting

The first flow is responsible for recording diagnostics and surfacing them to the correct code locations. Many methods within the ClangImporter are used to transform a particular Clang AST node and return a pointer to the corresponding Swift AST Node. In the case of import failure, nullptr is returned, leaving little room for communicating diagnostics. In the proof of concept, https://github.com/NuriAmari/swift/blob/diagnostics-pitch/lib/ClangImporter/ImporterImpl.h#L322 we have created a new wrapper structure to include both the Swift AST Node and any diagnostics that may have been produced. We have applied this new return type in a few locations, but would like feedback on the approach as a whole.

We have observed that other additional return values are communicated using by-ref parameters, for example when importing a method the async convention or the error type are returned this way. This option was briefly explored, but found it to be less appealing.

Stub Generation

To construct stubs for unimported components, we have tried as much as possible to reuse the existing AST generation code. As shown in the proof of concept swift/ImportType.cpp at diagnostics-pitch · NuriAmari/swift · GitHub when a small portion of the AST fails to import, we attempt to replace it with a version using Any as early as possible. For example, if a function fails to import due to an unsupported return type, we replace the return type with Any and allow the function generation to proceed as normal otherwise. Before the enclosing AST node is finalized, we check to see if diagnostics were reported, even for a “successful” import, and mark the node unavailable accordingly. We recognize some care must be taken in ensuring that such a replacement does not result in an erroneously imported unsupported entity, but we think reusing the existing code is worth this risk.

Diagnostics + Repeating Name Lookups

While attempting to understand the ClangImporter and how diagnostics are reported, we have come to realize that declaration lookups are often performed repeatedly in a series of increasingly broad searches: swift/PreCheckExpr.cpp at main · apple/swift · GitHub . The extra calls into the ClangImporter result in diagnostics (both new and existing) to be reported either repeatedly or at inappropriate times, particularly for the broader searches (ie. typo search). Thus far, we’ve been able to avoid additional lookups by providing a stub on the first search, but in our investigation of C macros, this hasn’t worked as well. If anyone has experience with this issue, or any suggestions as to a fix, we would appreciate any input.

Feedback / Future Direction

We would appreciate any feedback from those more familiar with the ClangImporter on our general approach. In particular, input from @zoecarver, @egor.zhdan (active in the Clang Importer code), @jrose (listed as the owner of the Clang Importer), and @michael_ilseman, @Douglas_Gregor (worked in the diagnostic code of the Clang Importer) would be wonderful. Also, feel free to bring anyone you think might be interested into the discussion. If the patterns presented in the proof of concept are reasonable, we plan to expand to other hopefully more impactful unimported C/Objective-C constructs. In the immediate future, we plan to tackle the following C constructs:

  • Macros. Only mostly trivial macros are supported currently, which sometimes creates problems importing some headers that rely on macro expansion to work.
  • Forward declared classes. Clang Importer supports some cases of forward declared classes, but when the forward declared class is not defined in the same module, functionality that uses it is not imported. Even importing both modules from Swift will not solve the problem.

That being said, we are actively seeking out more silently unavailable constructs that could use diagnostics. If anyone is aware of more examples, we would greatly appreciate any input and think compilation of such a list would be beneficial, even if we are not able to address every issue.

In some cases, it is complicated to distinguish between what is a bug in the Clang Importer and what is a limitation of the translation mechanisms. We are looking for the latter. If you have examples of the former, we encourage you to report them to https://bugs.swift.org.

11 Likes

Oops, I definitely shouldn’t be listed as the owner anymore! For now, I’ll just say that there’s currently no way for the importer to know what code belongs to you and what’s in someone else’s C library that you can’t change, and that’s pretty important for a diagnostic like this. So the opt-in mode is nice for running things one-off, but it’s not something we’d ever want to turn on by default. In order to do that, I’d suggest:

  • Providing a way to indicate which modules or headers are important. The simplest answer is “just the underlying module for a mixed-source framework”, but that doesn’t cover SwiftPM, which doesn’t allow mixed-source frameworks, or bridging headers for apps, which let you mix your own headers with arbitrary other headers that might not belong to you.

  • Providing a way to silence the warning on a per-decl basis in your own code.

But it’s not necessary to do those things just to have this available in the compiler. I just want to bring them up early enough that we don’t turn this on and then get dozens of bug reports about unsilenceable warnings.

4 Likes

Improvements in this area are very welcome; it's been a frequent pain point for us at Google since our Objective-C code is broken up into many small modules and they often use forward declarations instead of #imports to speed up compile time.

How about separating the warnings from the improved error diagnostics? I agree with @jrose that having the warnings visible on every import would be too much noise, but surfacing that same information (as you've done) in a note associated with the error if someone tries to use the decl is still a huge improvement over the current situation where the decl is just silently dropped.

As an alternative for the warnings, a good place where I think the information could be surfaced would be in the SourceKit interface generation request for the Clang module. When you render the Swift interface for the C/Objective-C decls, you could still include the ones that were unavailable, with the appropriate @available attributes that include the message why, so they would be visible to anyone viewing that in Xcode or some other tool that generates it for the module.

4 Likes

Lovely to see work in this area and agree with feedback above.

A more minor point is that this note is (to me) actively misleading in this context: "explicitly marked unavailable" strongly suggests intentionality on the part of an author. However, what's failed to import has been marked unavailable by an automated tool--indeed, the point of the improved diagnostics is to surface something that no human might have intended.

4 Likes

Thanks for your feedback. We might have focus a little bit on first party code, but we will definitely keep system and third party code in mind while we keep working on this. We imagine that the modules marked system should be easy to opt-out automatically from all these warnings, but we will keep an eye. We might need to keep this as strictly opt-in, which while not ideal, it is for sure less noisy.

We will have to look at this. We are still learning how the Clang Importer works, and figuring out the best way to approach this. Investigating how to attach those notes to the unavailability warning might be a good investigation path. Thanks!

We weren't planning in looking at SourceKit, but we did not know that it had that functionality. This can be something interesting for the future. Thanks for mentioning it.

We can look into the messaging and see if it can be improved. We are using the same code as it was already used by the functions that have variable arguments and others (swift/ImportDecl.cpp at c2fd49cebb37a7851c71f27d8aedbea136bf0614 · apple/swift · GitHub) which attaches an unavailable attribute to the node (I think the message comes from swift/DiagnosticsSema.def at e845af8fc2d8056b6ee5f296bd028396139b44f5 · apple/swift · GitHub).

Terms of Service

Privacy Policy

Cookie Policy