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 nullability annotation for -[OIDExternalUserAgentIOS init] #727

Conversation

tiwoc
Copy link
Contributor

@tiwoc tiwoc commented Oct 6, 2022

This PR fixes this build warning:

Source/AppAuth/iOS/OIDExternalUserAgentIOS.h:37:1: warning build: conflicting nullability specifier on return types, 'nullable' conflicts with existing specifier 'nonnull'

Unfortunately, this warning is misleading. -[NSObject init] doesn't have any nullability annotation, which is equivalent to null_unspecified, but -[OIDExternalUserAgentIOS init] has the conflicting nullable annotation.

The straightforward solution would be to remove the annotation here to get in sync with the base class, but this would be incorrect: In line 34, NS_ASSUME_NONNULL_BEGIN is used, which would make the annotation default to nonnull instead of null_unspecified, which is why null_unspecified has to be used explicitly.

To verify the fix, run the iOS tests in this repository with and without this PR. With this PR applied, the warning will no longer show up.

Fixes #442

@tiwoc tiwoc changed the title Fix nullability annotation for OIDExternalUserAgentIOS init Fix nullability annotation for -[OIDExternalUserAgentIOS init] Oct 6, 2022
@tiwoc tiwoc changed the title Fix nullability annotation for -[OIDExternalUserAgentIOS init] Fix nullability annotation for -[OIDExternalUserAgentIOS init] Oct 6, 2022
NSObject init doesn't have any nullability annotation, which is
equivalent to null_unspecified.

The straightforward solution would be to remove the annotation here to
get in sync with the base class, but this would be incorrect: In line
34, NS_ASSUME_NONNULL_BEGIN is used, which would make the annotation
default to nonnull instead of null_unspecified.
@tiwoc tiwoc force-pushed the fix-442-OIDExternalUserAgentIOS-init-nullability branch from 259065d to e5851da Compare October 6, 2022 14:36
@tiwoc
Copy link
Contributor Author

tiwoc commented Oct 26, 2022

@petea, is there anything I can improve here to make it more likely to get this merged? I'm working on bringing down the number of build warnings in the project I'm working on to zero so that new warnings in our own code stick out.

@petea
Copy link
Collaborator

petea commented Oct 26, 2022

@tiwoc I'm waiting for the next major release to address the handful of nullability annotation issues that we have since the fixes cause breaking API surface changes on the Swift side.

In the meantime, have you signed the contributor agreements?

@tiwoc
Copy link
Contributor Author

tiwoc commented Oct 27, 2022

Thanks for letting me know.

In the meantime, have you signed the contributor agreements?

👍

@LaurentiuUngur
Copy link

Any updates on this ? Thank you !

@orj
Copy link

orj commented Jan 25, 2023

I too would like this addressed. init being nullable is not valid Objective-C or Swift.

@imfractured
Copy link

hey can we get this merged? I'm looking to use this framework for an enterprise app but this issue has been going on in the background for the last few years. please get this resolved? its literally 3 lines of code and we don't need anyone to take credit for it, we just need it fixed please!!!

@cornr
Copy link

cornr commented Apr 17, 2023

Hi @petea, we would also like to see this warning to vanish from our project. Is there a timeframe for the next major release?

@krissbennett
Copy link

This looks to be approved but not merged? It would be great to get this in a new release so can get rid of this warning.
Is there an ETA for this getting into a release?

@ChrisPearce
Copy link

Hi @petea. Is it intended that this pull request will be merged and included in a release?

@Vyazovoy
Copy link

Vyazovoy commented Oct 3, 2023

Hi! Please release these trivial changes or at least let us know why can't it be merged 😊

@mdmathias mdmathias self-requested a review October 4, 2023 15:42
Copy link
Collaborator

@mdmathias mdmathias left a comment

Choose a reason for hiding this comment

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

This lgtm. We don't have immediate plans for a release, but will have to make one soon (sometime in mid Q4, perhaps) to add a privacy manfiest. This change will be included in that release.

@mdmathias mdmathias merged commit b376a87 into openid:master Oct 4, 2023
HadesPTIT pushed a commit to HadesPTIT/AppAuth-iOS that referenced this pull request Nov 14, 2023
@CarlSarkisS
Copy link

@HadesPTIT Could you please explain that revert? The warning is especially annoying for pods relying on AppAuth, as by default we want to block the release if there are warnings (and this is a good rule).
There seems to be a consensus on the validity of b376a87, could you please detail your push-back?

@tiwoc
Copy link
Contributor Author

tiwoc commented Nov 23, 2023

@CarlSarkisS The revert happened on the HadesPTIT/AppAuth-iOS fork, not the upstream openid/AppAuth-iOS repo. It won't make its way into this repo.

@CarlSarkisS
Copy link

@tiwoc, oh, ok, I had missed that :)
Do you know when it will be released into a 1.6.3 or 1.7.0?

@tiwoc
Copy link
Contributor Author

tiwoc commented Nov 23, 2023

I wish I knew.

@ashok
Copy link

ashok commented Jan 3, 2024

Bumping it up as still affected by this issue on v1.6.2. Waiting for new release.

OIDExternalUserAgentIOS.h:37:1: conflicting nullability specifier on return types, 'nullable' conflicts with existing specifier 'nonnull'

- (nullable instancetype)init API_AVAILABLE(ios(11))

@macdrevx
Copy link

+1 looking forward to a new release with this fix. We were in the process of switching to SPM to install this dependency, but it looks like we may be sticking with CocoaPods since we can't find a way to prevent the warning from failing our CI builds which treat swift warnings as errors.

@mdmathias
Copy link
Collaborator

This will come out in an upcoming release with AppAuth's privacy manifest. I do not have a specific date, but it will be before Apple's spring 2024 deadline for privacy manifests.

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.

Nullability warning in OIDExternalUserAgentIOS