Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[in_app_purchase] Fix iOS build warning #2192

Merged
jmagman merged 2 commits into from
Oct 15, 2019
Merged

[in_app_purchase] Fix iOS build warning #2192

jmagman merged 2 commits into from
Oct 15, 2019

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 15, 2019

Description

  • Fix mutable collection being stored immutably.

Slight cleanup of InAppPurchasePlugin since I was editing it anyway:

  • Decorated designated initializer to initialize mutable collection ivars on init, then remove (unthreadsafe) getters in favor of synthesized property getters.
  • Set ivars instead of properties in initializers.
  • Make category properties readonly.

Related Issues

Fixes flutter/flutter#42679.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@jmagman jmagman requested a review from cyanglaz October 15, 2019 01:19
@jmagman jmagman requested a review from mklim as a code owner October 15, 2019 01:19
jmagman
jmagman commented Oct 15, 2019
View reviewed changes
packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m Outdated
// After querying the product, the available products will be saved in the map to be used
// for purchase.
@property(copy, nonatomic) NSMutableDictionary *productsCache;
@property(strong, nonatomic) NSMutableDictionary *productsCache;
Copy link
Member Author

@jmagman jmagman Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is the actual fixed warning, everything else in this file is cleanup. This fix may close out some bug reports where cached products can't be fetched because the dictionary is immutable, but I didn't look too hard.

warning: Property of mutable type 'NSMutableDictionary' has 'copy' attribute; an immutable object
      will be stored instead

cyanglaz
cyanglaz approved these changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m
Comment on lines +45 to +46
_requestHandlers = [NSMutableSet new];
_productsCache = [NSMutableDictionary new];
Copy link
Contributor

@cyanglaz cyanglaz Oct 15, 2019

Choose a reason for hiding this comment

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

curious, what is the benefit of initialize them in at initialization v.s. the lazy init that we used to have?

Copy link
Member Author

@jmagman jmagman Oct 15, 2019

Choose a reason for hiding this comment

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

In this particular case the lazy getter wasn't threadsafe. Also this class assumes these collections are instantiated, but there's nothing stopping a subclass from overriding the getter to do something wonky that doesn't call super or set up the ivar (probably unlikely). To me it looked like it was lazily instantiated because there was no designated initializer decoration on the init methods, so there was no one canonical spot where these collections could be relied on to be instantiated.

Assuming this object is queue confined (I don't know if that's actually enforced or just convention) it's fine to keep it lazily instantiated, and I can change it back if you prefer!

Copy link
Contributor

@cyanglaz cyanglaz Oct 15, 2019

Choose a reason for hiding this comment

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

Thanks for the explanation. What you have now is good! LGTM again.

@jmagman jmagman merged commit 93fe8f7 into flutter:master Oct 15, 2019
@jmagman jmagman deleted the warnings-iap branch October 15, 2019 19:48
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@jmagman @mormih
[in_app_purchase] Fix iOS build warning (flutter#2192)
1a25f77
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@jmagman
[in_app_purchase] Fix iOS build warning (flutter#2192)
baae281
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mklim mklim Awaiting requested review from mklim

1 more reviewer

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[in_app_purchase] Fix clang analyzer warnings in iOS platform code

3 participants

@jmagman @cyanglaz @googlebot