[SR-6706] Swift should complain about weak references to classes that don't support them #17792
Conversation
lib/Sema/TypeCheckAttr.cpp
Outdated
| ClassDecl *underlyingClass = underlyingType->getClassOrBoundGenericClass(); | ||
| while (underlyingClass != nullptr) { | ||
| if (auto clangDecl = underlyingClass->getClangDecl()) { | ||
| if (auto underlyingObjCDecl = cast<clang::ObjCInterfaceDecl>(clangDecl)) { |
There was a problem hiding this comment.
Can this cast ever fail, eg if you have a CF type or DispatchQueue or something?
There was a problem hiding this comment.
Ah, I didn't think of that. Looks like it would fail then. My original intention was to use dyn_cast just in case some new language bridge was added (C++? lol). Thanks.
About CF types & Dispatch objects, isArcWeakrefUnavailable() is only defined on clang::ObjCInterfaceDecl so I assume the attribute it tests for cannot be added to anything else--or at least Clang doesn't support it at the moment.
ChristopherRogers
force-pushed
the
master
branch
2 times, most recently
from
dbdc991
to
b374827
Compare
|
@ChristopherRogers talked to me about this at try! Swift San Jose today. Sorry for losing the PR for almost a year! To avoid having Sema reason about Clang declarations, I suggested adding a bit to ClassDecl to track this, which ClangImporter can set and Sema can check. |
|
Some notes:
I think I've addressed everything. Let me know if you have any other concerns or nitpicks. Thank you for your help yesterday! |
There was a problem hiding this comment.
Thanks, @ChristopherRogers! I still think it'd be a good idea to check an Objective-C subclass of NoWeakClass as well, but this looks good regardless.
| /// references. | ||
| /// | ||
| /// Note that this is true if this class or any of its ancestor classes | ||
| /// are marked incompatible. |
There was a problem hiding this comment.
Nice, this does make sense to package up in the accessor.
|
@swift-ci Please test |
|
And while I don't see how it could cause a problem, let's make sure… @swift-ci Please test source compatibility |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ChristopherRogers
force-pushed
the
master
branch
from
31cbb20
to
db6f15e
Compare
|
I had forgotten to require objc_interop for the test file so it was failing on Linux. The macOS compatibility suite had some error with a ClockKit module not being found, but the previous build job had the same error as well. |
|
@swift-ci Please test |
|
I think this is still going to fail but I want to at least see the proper failures. @swift-ci Please test source compatibility |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Source compat failures match those on master. Thanks, @ChristopherRogers! |
This pull requests adds diagnostics similar to Clang when attempting to create weak or unowned references of types that do not support them (NSFont, NSWindow, etc.). This is indicated with the
objc_arc_weak_reference_unavailableClang attribute.I thought about limiting this to only run on macOS since in practice this only affects macOS targets. (The only classes affected are from the macOS SDK). I've left it for completion's sake since someone could add the attribute to their own Objective-C class if they so choose..
Resolves SR-6706.