-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 _ASAsyncTransaction
initialization method
#1656
Fix _ASAsyncTransaction
initialization method
#1656
Conversation
@@ -333,7 +333,7 @@ @implementation _ASAsyncTransaction | |||
|
|||
- (instancetype)initWithCompletionBlock:(void(^)(_ASAsyncTransaction *, BOOL))completionBlock | |||
{ | |||
if ((self = [self init])) { | |||
if ((self = [super init])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised there isn't a compiler warning enabled for this. Is it possible to enable the compiler warning as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bolsinga do you mean the Violation of 'self = [super init]' Rule (CLANG_ANALYZER_OBJC_SELF_INIT)
setting? It seems already enable in the Build Settings
:p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that warning catch this issue? I don't fully understand.
I like this change; my hope is just get the builds to fail if something like this bug should happen again. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a further investigation at Clang Static Analyzer
's code(ObjCSelfInitChecker) and document(osx.cocoa.SelfInit), I found the build setting of Violation of 'self = [super init]' Rule
is not targeted for our issue, instead, it is targeted for something like:
@interface MyObj : NSObject {
id x;
}
- (id)init;
@end
@implementation MyObj
- (id)init {
[super init];
x = 0; // warn: instance variable used while 'self' is not
// initialized
return 0;
}
@end
@interface MyObj : NSObject
- (id)init;
@end
@implementation MyObj
- (id)init {
[super init];
return self; // warn: returning uninitialized 'self'
}
@end
Back to our code, I think it is OK to use self = [self init]
in _ASAsyncTransaction
, since the init
method isn't overridden here, it's unnecessary to write self = [super init]
. However, it is an unsafe way. Take an example, if someone subclass_ASAsyncTransaction
, override init
and call initWithCompletionBlock:
from there, this could lead to endless recursion.
By searching through the codebase, this is the only place that has this bug. I suggest we use code review
to prevent PR introducing new bug at the moment, until the compile has a more smart checker to handle it, then we enable it :) What's your opinion?
- Invoke the superclass (super) initializer
According to Apple, the superclass (super) initializer need to be Invoked first.