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

Reduce SDK warnings from 11724 to 24 #1527

Merged
merged 3 commits into from
Jul 18, 2022
Merged

Reduce SDK warnings from 11724 to 24 #1527

merged 3 commits into from
Jul 18, 2022

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Jul 15, 2022

The project had previously almost 12K warnings which is both absurdly high and makes trivial to miss an important warning (objective-c class does not fully conform to a protocol => runtime crash) but also actually slows down Xcode massively on each build.

It turns out that the majority of warnings come from very few classes that do not have nullability annotations. Instead of adding NS_ASSUME_NONNULL which would falsly claim non-nullability I added a counterpart MX_ASSUME_MISSING_NULLABILITY macro, and when placed in individual files, count of warnings dropped to around a 100.

A few other warning changes:

  • ignore reteain cycles and deprecated methods in tests
  • build pods with the deployment target equivalent to the SDK
  • remove deprecated and unused methods

After these changes I am counting 24 warnings, around half in some deprecated Realm types and the other half in our deprecated store methods. I do not have enough context at the moment what to do with these.

@Anderas Anderas changed the title Reduce warnings to 24 Reduce SDK warnings to 24 Jul 15, 2022
@Anderas Anderas changed the title Reduce SDK warnings to 24 Reduce SDK warnings from 11724 to 24 Jul 15, 2022
@Anderas Anderas requested review from a team and gileluard and removed request for a team July 15, 2022 15:13
Copy link
Contributor

@gileluard gileluard left a comment

Choose a reason for hiding this comment

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

I'm not very confortable with the MX_ASSUME_MISSING_NULLABILITY_BEGIN you've introduced. Of course it reduces the number of warnings drastically but wouldn't it be more accurate to set the nullability of the parameters in the h files? @ismailgulek what do you think?

@Anderas
Copy link
Contributor Author

Anderas commented Jul 18, 2022

I'm not very confortable with the MX_ASSUME_MISSING_NULLABILITY_BEGIN you've introduced. Of course it reduces the number of warnings drastically but wouldn't it be more accurate to set the nullability of the parameters in the h files? @ismailgulek what do you think?

Fair enough. What is your main concern with adding these? That we will no longer see nullability warnings? I just worry that seeing the warnings does not actually result in any improvement, seeing there are so many. This in turn slows build and testing times.

It would be good to give the .h files accurate annotations, though I would default to adding nullable everywhere as it would be a mamoth task to figure out exactly which of these fields can be nullable and which cannot.

@gileluard
Copy link
Contributor

I'm not very confortable with the MX_ASSUME_MISSING_NULLABILITY_BEGIN you've introduced. Of course it reduces the number of warnings drastically but wouldn't it be more accurate to set the nullability of the parameters in the h files? @ismailgulek what do you think?

Fair enough. What is your main concern with adding these? That we will no longer see nullability warnings? I just worry that seeing the warnings does not actually result in any improvement, seeing there are so many. This in turn slows build and testing times.

It would be good to give the .h files accurate annotations, though I would default to adding nullable everywhere as it would be a mamoth task to figure out exactly which of these fields can be nullable and which cannot.

The initial idea has been to let every developers update the h files when they have time or when they add something. It is now clear that this idea didn't work as well as expected. That's why I'd like to have a second opinion on this.

@Anderas
Copy link
Contributor Author

Anderas commented Jul 18, 2022

I'm not very confortable with the MX_ASSUME_MISSING_NULLABILITY_BEGIN you've introduced. Of course it reduces the number of warnings drastically but wouldn't it be more accurate to set the nullability of the parameters in the h files? @ismailgulek what do you think?

Fair enough. What is your main concern with adding these? That we will no longer see nullability warnings? I just worry that seeing the warnings does not actually result in any improvement, seeing there are so many. This in turn slows build and testing times.
It would be good to give the .h files accurate annotations, though I would default to adding nullable everywhere as it would be a mamoth task to figure out exactly which of these fields can be nullable and which cannot.

The initial idea has been to let every developers update the h files when they have time or when they add something. It is now clear that this idea didn't work as well as expected. That's why I'd like to have a second opinion on this.

One more idea: I could ammend MX_ASSUME_MISSING_NULLABILITY_BEGIN to manually trigger one warning via #warning. This way we still get notified these have not been converted, but will have 6 warnings for those 6 files instead of 10.000. What do you think?

@gileluard
Copy link
Contributor

I'm not very confortable with the MX_ASSUME_MISSING_NULLABILITY_BEGIN you've introduced. Of course it reduces the number of warnings drastically but wouldn't it be more accurate to set the nullability of the parameters in the h files? @ismailgulek what do you think?

Fair enough. What is your main concern with adding these? That we will no longer see nullability warnings? I just worry that seeing the warnings does not actually result in any improvement, seeing there are so many. This in turn slows build and testing times.
It would be good to give the .h files accurate annotations, though I would default to adding nullable everywhere as it would be a mamoth task to figure out exactly which of these fields can be nullable and which cannot.

The initial idea has been to let every developers update the h files when they have time or when they add something. It is now clear that this idea didn't work as well as expected. That's why I'd like to have a second opinion on this.

One more idea: I could ammend MX_ASSUME_MISSING_NULLABILITY_BEGIN to manually trigger one warning via #warning. This way we still get notified these have not been converted, but will have 6 warnings for those 6 files instead of 10.000. What do you think?

Sounds fair :)

@@ -46,6 +46,8 @@
NSInteger const kMXRoomAlreadyJoinedErrorCode = 9001;
NSInteger const kMXRoomInvalidInviteSenderErrorCode = 9002;

#warning File has not been annotated with nullability, see MX_ASSUME_MISSING_NULLABILITY_BEGIN
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that #warning cannot be a part of macro expansions so have to insert it manually, and moreoever each header file gets processed dozens of times which generates duplicate warnings (not sure why). So If these 6 warnings are kept in header files, build produces 1000 extra warnings, which is not great. So as a compromise I am putting these warnings in the implementation file instead

@Anderas Anderas requested a review from gileluard July 18, 2022 08:27
Copy link
Contributor

@gileluard gileluard left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@Anderas Anderas merged commit ae50268 into develop Jul 18, 2022
@Anderas Anderas deleted the andy/warnings branch July 18, 2022 09:14
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.

2 participants