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

fix(Swift): Nullability stuff broke Swift compatibility #1521

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Jan 17, 2025

Platforms affected

iOS

Motivation and Context

CDVPlugin's commandDelegate is a weak pointer, which means technically in Swift it should be an optional type that requires unwrapping. For some reason, it is not.

If the wrap the CDVPlugin class in the ASSUME_NONNULL macro, Swift suddenly starts enforcing that it's an optional, and this breaks all existing Swift plugins.

Description

You aren't allowed to combine weak and nonnull, and all the properties in CDVPlugin are weak, so just... don't wrap it in ASSUME_NONNULL to make life easier for everyone 🙃

Testing

Tested with mobilespec (which fails on master and passes with this change)

Checklist

  • I've run the tests to see all new and existing tests pass

@dpogue dpogue added this to the 8.0.0 milestone Jan 17, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.41%. Comparing base (12aeeb4) to head (b4d8d75).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1521   +/-   ##
=======================================
  Coverage   81.41%   81.41%           
=======================================
  Files          16       16           
  Lines        1862     1862           
=======================================
  Hits         1516     1516           
  Misses        346      346           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erisu
Copy link
Member

erisu commented Jan 17, 2025

  • Building the mobilespec project with main branch fails as stated in PR description.
  • Building the mobilespec project with this PR shows a successful build.

There is warnings with the build though:

While building module 'Cordova':
In file included from <module-includes>:1:
In file included from /path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/Cordova.h:25:
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:45:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   45 | extern const NSNotificationName CDVPageDidLoadNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:45:14: note: insert '_Nullable' if the
      pointer may be null
   45 | extern const NSNotificationName CDVPageDidLoadNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:45:14: note: insert '_Nonnull' if the pointer
      should never be null
   45 | extern const NSNotificationName CDVPageDidLoadNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:46:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   46 | extern const NSNotificationName CDVPluginHandleOpenURLNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:46:14: note: insert '_Nullable' if the
      pointer may be null
   46 | extern const NSNotificationName CDVPluginHandleOpenURLNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:46:14: note: insert '_Nonnull' if the pointer
      should never be null
   46 | extern const NSNotificationName CDVPluginHandleOpenURLNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:47:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   47 | extern const NSNotificationName CDVPluginHandleOpenURLWithAppSourceAndAnnotationNotification CDV_DEPRECATED(8, "Find sourceApplication and annotations in the userInfo of the CDVPlugi...
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:47:14: note: insert '_Nullable' if the
      pointer may be null
   47 | extern const NSNotificationName CDVPluginHandleOpenURLWithAppSourceAndAnnotationNotification CDV_DEPRECATED(8, "Find sourceApplication and annotations in the userInfo of the CDVPlugi...
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:47:14: note: insert '_Nonnull' if the pointer
      should never be null
   47 | extern const NSNotificationName CDVPluginHandleOpenURLWithAppSourceAndAnnotationNotification CDV_DEPRECATED(8, "Find sourceApplication and annotations in the userInfo of the CDVPlugi...
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:48:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   48 | extern const NSNotificationName CDVPluginResetNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:48:14: note: insert '_Nullable' if the
      pointer may be null
   48 | extern const NSNotificationName CDVPluginResetNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:48:14: note: insert '_Nonnull' if the pointer
      should never be null
   48 | extern const NSNotificationName CDVPluginResetNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:49:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   49 | extern const NSNotificationName CDVViewWillAppearNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:49:14: note: insert '_Nullable' if the
      pointer may be null
   49 | extern const NSNotificationName CDVViewWillAppearNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:49:14: note: insert '_Nonnull' if the pointer
      should never be null
   49 | extern const NSNotificationName CDVViewWillAppearNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:50:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   50 | extern const NSNotificationName CDVViewDidAppearNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:50:14: note: insert '_Nullable' if the
      pointer may be null
   50 | extern const NSNotificationName CDVViewDidAppearNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:50:14: note: insert '_Nonnull' if the pointer
      should never be null
   50 | extern const NSNotificationName CDVViewDidAppearNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:51:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   51 | extern const NSNotificationName CDVViewWillDisappearNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:51:14: note: insert '_Nullable' if the
      pointer may be null
   51 | extern const NSNotificationName CDVViewWillDisappearNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:51:14: note: insert '_Nonnull' if the pointer
      should never be null
   51 | extern const NSNotificationName CDVViewWillDisappearNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:52:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   52 | extern const NSNotificationName CDVViewDidDisappearNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:52:14: note: insert '_Nullable' if the
      pointer may be null
   52 | extern const NSNotificationName CDVViewDidDisappearNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:52:14: note: insert '_Nonnull' if the pointer
      should never be null
   52 | extern const NSNotificationName CDVViewDidDisappearNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:53:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   53 | extern const NSNotificationName CDVViewWillLayoutSubviewsNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:53:14: note: insert '_Nullable' if the
      pointer may be null
   53 | extern const NSNotificationName CDVViewWillLayoutSubviewsNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:53:14: note: insert '_Nonnull' if the pointer
      should never be null
   53 | extern const NSNotificationName CDVViewWillLayoutSubviewsNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:54:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   54 | extern const NSNotificationName CDVViewDidLayoutSubviewsNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:54:14: note: insert '_Nullable' if the
      pointer may be null
   54 | extern const NSNotificationName CDVViewDidLayoutSubviewsNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:54:14: note: insert '_Nonnull' if the pointer
      should never be null
   54 | extern const NSNotificationName CDVViewDidLayoutSubviewsNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:55:14: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   55 | extern const NSNotificationName CDVViewWillTransitionToSizeNotification;
      |              ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:55:14: note: insert '_Nullable' if the
      pointer may be null
   55 | extern const NSNotificationName CDVViewWillTransitionToSizeNotification;
      |              ^
      |                                 _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:55:14: note: insert '_Nonnull' if the pointer
      should never be null
   55 | extern const NSNotificationName CDVViewWillTransitionToSizeNotification;
      |              ^
      |                                 _Nonnull
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:84:4: warning: pointer is missing a
      nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
   84 | - (id)appDelegate;
      |    ^
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:84:4: note: insert '_Nullable' if the pointer
      may be null
   84 | - (id)appDelegate;
      |    ^
      |       _Nullable
/path/to/mobilespec/node_modules/cordova-ios/CordovaLib/include/Cordova/CDVPlugin.h:84:4: note: insert '_Nonnull' if the pointer
      should never be null
   84 | - (id)appDelegate;
      |    ^
      |       _Nonnull
12 warnings generated.

CDVPlugin's commandDelegate is a weak pointer, which means technically
in Swift it should be an optional type that requires unwrapping. For
some reason, it is not.

If the wrap the CDVPlugin class in the ASSUME_NONNULL macro, Swift
suddenly starts enforcing that it's an optional, and this breaks all
existing Swift plugins.

You aren't allowed to combine `weak` and `nonnull`, and all the
properties in CDVPlugin are weak, so just... don't wrap it in
ASSUME_NONNULL to make life easier for everyone 🙃
@dpogue dpogue force-pushed the swift-optional-fix branch from 89efb6f to b4d8d75 Compare January 17, 2025 17:18
@dpogue
Copy link
Member Author

dpogue commented Jan 17, 2025

Good catch, those warnings should be fixed now.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Ran npm t, passed, and no warnings.
  • Built the sample app successfully with no warnings.
  • Built the mobilespec app successfully with no warnings from CordovaLib.

@dpogue dpogue merged commit 5cf4d94 into apache:master Jan 17, 2025
11 checks passed
@dpogue dpogue deleted the swift-optional-fix branch January 17, 2025 18:02
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

Successfully merging this pull request may close these issues.

3 participants