Diagnose public protocol requirements witnessed by internal protocol extensions #85846

sepy97 · 2025-12-04T21:06:41Z

Private member witnessing a public constraint should be deprecated. Previously existing workaround is checked and compiler emits a warning when strict access check is not passed. Test files were fixed to expect the corresponding warning.
rdar://74904373

sepy97 · 2025-12-04T21:14:43Z

There is still work to be done here (since a draft PR). This change breaks 18 tests, most of them due to the warning that is generated. One approach is to introduce a new UPCOMING_FEATURE flag and hide the diagnose behind it.

sepy97 · 2025-12-04T21:58:01Z

These fail due to the warning:
Swift(macosx-arm64) :: Compatibility/accessibility.swift
Swift(macosx-arm64) :: Compatibility/optional_visibility.swift
Swift(macosx-arm64) :: Distributed/distributed_actor_system_missing_adhoc_requirement_impls.swift
Swift(macosx-arm64) :: Generics/rdar123013710.swift
Swift(macosx-arm64) :: Interop/Cxx/class/access/access-inversion-typechecker.swift
Swift(macosx-arm64) :: NameLookup/accessibility.swift
Swift(macosx-arm64) :: SILGen/internal_protocol_refines_public_protocol_with_public_default_implementation.swift
Swift(macosx-arm64) :: Sema/accessibility.swift
Swift(macosx-arm64) :: Sema/accessibility_private.swift
Swift(macosx-arm64) :: attr/accessibility_proto.swift
Swift(macosx-arm64) :: attr/global_actor.swift
Swift(macosx-arm64) :: decl/protocol/conforms/access_corner_case.swift
Swift(macosx-arm64) :: decl/protocol/req/optional_visibility.swift

These fail due to the error:
Swift(macosx-arm64) :: Profiler/samplepgo.swift
Swift(macosx-arm64) :: Interop/Cxx/function/default-arguments-multifile.swift
Swift(macosx-arm64) :: embedded/synchronization.swift
Swift(macosx-arm64) :: SILGen/protocol_resilience.swift
Swift(macosx-arm64) :: IRGen/mangle-opaque-return-type.swift

slavapestov · 2025-12-04T22:05:53Z

lib/Sema/TypeCheckProtocol.cpp

+  if (!witness->isAccessibleFrom(actualScopeToCheck.getDeclContext())) {
+    if (dc->getParentModule()->isResilient()) {
+      // This module was built with -enable-library-evolution
+      dc->getASTContext().Diags.diagnose(witness->getLoc(), diag::err_private_member_witness_public_protocol,


witness->diagnose(...)

slavapestov · 2025-12-04T22:06:12Z

test/Sema/diag_internal_protocol_access_check.swift

+    return 42
+  }
+}
+


Too many blank lines

slavapestov · 2025-12-04T22:06:17Z

test/Sema/diag_internal_protocol_access_check.swift

+protocol InternalProtocol: PublicProtocol {}
+
+extension InternalProtocol {
+


slavapestov · 2025-12-04T22:07:53Z

include/swift/AST/DiagnosticsSema.def

+ERROR(err_private_member_witness_public_protocol,none,
+      "%0 is private and should not witness public protocol %1",
+      (const ValueDecl *,const ProtocolDecl *))
+WARNING(warn_private_member_witness_public_protocol,none,


There's already a mechanism to emit something as a warning or error, with warnUntilFutureSwiftVersionIf()

slavapestov · 2025-12-08T23:54:25Z

lib/Sema/TypeCheckProtocol.cpp

  auto actualScopeToCheck = requiredAccessScope.first;

+  // without StrictTextualInterfaceChecking feature forConformance is set true
+  bool forConformance = not dc->getASTContext().LangOpts.hasFeature(


Please use ! instead of not

Can you extract a local variable for dc->getASTContext()

slavapestov · 2025-12-08T23:54:38Z

lib/Sema/TypeCheckProtocol.cpp


  auto actualScopeToCheck = requiredAccessScope.first;

+  // without StrictTextualInterfaceChecking feature forConformance is set true


This comment is unnecessary in my opinion

slavapestov · 2025-12-08T23:55:28Z

lib/Sema/TypeCheckProtocol.cpp

+                      diagKind, getProtocolRequirementKind(requirement),
+                      witness, isSetter, requiredAccess,
+                      protoAccessScope.accessLevelForDiagnostics(), proto)
+            .warnUntilFutureSwiftVersionIf(


With this change, all access control errors from conformance checking now become warnings, we don't want that

sepy97 · 2025-12-11T23:25:13Z

@swift-ci please smoke test

nkcsgexi · 2025-12-12T17:03:00Z

@swift-ci please smoke test Windows

slavapestov · 2025-12-15T17:30:49Z

include/swift/AST/RequirementMatch.h

    ASSERT(kind != CheckKind::Availability);
  }

  RequirementCheck(AccessScope requiredAccessScope, bool forSetter)


You can probably remove the old overload then

slavapestov · 2025-12-15T17:31:02Z

include/swift/AST/RequirementMatch.h

+  // Exists specifically for AccessStrict
+  RequirementCheck(CheckKind accessKind, AccessScope requiredAccessScope,
+                   bool forSetter)
+      : Kind(accessKind), Access{requiredAccessScope, forSetter} {}


ASSERT() here that the kind is either Access or AccessStrict

slavapestov · 2025-12-15T17:32:11Z

lib/Sema/TypeCheckProtocol.cpp

+  if (match.Witness->isAccessibleFrom(requiredAccessLevel.getDeclContext(),
+                                      true) &&
+      !match.Witness->isAccessibleFrom(requiredAccessLevel.getDeclContext(),
+                                       false)) {


We write /*forConformance=*/true and /*forConformance=*/false to make this clearer

slavapestov · 2025-12-15T17:32:58Z

lib/Sema/TypeCheckProtocol.cpp

+    return RequirementCheck(CheckKind::AccessStrict, requiredAccessLevel,
+                            isSetter);
+  }
  if (checkWitnessAccess(DC, requirement, match.Witness, &isSetter))


You could sink your new check down into checkWitnessAccess() if you change the return type of checkWitnessAccess(). That would avoid repeating the /*forConformance=*/true check twice.

Wouldn't it be better to keep checkWitnessAccess() as is? Since the end goal is to get rid of forConformance altogether (then the check will only occur once).

slavapestov · 2025-12-15T17:33:16Z

test/Sema/diag_internal_protocol_access_check.swift

@@ -0,0 +1,30 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-typecheck-verify-swift -enable-library-evolution


Is -enable-library-evolution required for the test?

slavapestov · 2025-12-15T17:35:13Z

test/decl/protocol/conforms/access_corner_case.swift

  func privateRequirement()
  func privateRequirementCannotWork()
  // expected-note@-1 {{protocol requires function 'privateRequirementCannotWork()' with type '() -> ()'}}
 }


It looks like this file already covers this behavior, so it probably suffices to just update the expectations here as you did. Maybe you can add some new test cases here instead of adding a new file to test/Sema?

Private member witnessing a public constraint should be deprecated. Previously existing workaround is checked and compiler emits a warning when strict access check is not passed. Test files were fixed to expect the corresponding warning. rdar://74904373

sepy97 · 2025-12-16T00:27:38Z

@swift-ci please smoke test

sepy97 · 2025-12-16T17:36:12Z

@swift-ci please test windows

nkcsgexi · 2025-12-16T18:22:52Z

cc: @slavapestov to take another round of review🙏

sepy97 requested review from nkcsgexi and slavapestov December 4, 2025 21:06

slavapestov reviewed Dec 4, 2025

View reviewed changes

sepy97 force-pushed the public-decl-internal-protocol branch 4 times, most recently from 3340009 to b2a7b32 Compare December 8, 2025 18:40

slavapestov reviewed Dec 8, 2025

View reviewed changes

sepy97 force-pushed the public-decl-internal-protocol branch 2 times, most recently from dee5723 to 40e5c52 Compare December 10, 2025 22:14

sepy97 requested a review from slavapestov December 10, 2025 22:14

sepy97 force-pushed the public-decl-internal-protocol branch from 40e5c52 to 5d9d8b6 Compare December 10, 2025 23:55

sepy97 marked this pull request as ready for review December 10, 2025 23:55

sepy97 requested review from AnthonyLatsis, hborla, jckarter and xedin as code owners December 10, 2025 23:56

slavapestov reviewed Dec 15, 2025

View reviewed changes

sepy97 force-pushed the public-decl-internal-protocol branch from 5d9d8b6 to 6280a70 Compare December 16, 2025 00:23

sepy97 requested a review from slavapestov December 16, 2025 17:36

nkcsgexi approved these changes Dec 17, 2025

View reviewed changes

nkcsgexi merged commit 4a6badd into swiftlang:main Dec 17, 2025
3 checks passed

		protocol InternalProtocol: PublicProtocol {}

		extension InternalProtocol {


		auto actualScopeToCheck = requiredAccessScope.first;

		// without StrictTextualInterfaceChecking feature forConformance is set true

		@@ -0,0 +1,30 @@
		// RUN: %empty-directory(%t)
		// RUN: %target-typecheck-verify-swift -enable-library-evolution

Diagnose public protocol requirements witnessed by internal protocol extensions #85846

Diagnose public protocol requirements witnessed by internal protocol extensions #85846

Conversation

sepy97 commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sepy97 commented Dec 4, 2025

Uh oh!

sepy97 commented Dec 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sepy97 commented Dec 11, 2025

Uh oh!

nkcsgexi commented Dec 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sepy97 commented Dec 16, 2025

Uh oh!

sepy97 commented Dec 16, 2025

Uh oh!

nkcsgexi commented Dec 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sepy97 commented Dec 4, 2025 •

edited

Loading