Skip to content
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.

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

Xcode 11.4 beta: 'NSTextView' is incompatible with 'weak' references #2125

Closed
5 of 17 tasks
tonyarnold opened this issue Feb 6, 2020 · 26 comments
Closed
5 of 17 tasks

Comments

@tonyarnold
Copy link
Contributor

Short description of the issue:

When building using the just released Xcode 11.4 beta, RxCocoa fails to build on the following line:

public weak private(set) var textView: NSTextView?

Expected outcome:

RxCocoa compiles without errors or warnings.

What actually happens:

The compile fails with the following error:

RxSwift/RxCocoa/macOS/NSTextView+Rx.swift:20:12: 'NSTextView' is incompatible with 'weak' references

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

Version 5.0.1

Platform/Environment

  • iOS
  • macOS
  • tvOS
  • watchOS
  • playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

  Version 11.4 beta (11N111s)

Installation method:

  • CocoaPods
  • Carthage
  • Git submodules

I have multiple versions of Xcode installed:
(so we can know if this is a potential cause of your issue)

  • yes (11.3.1, 11.4 beta 1)
  • no

Level of RxSwift knowledge:
(this is so we can understand your level of knowledge
and formulate the response in an appropriate manner)

  • just starting
  • I have a small code base
  • I have a significant code base
@freak4pc
Copy link
Member

freak4pc commented Feb 6, 2020

This seems like a bug to me. Do you care filing a Feedback with Apple referring to this thread? There's no reason we would strongly capture a UI component afaik

@freak4pc
Copy link
Member

freak4pc commented Feb 6, 2020

Uhm seems that according to some resources, that class doesn't support a weak reference? ;O That's quite strange.

http://zpasternack.org/some-objects-dont-support-weak-references/

Regardless, seems like a fix that needs to be made.

@tonyarnold
Copy link
Contributor Author

tonyarnold commented Feb 6, 2020

I’m happy to file feedback with Apple on this, but this limitation has been around for years (it just wasn’t highlighted by Xcode).

I don’t know that there is an answer here that anyone is going to be happy with.

I’ll let you know what they say 👍

@brunophilipe
Copy link

I've been told this used to be the case but it had been fixed. Maybe they accidentally removed the incompatibility flag and only reintroduced it now?

@tonyarnold
Copy link
Contributor Author

Ok, I've narrowed this down - it's the deployment target for RxSwift:

.macOS(.v10_10), .iOS(.v8), .tvOS(.v9), .watchOS(.v3)

If you raise the target to .macOS(.v10_12), this works (which makes sense, given @brunophilipe's comments - I'd imagine this is when the issue was addressed).

I'm happy to submit a PR - could we bump this up for RxSwift 5.1, or 6.0?

@freak4pc
Copy link
Member

freak4pc commented Feb 6, 2020

I’m not too excited bumping a deployment target for no reason. If bumping to 10.12 “fixes” it, it would seem it’s more clear this error is some sort of discrepancy/change in how it used to work.

Could you share the FB link here once you make one ?

I’ll look around some more, regardless.

Thanks!

@tonyarnold
Copy link
Contributor Author

tonyarnold commented Feb 6, 2020

OK, I've filed this as FB7566796.

I guess let's wait to see what the AppKit/Xcode team have to say about this - my guess is that this error was meant to be turned on a while back (or is considered an improvement to diagnostics & warnings), and we may be stuck with it.

Here's the relevant code from NSTextView.h:

#if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_12
NS_AUTOMATED_REFCOUNT_WEAK_UNAVAILABLE
#endif

// ... 

/**************************** Ownership policy ****************************/
// Returns whether instances of the class operate in the object ownership policy introduced with macOS Sierra and later. When YES, the new object owner policy is used. Under the policy, each text view strongly retains its text storage and its text container weakly references the view. Also, the text views are compatible with __weak storage. The default is YES.
@property (readonly, class) BOOL stronglyReferencesTextStorage API_AVAILABLE(macos(10.12));

I understand your reluctance to bump deployment targets for a bug that might be transitory, but I don't think raising the deployment target is a bad thing for a major release. Consider the impact on yourself and the other developers of this project in supporting code that needs to compile and run on an operating system you no longer have access to. I believe you can't even run CI anywhere for Yosemite?

@freak4pc
Copy link
Member

freak4pc commented Feb 8, 2020

I don't think raising the deployment target is a bad thing for a major release

I have no issue with bumping the versions, but I'm not a macOS developer myself, so bumping a version of a platform I don't have intimate knowledge with seems a bit careless from my part.

Regardless, let's wait for Apple and if we don't get appropriate feedback, we might decide to bump.

Thanks for the research and time you've put into this @tonyarnold !

tonyarnold added a commit to tonyarnold/RxSwift that referenced this issue Feb 20, 2020
NSTextView did not support weak references until this macOS release. Xcode 11.4 has started to call this out, as before this commit, the project has been targeting macOS 10.9.

Fixes ReactiveX#2125.
tonyarnold added a commit to tonyarnold/RxSwift that referenced this issue Feb 20, 2020
NSTextView did not support weak references until this macOS release. Xcode 11.4 has started to call this out, as before this commit, the project has been targeting macOS 10.9.

Fixes ReactiveX#2125.
tonyarnold added a commit to tonyarnold/RxSwift that referenced this issue Feb 20, 2020
NSTextView did not support weak references until this macOS release. Xcode 11.4 has started to call this out, as before this commit, the project has been targeting macOS 10.9.

Fixes ReactiveX#2125.
@serges147
Copy link

Is it possible to raise min macOS 10.12 only for RxCocoa?

@freak4pc
Copy link
Member

If we bump, it would be for the framework as a whole

mayoff pushed a commit to mayoff/RxSwift that referenced this issue Feb 29, 2020
ReactiveX#2125

macOS prior to 10.12 does not support weak references to NSTextViews,
and Xcode 11.4 beta enforces this as a compile-time error if the build
target's deployment target is earlier than 10.12. RxCocoa 5.0.1's
deployment target is 10.10.

To work around the problem, I've marked this class, and the methods that
use it, as only available on 10.12 or later. However, that's not enough
to convince the compiler! So I've also changed the type of the weak
reference to AnyObject?, and hidden the AnyObject? reference behind a
computed property.
dannyow added a commit to dannyow/RxSwift that referenced this issue Mar 17, 2020
@serges147
Copy link

serges147 commented Mar 25, 2020

This whole "bumping to 10.12" is very sad story :( For us it means we stuck on 5.1.0 RxSwift and XCode 11.3.1 until we drop 10.11 (which won't happen till 2020 fall for sure). And the only reason for this is that lousy NSTextView rx in RxCocoa which we don't use at all.

@AdamAexol
Copy link

I'm hoping for different solution than bumping os version. A lot of users still using 10.10 and 10.11.

@tonyarnold
Copy link
Contributor Author

Apple shipped this error in Xcode 11.4 (released today). As best I can see, there's not really another option available.

A lot of users still using 10.10 and 10.11.

What apps are you shipping that support macOS releases back that far? As I mentioned earlier in the conversation, I'm unsure if you can even run CI to test RxSwift on releases back that far, so I'd be squinty-eyed at shipping a library on a platform I couldn't test for (not that this is my project, mind you!).

@freak4pc
Copy link
Member

Another option is counting on the fact Xcode 11.4 is Swift 5.2 and through a ton of #if compiler(>=5.2) around weak-handling portions.

@tonyarnold
Copy link
Contributor Author

You can only do that around type constructs and methods though, right? Can you conditionally include/exclude properties that way?

@freak4pc
Copy link
Member

It's a compiler directive, I imagine I can even do another version of the entire class, possibly. Better than dropping support if I see so many people against it.

"Luckily", my job was cut to 4 days a week thanks to Coronavirus, so I have some free time tomorrow. I'll look into it :D

@freak4pc
Copy link
Member

Yup, this works. I'm cutting a patch release:
image

@freak4pc freak4pc mentioned this issue Mar 25, 2020
5 tasks
@bukira
Copy link

bukira commented Mar 25, 2020

so no one can build this framework using swift 5.2 and Xcode 11.4 ?

@tonyarnold
Copy link
Contributor Author

“Luckily” — sorry to hear that, Shai 😞

@freak4pc
Copy link
Member

This was fixed in 5.1.1.

@nighthawk
Copy link
Contributor

@freak4pc Has this also been pushed to the main CocoaPods repo? https://cocoapods.org/pods/RxCocoa still says 5.1.0 which. (RxSwift is on 5.1.1 though.)

@freak4pc
Copy link
Member

Hey, you're right! This is odd, was sure I pushed all of them. Taking care of the rest RN.

@freak4pc
Copy link
Member

Should be good to go :)

@nighthawk
Copy link
Contributor

All good now, thank you very much!

@DivineDominion
Copy link
Contributor

Late to the party, but I'd suggest making the property itself unavailable for macOS 10.11 and earlier:

public var string: ControlProperty<String> {

Because now it compiles on all platforms, but on macOS 10.11, you simply get runtime exceptions as soon as you try to form weak references to a NSTextView.

@freak4pc
Copy link
Member

@DivineDominion Just saw your note. Unfortunately we can't hard deprecate in a minor release, this is against semver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants