Diagnose non_runtime_protocol #77339

SharonXSharon · 2024-10-31T23:12:04Z

Diagnose on usage of ObjC protocols when their metadata are required, but the protocol actually have the non_runtime_protocol attribute, which removes the metadata on the ObjC side.

The patch is based on the discussion with John McCall and Manman Ren.

Diagnose on ObjC protocols which have the non_runtime_protocol attribute, when their metadata is actually required.

drodriguez · 2024-10-31T23:20:29Z

@swift-ci please test

drodriguez · 2024-10-31T23:48:23Z

@swift-ci please test macOS platform

Diagnose on ObjC protocols which have the non_runtime_protocol attribute, when their metadata is actually required.

drodriguez · 2024-11-01T18:06:19Z

@swift-ci please test

SharonXSharon · 2024-11-04T17:59:00Z

@rjmccall could you please take a look? Thanks!

tshortli · 2024-11-04T18:01:23Z

lib/Sema/TypeCheckAvailability.cpp

+ auto *proto = dyn_cast<clang::ObjCProtocolDecl>(ClangD);
+ if (!proto || !proto->isNonRuntimeProtocol())
+ return;
+ auto *clangImporter = static_cast<ClangImporter *>(D->getASTContext().getClangModuleLoader());


There are a few formatting issues in this function, please run clang-format

tshortli · 2024-11-04T18:06:20Z

include/swift/AST/DiagnosticsSema.def

+ " and its metadata is not available in Swift.",
+ (StringRef))
+
+NOTE(non_runtime_objc_protocol_metadata_not_available_note,none,


Can this new diagnostic string be avoided by reusing decl_declared_here instead?

Just checked , the decl_declared_here family are all using swift types, e.g. ValueDecl DeclName, https://github.com/swiftlang/swift/blob/main/include/swift/AST/DiagnosticsSema.def#L30

not clang decl types, like const clang::NamedDecl*, do you suggest I add a new decl_declared_here with the clang type, or just keep this non_runtime_objc_protocol_metadata_not_available_note?

Yes, I think it would be worth introducing a generic diagnostic string that's compatible with this use case, rather than creating a diagnostic with a very specific name but very generic text.

tshortli · 2024-11-04T18:09:17Z

include/swift/AST/DiagnosticsSema.def

+ "this Objective-C protocol '%0' has the non_runtime_protocol attribute"
+ " and its metadata is not available in Swift.",


Diagnostic style consistency suggestion:

Suggested change

"this Objective-C protocol '%0' has the non_runtime_protocol attribute"

" and its metadata is not available in Swift.",

"Objective-C protocol '%0' has the non_runtime_protocol attribute"

" and its metadata is not available in Swift",

tshortli · 2024-11-04T18:10:57Z

test/ClangImporter/availability_non_runtime_protocol_objc_protocol.swift

+}
+
+@objc class Wrapper : NSObject {
+ @objc public func runMutations(_ mutations: [ComposerMutationBridging]) { // expected-error {{this Objective-C protocol 'ComposerMutationBridging' has the non_runtime_protocol attribute and its metadata is not available in Swift.}}


Could you add more test cases covering additional positions that the protocol type could appear in?

Please also add test cases that we don't diagnose in places where we wouldn't need runtime support for it, like just as the type of a local variable.

tshortli · 2024-11-04T18:11:14Z

test/ClangImporter/availability_non_runtime_protocol_objc_protocol.swift

+// REQUIRES: objc_interop
+// REQUIRES: foundation
+
+// RUN: %target-swift-frontend -enable-objc-interop -typecheck -verify -import-objc-header %S/Inputs/availability_non_runtime_protocol_objc_protocol.h %s -emit-objc-header -emit-objc-header-path %t/SwiftCallObjC-Swift.h 


Why does this test use -emit-objc-header?

tshortli · 2024-11-04T18:12:19Z

test/ClangImporter/availability_non_runtime_protocol_objc_protocol.swift

+class AppDelegate {
+ @inline(never) func FOOBARwrapper() {
+ // CustomObject is defined in ObjC.
+ let instanceOfCustomObject = CustomObject()
+ instanceOfCustomObject.someProperty = "Hello World"
+ print(instanceOfCustomObject.someProperty)
+ instanceOfCustomObject.someMethod()
+ }
+}


This code looks like it might be superfluous to what is being tested in the Swift compiler here

tshortli · 2024-11-04T18:12:54Z

test/ClangImporter/Inputs/availability_non_runtime_protocol_objc_protocol.m

@@ -0,0 +1,23 @@
+#import "availability_non_runtime_protocol_objc_protocol.h"


Why is this .m file included in the test inputs?

Xazax-hun · 2024-11-18T11:49:47Z

lib/Sema/TypeCheckAvailability.cpp

+ Diags.diagnose(Loc, diag::non_runtime_objc_protocol_metadata_not_available,
+ D->getNameStr());
+ Diags
+ .diagnose(clangImporter->importSourceLocation(proto->getLocation()),


Do we need to go to the ClangDecl from D? I wonder if we actually construct the Swift declaration with the correct ObjC header locations. If that is not the case, I wonder if we should fix that instead of going through the ClangDecl here.

SharonXSharon · 2024-12-14T02:43:06Z

lib/Sema/TypeCheckAvailability.cpp

 if (Where.mustOnlyReferenceExportedDecls())
 TypeChecker::diagnoseDeclRefExportability(Loc, decl, Where);
+
+ diagnoseNonRuntimeProtocol(Loc, decl);


@rjmccall

Please also add test cases that we don't diagnose in places where we wouldn't need runtime support for it, like just as the type of a local variable.

I tried to add some test case as follows

public func foo(){ let _ : ObjCNonRuntimeProtocolUsedinSwift }

and found out it will also throw the newly added diagnostics for the test case, is my test case not correct, or is this not actually the right place to add the diagnostic?
When debugging, I found the above code would call visitNominalType which will call this visitTypeDecl and trigger the diagnostic.

Hmm, no, you want to trigger this from checkTypeMetadataAvailabilityInternal, I think. This place is much broader and will catch every use of the type.

@rjmccall IIUC any use of a non-runtime protocol internal to a module should be okay (if/because) the compiler can flatten the substitution map into the concrete type no? Would we be able to lower other use of NRP to AnyObject after type-checking to use the objc runtime instead of Swift's protocol witnesses?

ObjC protocols (NRP or not) always use the ObjC runtime, so yes, any uses as a value should be fine. That's not "any use ... internal to a module", though; it would not be correct to erase NRP from type uses of the protocol, and Swift does not reliably specialize generic code in ways that would eliminate all uses of the metadata. If you need an Array<any NRP>, you can (and must) wrap the any NRP in a struct so that the metadata of the NRP itself is not required.

rjmccall · 2024-12-18T20:58:27Z

lib/Sema/TypeCheckAvailability.cpp

 if (Where.mustOnlyReferenceExportedDecls())
 TypeChecker::diagnoseDeclRefExportability(Loc, decl, Where);
+
+ diagnoseNonRuntimeProtocol(Loc, decl);


Hmm, no, you want to trigger this from checkTypeMetadataAvailabilityInternal, I think. This place is much broader and will catch every use of the type.

SharonXSharon · 2024-12-20T23:00:00Z

test/ClangImporter/availability_non_runtime_protocol_objc_protocol.swift

+
+ public func iterate(collection mutations: some Collection<any ObjCNonRuntimeProtocolUsedinSwift>) { 
+ for m in mutations {
+ print(typeOf(anyNRP: m)) 


@rjmccall I moved the check to checkTypeMetadataAvailabilityInternal, but I am still a bit confused with what scenarios we should throw the diagnostics, e.g. is it expected we don't need metadata for this call at line 26, while we need metadata for the next call in line 27? Or are we missing this case in checkTypeMetadataAvailabilityInternal

Hi @rjmccall just checking in that do you have any suggestions how should I proceed with this patch?

Diagnose non_runtime_protocol

0b2b5a5

Diagnose on ObjC protocols which have the non_runtime_protocol attribute, when their metadata is actually required.

Diagnose non_runtime_protocol

61a7584

Diagnose on ObjC protocols which have the non_runtime_protocol attribute, when their metadata is actually required.

SharonXSharon marked this pull request as ready for review November 4, 2024 17:58

SharonXSharon requested review from Xazax-hun, beccadax, egorzhdan, hborla, hyp, ian-twilightcoder, slavapestov, tshortli, xedin and zoecarver as code owners November 4, 2024 17:58

tshortli requested changes Nov 4, 2024

View reviewed changes

Xazax-hun reviewed Nov 18, 2024

View reviewed changes

Sharon Xu added 3 commits December 13, 2024 16:27

address comments

e6bdec6

add more test cases

9d8fa7e

add missing changes

0933a0b

SharonXSharon requested a review from tshortli December 17, 2024 17:30

SharonXSharon commented Dec 17, 2024

View reviewed changes

rjmccall requested changes Dec 18, 2024

View reviewed changes

SharonXSharon and others added 5 commits December 19, 2024 10:31

Merge branch 'swiftlang:main' into swift-objc-protocol

5773eec

move check to checkTypeMetadataAvailability

127695e

move check to checkTypeMetadataAvailability

f87872a

delete unnecessary tests

4ebfa25

fix typos

47c3634

SharonXSharon commented Dec 20, 2024

View reviewed changes

SharonXSharon requested a review from rjmccall December 20, 2024 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Diagnose non_runtime_protocol #77339

Diagnose non_runtime_protocol #77339

Uh oh!

SharonXSharon commented Oct 31, 2024

drodriguez commented Oct 31, 2024

drodriguez commented Oct 31, 2024

drodriguez commented Nov 1, 2024

SharonXSharon commented Nov 4, 2024

tshortli Nov 4, 2024

tshortli Nov 4, 2024

SharonXSharon Nov 15, 2024

tshortli Nov 15, 2024

tshortli Nov 4, 2024

tshortli Nov 4, 2024

rjmccall Nov 15, 2024

tshortli Nov 4, 2024

tshortli Nov 4, 2024

tshortli Nov 4, 2024

Xazax-hun Nov 18, 2024

SharonXSharon Dec 14, 2024

rjmccall Dec 18, 2024

AdamCmiel Dec 20, 2024

rjmccall Dec 20, 2024 •

edited

Loading

rjmccall Dec 18, 2024

SharonXSharon Dec 20, 2024

SharonXSharon May 21, 2025

Labels

6 participants

		"this Objective-C protocol '%0' has the non_runtime_protocol attribute"
		" and its metadata is not available in Swift.",

		@@ -0,0 +1,23 @@
		#import "availability_non_runtime_protocol_objc_protocol.h"

Diagnose non_runtime_protocol #77339

Are you sure you want to change the base?

Diagnose non_runtime_protocol #77339

Uh oh!

Conversation

SharonXSharon commented Oct 31, 2024

drodriguez commented Oct 31, 2024

drodriguez commented Oct 31, 2024

drodriguez commented Nov 1, 2024

SharonXSharon commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

6 participants

rjmccall Dec 20, 2024 •

edited

Loading