Static analysis checks related to NSError** argument

[llvmbot]

Bugzilla Link	2600
Resolution	FIXED
Resolved on	Nov 07, 2018 00:22
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@tkremenek

Extended Description

Here's an idea for another static analyzer check:
If a method accepts NSError** argument, based on ObjC conventions and recommendations from Apple that method should have non-void return value.

From "Creating and Returning NSError Objects" [1]

"In general, these methods should not indicate an error through the existence of an NSError object. Instead, they should return NO or nil from the method to indicate that an error occurred. Return the NSError object to describe the error."

For example:

• (void)myMethodWhichMayFail:(NSError *)error {
  error = [NSError errorWithDomain:@"domain" code:1 userInfo:nil];
  // expected-warning: {{Method accepting NSError argument should have non-void return value to indicate that an error occurred.}}
  }

The correct way, for example:

• (BOOL)myMethodWhichMayFail:(NSError **)error {
  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:nil];
  return FALSE; // no-warning
  }

The example above is still dangerous, because caller of myMethodWhichMayFail: may not be interested in NSError and pass nil as 'error' argument, which would cause null dereference (which clang doesn't notice at this moment). So another static analysis check could make sure that code checks that 'error' argument is not null before dereferencing it. For example:

• (BOOL)myMethodWhichMayFail:(NSError **)error {
  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:nil];
  // expected-warning: {{Potential null dereference. 'error' may be NULL.}}
  return FALSE;
  }

A simple if check would be enough to fix this:

• (BOOL)myMethodWhichMayFail:(NSError **)error {
  if(error != nil)
  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:nil];
  // no-warning
  return FALSE;
  }

[1] http://developer.apple.com/documentation/Cocoa/Conceptual/ErrorHandlingCocoa/CreateCustomizeNSError/chapter_4_section_4.html

[llvmbot]

assigned to @tkremenek

[tkremenek]

I found this code guideline very interesting. Is there a significant risk of false positives here? From my standpoint, it looks like NSError** (NSError* passed by reference) should always be checked against nil.

[llvmbot]

I found this code guideline very interesting. Is there a significant risk of
false positives here? From my standpoint, it looks like NSError** (NSError*
passed by reference) should always be checked against nil.

I think checking NSError** against nil is the recommended pattern. It's completely normal for user to pass nil (or NULL) as NSError** argument (at least this is convention used in all methods in Cocoa framework which accept NSError** argument). Assuming that NSError** argument is not nil (by not checking it against nil before dereferencing) goes against that convention.

I don't think there should be lot of false positives (at least there won't be any false positives in my code). If someone really wants to go against the convention, maybe they could disable this check by marking each NSError** argument with 'nonnull' attribute?

[tkremenek]

The first check is now implemented:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080915/007584.html

[llvmbot]

The first check is now implemented:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080915/007584.html

Great, thanks! I tested it and it worked with my cases. Now I can remove my own implementation of this check from my private clang build :)

I noticed that this check warns only about those methods which are declared in the class interface and ignores all other methods. So, for example, the following code doesn't generate any warnings:

@​interface MyClass @​end
@​implementation MyClass

• (void)doSomething:(NSError **)error { }
  @​end

... but this does:

@​interface MyClass

• (void)doSomething:(NSError **)error;
  @​end
  @​implementation MyClass
• (void)doSomething:(NSError **)error { }
  @​end

Is the rationale behind this decision that there may be private methods (which are not declared in the interface of the class) which may not confirm to standard code guidelines?

[tkremenek]

Is the rationale behind this decision that there may be private methods (which
are not declared in the interface of the class) which may not confirm to
standard code guidelines?

That's the idea, but I'm fine with checking the methods only declared in @​implementation as well.

What do you think?

Quick Objective-C clarification: Can "private" methods declared in @​implementation actually be declared in the @​interface of an anscestor class? That is, the method is in the public interface of the class, but might be declared higher up in the class hierarchy.

[llvmbot]

That's the idea, but I'm fine with checking the methods only declared in
@​implementation as well.

What do you think?

Following guidelines is certainly more important in methods exposed in class interface than in "private" methods. Personally, however, I try to follow guidelines in all of my code. The analysis which I implemented for my own use checked all methods (also declarations in the @​implementation part).

My analysis did find couple of cases from my code which were strictly speaking false positives. However, the analysis was still useful because since those methods didn't follow guidelines, they were clearly bad style. I also asked couple of other Cocoa developers about understandability of those methods, and their opinion confirmed that understandability was weak.

Also a quick search in Adium source code for "NSError **" confirmed that 100% of methods accepting NSError ** do return non-void value, including those not exposed in the @​interface.

Quick Objective-C clarification: Can "private" methods declared in
@​implementation actually be declared in the @​interface of an anscestor class?
That is, the method is in the public interface of the class, but might be
declared higher up in the class hierarchy.

Yes, that's completely possible.

[tkremenek]

The second part of the check is now implemented:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080915/007593.html

I've also modified the check to scan all the methods of an implementation.

[llvmbot]

I tested your implementation and it works perfectly. Thanks :) I think this case can be closed now.