This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy/google] Improve the Objective-C global variable declaration check 🔧
ClosedPublic

Authored by stephanemoore on Feb 21 2018, 8:34 AM.

Details

Summary

The current Objective-C global variable declaration check restricts naming that is permitted by the Google Objective-C style guide.

The Objective-C style guide states the following:
"Global and file scope constants should have an appropriate prefix. [...] Constants may use a lowercase k prefix when appropriate"
http://google.github.io/styleguide/objcguide#constants

This change fixes the check to allow two or more capital letters as an appropriate prefix. This change intentionally avoids making a decision regarding whether to flag constants that use a two letter prefix (two letter prefixes are reserved by Apple¹ but many projects seem to violate this guideline).

This change eliminates an important category of false positives (constants prefixed with '[A-Z]{2,}') at the cost of introducing a less important category of false negatives (constants prefixed with only '[A-Z]'). The false positives are observed in standard recommended code while the false negatives occur in non-standard unrecommended code. The number of eliminated false positives is expected to be significantly larger than the number of exposed false negatives.

(1)
"Two-letter prefixes like these are reserved by Apple for use in framework classes."
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/Conventions/Conventions.html

Diff Detail

Event Timeline

stephanemoore created this revision.Feb 21 2018, 8:34 AM
aaron.ballman accepted this revision.Feb 21 2018, 8:49 AM
aaron.ballman added a subscriber: aaron.ballman.

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

test/clang-tidy/google-objc-global-variable-declaration.m
33

Can you also add the code from the style guide as a test case?

extern NSString *const GTLServiceErrorDomain;

typedef NS_ENUM(NSInteger, GTLServiceError) {
  GTLServiceErrorQueryResultMissing = -3000,
  GTLServiceErrorWaitTimedOut       = -3001,
};
This revision is now accepted and ready to land.Feb 21 2018, 8:49 AM

Added excerpts from the Google Objective-C style guide section on constant naming (http://google.github.io/styleguide/objcguide#constants) as additional test cases.

stephanemoore marked an inline comment as done.Feb 21 2018, 10:48 AM

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

I will work on updating the Google Objective-C style guide to provide more clarity regarding what constitutes an appropriate prefix.

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

I will work on updating the Google Objective-C style guide to provide more clarity regarding what constitutes an appropriate prefix.

Fantastic, thank you! This patch LGTM.

Wizard accepted this revision.Feb 22 2018, 11:29 AM

I forgot to update the diagnostic message per the change.

Wizard requested changes to this revision.Feb 22 2018, 11:32 AM

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

This revision now requires changes to proceed.Feb 22 2018, 11:32 AM

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

Wizard accepted this revision.Feb 22 2018, 11:41 AM

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

Hmm I feel it is a bit unfriendly to show users a rather complicated regex in the info, but I will leave it to you. Not a big problem.

This revision is now accepted and ready to land.Feb 22 2018, 11:41 AM

Update the diagnostic information to link to the Google Objective-C style guide for guidance on an appropriate prefix.

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

Hmm I feel it is a bit unfriendly to show users a rather complicated regex in the info, but I will leave it to you. Not a big problem.

At a glance this seems unconventional but could we have the warning link to the Google Objective-C style guide for guidance on an appropriate prefix? Is that allowed for diagnostic messages? I can revert if that is considered inappropriate.

aaron.ballman added inline comments.Feb 22 2018, 12:16 PM
clang-tidy/google/GlobalVariableDeclarationCheck.cpp
92

We don't usually put hyperlinks in the diagnostic messages, so please remove this.

My suggestion about describing what constitutes an appropriate prefix was with regards to the style guide wording itself. For instance, that document doesn't mention that two capital letters is good. That's not on you to fix before this patch goes in, of course.

Wizard added inline comments.Feb 22 2018, 12:55 PM
clang-tidy/google/GlobalVariableDeclarationCheck.cpp
92

Btw it is actually "2 or more" characters for prefix. I think it makes sense because we need at least 2 or more characters to call it a "prefix" :-)

Revert the inclusion of the link to the Google Objective-C style guide in the diagnostic message.

stephanemoore marked 2 inline comments as done.
stephanemoore retitled this revision from [clang-tidy/google] Fix the Objective-C global variable declaration check 🔧 to [clang-tidy/google] Improve the Objective-C global variable declaration check 🔧.
stephanemoore edited the summary of this revision. (Show Details)

I had to make two more fixes to get the tests working:
• Updated the CHECK-MESSAGES in test/clang-tidy/google-objc-global-variable-declaration.m to the revised diagnostic message.
• Removed NS_ENUM and NSInteger from the additions to test/clang-tidy/google-objc-global-variable-declaration.m because their declarations were missing and I assumed that replication of the Foundation macro and type alias was not meaningfully important to this test case.

I verified the changes with the following commands:

$ ${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/check_clang_tidy.py ${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/google-objc-global-variable-declaration.m google-objc-global-variable-declaration /tmp/test && make check-all

Is that satisfactory verification of these changes?

stephanemoore added inline comments.Feb 22 2018, 10:52 PM
clang-tidy/google/GlobalVariableDeclarationCheck.cpp
92

Reverted the inclusion of the hyperlink in the message. I agree that embedding the hyperlink in the message is indirect and inconsistent with other diagnostic messages in LLVM projecta.

92

§1 Regarding Prefixes And Character Count

Strictly speaking, a single character prefix (e.g., extern const int ALimit; ❌) is possible but not allowed for constants in Google Objective-C style except for lowercase 'k' as a prefix for file scope constants (e.g., static const int kLimit = 3; ✔️). As hinted in the commit description, Google currently enforces a minimum two character prefix (e.g., extern const int ABPrefix; ✔️) or exceptionally 'k' where appropriate.

Technically this modified regex allows authored code to include inadvisable constant names with a single character prefix (e.g., extern const int APrefix; ❌). In this respect I would say that this change eliminates an important category of false positives (constants prefixed with '[A-Z]{2,}') while introducing a less important category of false negatives (constants prefixed with only '[A-Z]'). The false positives are observed in standard recommended code while the false negatives occur in non-standard unrecommended code. Without a reliable mechanism to separate the constant prefix from the constant name (e.g., splitting "FOOURL" into "FOO" and "URL" or "HTTP2Header" into "HTTP2" and "Header"), I cannot immediately think of a way to eliminate the introduced category of false negatives without introducing other false positives. I intend to continue thinking about alternative methods that might eliminate these false negatives without introducing other compromises. If it is acceptable to the reviewers I would propose to move forward with this proposed change to eliminate the observed false positives and then consider addressing the false negatives in a followup.

§2 Regarding the Google Objective-C Style Guide's Description of Appropriate Prefixes

I agree that the Google Objective-C style guide should be clearer on this topic and will pursue clarifying this promptly.

stephanemoore edited the summary of this revision. (Show Details)Feb 22 2018, 10:53 PM
stephanemoore marked an inline comment as not done.Feb 22 2018, 10:58 PM
stephanemoore added inline comments.
test/clang-tidy/google-objc-global-variable-declaration.m
33

NS_ENUM and NSInteger are not defined in this implementation file.

I can either (1) Omit the macro and the type alias; or (2) reasonably replicate the macro and type alias in this source file.

Which option is preferred? For the time being, I have pursued option (1) on the assumption that NS_ENUM and NSInteger are not critical to this test case.

aaron.ballman added inline comments.Feb 23 2018, 5:30 AM
clang-tidy/google/GlobalVariableDeclarationCheck.cpp
92

I don't think we should have the regex as part of the diagnostic either -- it's a distraction and it's not something we typically do. While it's nice for the diagnostic to explain how to fix the issue, the critical bit is diagnosing what's wrong with the code. The diagnostic without the regex does that and users have a place to find that information (the style guide docs).

test/clang-tidy/google-objc-global-variable-declaration.m
33

I think (1) is totally fine. It's the identifiers we're worried about, not the macros or types.

stephanemoore marked 3 inline comments as done.

Removed the regex from the diagnostic ✂️

stephanemoore marked 2 inline comments as done.Feb 23 2018, 5:22 PM
stephanemoore added inline comments.
clang-tidy/google/GlobalVariableDeclarationCheck.cpp
92

Sounds good to me 👌 Removed the regex from the diagnostic.

aaron.ballman accepted this revision.Feb 24 2018, 6:07 AM

This LGTM! Do you need someone to commit on your behalf?

stephanemoore marked an inline comment as done.Feb 24 2018, 7:42 PM

This LGTM! Do you need someone to commit on your behalf?

I would be happy to commit assuming that I am able to and can meet submission requirements.

I see a "Submit" button in Phabricator that I assume will land the commit if I press it?

I found some submission guidelines in the LLVM Developer Policy. Are there any other submission guidelines I should follow?

I ran the LLVM and Clang regression tests (by running make check-all from my LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":

******************** TEST 'LLVM :: Bindings/Go/go.test' FAILED ********************
Script:
--
/Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test llvm.org/llvm/bindings/go/llvm
--
Exit Code: 1

Command Output (stdout):
--
FAIL	llvm.org/llvm/bindings/go/llvm [build failed]

--
Command Output (stderr):
--
# llvm.org/llvm/bindings/go/llvm
In file included from /var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
/Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: fatal error: 'math.h' file not found
#include <math.h>
         ^~~~~~~~
1 error generated.

--

********************

I reran these tests on a clean checkout and I still encountered the failure so I presume that this failure is unrelated?

I have also been trying to run the test-suite but I have been encountering Python exceptions while trying to run lnt 😕.

If someone wants to submit on my behalf that works for me. If not, I can also continue trying to drive this myself (though verification of submission requirements would be helpful in that case).

Wizard added a comment.EditedFeb 24 2018, 7:54 PM

This LGTM! Do you need someone to commit on your behalf?

I would be happy to commit assuming that I am able to and can meet submission requirements.

I see a "Submit" button in Phabricator that I assume will land the commit if I press it?

I found some submission guidelines in the LLVM Developer Policy. Are there any other submission guidelines I should follow?

I ran the LLVM and Clang regression tests (by running make check-all from my LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":

******************** TEST 'LLVM :: Bindings/Go/go.test' FAILED ********************
Script:
--
/Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test llvm.org/llvm/bindings/go/llvm
--
Exit Code: 1

Command Output (stdout):
--
FAIL	llvm.org/llvm/bindings/go/llvm [build failed]

--
Command Output (stderr):
--
# llvm.org/llvm/bindings/go/llvm
In file included from /var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
/Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: fatal error: 'math.h' file not found
#include <math.h>
         ^~~~~~~~
1 error generated.

--

********************

I reran these tests on a clean checkout and I still encountered the failure so I presume that this failure is unrelated?

I have also been trying to run the test-suite but I have been encountering Python exceptions while trying to run lnt 😕.

If someone wants to submit on my behalf that works for me. If not, I can also continue trying to drive this myself (though verification of submission requirements would be helpful in that case).

You may wanna follow this http://llvm.org/docs/Phabricator.html#git-svn-and-arcanist to submit your changes. But as aaron.ballman@ mentioned, you may need someone who has acl to commit it for you (I have committed it for you)
For the error you see it is likely because you only updated the tools/extra repo. Usually when we sync this repo, we will have to pull parent repos as well (i.e. clang repo and llvm repo).

This revision was automatically updated to reflect the committed changes.