Page MenuHomePhabricator

[analyzer] Add missing documentation for static analyzer checkers
ClosedPublic

Authored by szdominik on May 29 2017, 6:15 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

szdominik created this revision.May 29 2017, 6:15 AM
dkrupp added a subscriber: dkrupp.May 31 2017, 3:51 AM
zaks.anna edited edge metadata.Jun 14 2017, 3:10 PM

Great cleanup! Some comments below.

www/analyzer/alpha_checks.html
91 ↗(On Diff #100607)

for function calls -> in function calls?
After briefly looking into this, the checker only reports the use of uninitialized arguments in calls, not the other issues (like null function pointers). Could you double check this?

152 ↗(On Diff #100607)

sign/precision -> sign or precision

www/analyzer/available_checks.html
1423 ↗(On Diff #100607)

I cannot find MallocWithAnnotations checker. Also if it exists, it should not be on by default, it should be alpha.

(I do not know if anyone is using Optimistic=true and think we should just remove that mode..)

1556 ↗(On Diff #100607)

I do not think we should list "modeling" checkers here.

szdominik updated this revision to Diff 102650.Jun 15 2017, 3:14 AM

Delete modeling checkers (unix.StdCLibraryFunctions, cplusplus.SelfAssignment).
Delete unix.MallocWithAnnotations.

szdominik marked 3 inline comments as done.Jun 15 2017, 3:17 AM
szdominik added inline comments.
www/analyzer/alpha_checks.html
91 ↗(On Diff #100607)
zaks.anna added inline comments.Jun 16 2017, 11:01 AM
www/analyzer/alpha_checks.html
91 ↗(On Diff #100607)

The file implements 2 checkers and only one of them is in alpha:
REGISTER_CHECKER(CallAndMessageUnInitRefArg)
REGISTER_CHECKER(CallAndMessageChecker)

You can search for "CallAndMessageUnInitRefArg" to see that it effects uninitialized parameters warnings.

szdominik updated this revision to Diff 103378.Jun 21 2017, 7:40 AM
szdominik marked 3 inline comments as done.

Fixed alpha.core.CallAndMessageUnInitRefArg.

NoQ added a subscriber: NoQ.Jun 28 2017, 3:23 AM
NoQ added a comment.Jul 8 2017, 12:09 AM

I think next time we should add examples of warning messages here - not just "//warn" but how they literally may look - so that people were able to find this page, and not doxygen/github source code, when they google up their warning messages.

dcoughlin edited edge metadata.Jul 11 2017, 5:35 PM

Thanks for the patch. It is really great to see these documented!

Who is the target of this documentation? Is it developers of the analyzer or is it end users of the analyzer? If it is end users, it is unfortunate that we've been just grabbing examples from the regression tests. These tests are usually not idiomatic and, since they're designed for testing, they typically won't explain the check well to an end user.

I've suggested some more idiomatic examples inline for for some of the Objective-C targeted checkers -- but it is probably worth thinking about the remaining examples from the perspective of an end user.

www/analyzer/alpha_checks.html
180 ↗(On Diff #103378)

I'd suggest replacing all of this with the following, which is more idiomatic:

id date = [NSDate date];

// Warning: Object has a dynamic type 'NSDate *' which is incompatible with static type 'NSNumber *'"
NSNumber *number = date;
[number doubleValue];
563 ↗(On Diff #103378)

Let's replace all of this with:

NSString *reminderText = NSLocalizedString(@"None", @"Indicates no reminders");
if (reminderCount == 1) {
  // Warning: Plural cases are not supported accross all languages. Use a .stringsdict file instead
  reminderText = NSLocalizedString(@"1 Reminder", @"Indicates single reminder");
} else if (reminderCount >= 2) {
  // Warning: Plural cases are not supported accross all languages. Use a .stringsdict file instead
  reminderText = [NSString stringWithFormat:
                      NSLocalizedString(@"%@ Reminders", @"Indicates multiple reminders"),
                       reminderCount];
}
www/analyzer/available_checks.html
475 ↗(On Diff #103378)

How about:

if (name != nil)
  return;
// Warning: nil passed to a callee that requires a non-null 1st parameter
NSString *greeting = [@"Hello " stringByAppendingString:name];
492 ↗(On Diff #103378)

How about:

- (nonnull id)firstChild {
  id result = nil;
  if ([_children count] > 0)
    result = _children[0];

  // Warning: nil returned from a method that is expected to return a non-null value
  return result;
}
507 ↗(On Diff #103378)

How about:

struct LinkedList {
  int data;
  struct LinkedList *next;
};

struct LinkedList * _Nullable getNext(struct LinkedList *l);

void updateNextData(struct LinkedList *list, int newData) {
  struct LinkedList *next = getNext(list);
  // Warning: Nullable pointer is dereferenced
  next->data = 7;
}
597 ↗(On Diff #103378)

How about:

NSString *alarmText = NSLocalizedString(@"Enabled", @"Indicates alarm is turned on");
if (!isEnabled) {
  alarmText = @"Disabled";
}
UILabel *alarmStateLabel = [[UILabel alloc] init];

// Warning: User-facing text should use localized string macro
[alarmStateLabel setText:alarmText];
639 ↗(On Diff #103378)

Here is an idiomatic example:

NSNumber *photoCount = [albumDescriptor objectForKey:@"PhotoCount"];
// Warning: Comparing a pointer value of type 'NSNumber *' to a scalar integer value
if (photoCount > 0) {
  [self displayPhotos];
}
951 ↗(On Diff #103378)

The example for this checker should have Objective-C generics in it.

How about:

NSMutableArray<NSString *> *names = [NSMutableArray array];
NSMutableArray *birthDates = names;

// Warning: Conversion from value of type 'NSDate *' to incompatible type 'NSString *'
[birthDates addObject: [NSDate date]];
1038 ↗(On Diff #103378)

I don't think it is necessary to include the -initWithIvar: method in this example; just -dealloc is enough.

Who is the target of this documentation? Is it developers of the analyzer or is it end users of the analyzer? If it is end users, it is unfortunate that we've been just grabbing examples from the regression tests. These tests are usually not idiomatic and, since they're designed for testing, they typically won't explain the check well to an end user.

I've suggested some more idiomatic examples inline for for some of the Objective-C targeted checkers -- but it is probably worth thinking about the remaining examples from the perspective of an end user.

Thank you for the examples, I'll update the patch.
I think the target is the end users, so you're right, probably more idiomatic examples would be better. (And @NoQ is right with warning messages too - but it should be reconsider the whole documentation, because it doesn't have really recognizable convention for that.)
However I'm not very good with Objective-C, so probably I'm not the best for this task :)

szdominik updated this revision to Diff 106184.Jul 12 2017, 5:23 AM
szdominik marked 9 inline comments as done.

Update with more idiomatic examples (from @dcoughlin).

dcoughlin accepted this revision.Jul 12 2017, 11:00 AM

LGTM. Thanks!

Do you have commit access, or do you need someone to commit it for you?

This revision is now accepted and ready to land.Jul 12 2017, 11:00 AM

Do you have commit access, or do you need someone to commit it for you?

Thank you.
I don't have, I'm quite new at this project. :)

This revision was automatically updated to reflect the committed changes.

Committed in r308242. Thanks Dominik!