Skip to content

SR-14635: Casts to NSCopying should not always succeed #37683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 1, 2021
Merged

SR-14635: Casts to NSCopying should not always succeed #37683

merged 3 commits into from
Jun 1, 2021

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented May 28, 2021 •
edited
Loading

The runtime dynamic casting logic explores a variety of strategies for
each cast request. One of the last options is to wrap the source
in a __SwiftValue box so it can bridge to Obj-C. The previous
code was overly aggressive about such boxing; it performed the boxing
for any source type and only checked to verify that the __SwiftValue
box itself was compatible with the destination.

Among other oddities, this results in the behavior discussed
in SR-14635, where any Swift or Obj-C type will always successfully cast
to NSCopying or to NSObjectProtocol because __SwiftValue is compatible with NSCopying and NSObjectProtocol.

This is actually two subtly different issues:

  • Class types should not be subject to __SwiftValue boxing at all.
    Casting class types to class existentials is already handled elsewhere,
    so this function should just reject any source with class type.

  • Non-class types should be boxed only when being assigned to
    an AnyObject (an "unconstrained class existential"). If
    the class existential has constraints, it is by definition
    a class-constrained existential which should not receive
    any non-class object.

To solve these, this PR disables __SwiftValue boxing in two cases:

  1. If the source is a class (reference) type.
  2. If the destination has constraints

Resolves SR-14635
Resolves rdar://78224322

@tbkka
SR-14635: Casts to NSCopying should not always succeed
231825a 
The runtime dynamic casting logic explores a variety of strategies for
each cast request.  One of the last options is to wrap the source
in a `__SwiftValue` box so it can bridge to Obj-C.  The previous
code was overly aggressive about such boxing; it performed the boxing
for any source type and only checked to verify that the `__SwiftValue`
box itself was compatible with the destination.

Among other oddities, this results in the behavior discussed
in SR-14635, where any Swift or Obj-C type will always successfully cast
to NSCopying because `__SwiftValue` is compatible with NSCopying.

This is actually two subtly different issues:

* Class types should not be subject to `__SwiftValue` boxing at all.
  Casting class types to class existentials is already handled elsewhere,
  so this function should just reject any source with class type.

* Non-class types should be boxed only when being assigned to
  an AnyObject (an "unconstrained class existential").  If
  the class existential has constraints, it is by definition
  a class-constrained existential which should not receive
  any non-class object.

To solve these, this PR disables `__SwiftValue` boxing in two cases:
1. If the source is a class (reference) type.
2. If the destination has constraints

Resolves SR-14635
Resolves rdar://78224322
@tbkka tbkka requested review from jckarter, mikeash and hborla May 28, 2021 03:28
mikeash
mikeash reviewed May 28, 2021
View reviewed changes
@tbkka
Copy link
Contributor Author

tbkka commented May 28, 2021

Besides the metatype question above, this also does not resolve this peculiarity:

   struct Bar {}
   Bar() as Any as! AnyObject is NSCopying // Returns true! But should fail.

This should of course fail; Bar does not conform to NSCopying and casting to an existential does not change that. But in the current implementation, this succeeds since the cast to AnyObject generates a __SwiftValue box which does indeed conform to NSCopying. To resolve this, the is NSCopying check would have to notice that the source was a __SwiftValue box and unwrap it before testing. (The current logic blindly does the check first and only proceeds to unwrap if the check fails.)

@tbkka
Avoid boxing class metatypes on Darwin
5f9a903 
But continue boxing
 * Non-class metatypes on all platforms
 * All metatypes on non-Darwin platforms

Obj-C interop requires that we do not box class metatypes;
those must be usable as simple pointers when passed to Obj-C.
But no other metatype is object-compatible, so we have to
continue boxing everything else.
@tbkka
Copy link
Contributor Author

tbkka commented May 28, 2021

@swift-ci Please test

@tbkka
Copy link
Contributor Author

tbkka commented May 28, 2021

@swift-ci Please test

@tbkka tbkka merged commit c1fce93 into swiftlang:main Jun 1, 2021
@tbkka tbkka deleted the tbkka/sr14635-overlyAggressiveSwiftValueBoxing branch June 1, 2021 18:29
@tbkka tbkka requested a review from al45tair June 2, 2021 14:59
@tbkka tbkka mentioned this pull request Jun 15, 2021
Merged
tbkka added a commit to tbkka/swift that referenced this pull request Jan 12, 2022
@tbkka
Conditionalize the support for SR-14635
b266b7f 
SR-14635 pointed out a hole in the updated dynamic casting logic
that allowed certain casts that should have been illegal.

In particular, when casting certain types to Obj-C protocols,
the Swift value gets boxed; we would permit the cast to succeed
whenever the resulting box satisfied the protocol.  For example,
this allowed any Swift value to be cast to `NSCopying` regardless of
whether or not it implemented the required `copy(with:)` method.

This was fixed in swiftlang#37683 to reject such casts but of course some folks were
depending on this behavior to pass Swift data into Obj-C functions.
(The properly supported approach for passing arbitrary Swift data into
Obj-C functions is to cast the Swift value to `AnyObject`.)

This change makes that new behavior conditional.  For now,
the legacy semantics are enabled on Apple platforms and the
new semantics are in use everywhere else.  This will allow
us to gradually enable enforcement of the new behavior over
time.
@tbkka tbkka mentioned this pull request Jan 12, 2022
Merged
tbkka added a commit that referenced this pull request Jan 14, 2022
@tbkka
Conditionalize the fix for SR-14635 (#40822)
184488d 
* Refactor Bincompat

Organize everything around internal functions that test for
a particular OS version.

Correctly handle cases where we don't know the version of the app.

Make all bincompat functions consistently return `true` for the
legacy semantics, `false` for new semantics.  Consistently name
them all to reflect this.

* Conditionalize the support for SR-14635

SR-14635 pointed out a hole in the updated dynamic casting logic
that allowed certain casts that should have been illegal.

In particular, when casting certain types to Obj-C protocols,
the Swift value gets boxed; we would permit the cast to succeed
whenever the resulting box satisfied the protocol.  For example,
this allowed any Swift value to be cast to `NSCopying` regardless of
whether or not it implemented the required `copy(with:)` method.

This was fixed in #37683 to reject such casts but of course some folks were
depending on this behavior to pass Swift data into Obj-C functions.
(The properly supported approach for passing arbitrary Swift data into
Obj-C functions is to cast the Swift value to `AnyObject`.)

This change makes that new behavior conditional.  For now,
the legacy semantics are enabled on Apple platforms and the
new semantics are in use everywhere else.  This will allow
us to gradually enable enforcement of the new behavior over
time.

* Just skip this test on Apple platforms, since it is inconsistently implemented there (and is therefore not really testable)
tbkka added a commit to tbkka/swift that referenced this pull request Jan 14, 2022
@tbkka
Revert "SR-14635: Casts to NSCopying should not always succeed (swift…
224b47b 
…lang#37683)"

This reverts commit c1fce93.

The stricter semantics for casting to a protocol-constrained class existential
seems to break some existing code.

So we're going to revert that in the 5.6 branch to eliminate problems.

Note:  The new semantics will remain in the main branch with the usual
backwards-compatibility allowances so that we can continue to improve
the correctness of the casting behavior.
tbkka added a commit that referenced this pull request Jan 18, 2022
@tbkka
Revert "SR-14635: Casts to NSCopying should not always succeed (#37683)…
858c109 
…" (#40859)

This reverts commit c1fce93.

The stricter semantics for casting to a protocol-constrained class existential
seems to break some existing code.

So we're going to revert that in the 5.6 branch to eliminate problems.

Note:  The new semantics will remain in the main branch with the usual
backwards-compatibility allowances so that we can continue to improve
the correctness of the casting behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeash mikeash mikeash approved these changes

@jckarter jckarter Awaiting requested review from jckarter

@hborla hborla Awaiting requested review from hborla

@al45tair al45tair Awaiting requested review from al45tair al45tair is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tbkka @mikeash