Bug 206480 – check-webkit-style: Improve header guard checks

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Improve header guard checks in check-webkit-style.

Bug 206448 demonstrated that the header guard checks in check-webkit-style would not warn about new header files that were missing "#pragma once" statements because it assumed that any header file that didn't already have an #ifndef/#define/#endif check _might_ be an Objective-C header that didn't need a header guard.

We can do better than this.

[David Kilzer (:ddkilzer)]

Created attachment 388171 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Here's how to find header files with missing guards:

$ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing [path]

To find "legacy header guards" that could be updated to "#pragma once", use:

$ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard [path]

[David Kilzer (:ddkilzer)]

Hmm...apparently this patch is not python3-compliant:

[866/1893] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_build_header_guard erred:
  Traceback (most recent call last):
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 2684, in test_build_header_guard
      self.assert_header_guard('#pragma once', '')
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 348, in assert_header_guard
      self.perform_header_guard_check(code, filename))
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 314, in perform_header_guard_check
      return self.perform_lint(code, filename, basic_error_rules)
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 260, in perform_lint
      self.process_file_data(filename, extension, lines, error_collector, unit_test_config)
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 254, in process_file_data
      checker.check(lines)
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 4279, in check
      self.handle_style_error, self.min_confidence)
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 4111, in _process_lines
      check_for_header_guard(filename, lines, error)
    File "/Volumes/Data/worker/WebKitPy-Tests-EWS/build/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 963, in check_for_header_guard
      if reduce(lambda x, y: x or y, map(lambda x: x in line, ['@class', '@interface', '@prototype'])):
  NameError: name 'reduce' is not defined

[David Kilzer (:ddkilzer)]

Created attachment 388173 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 388178 [details]
Patch v3

[David Kilzer (:ddkilzer)]

See Bug 206481 for output from this script for bmalloc, WTF, JavaScriptCore.

Below are WebCore, WebKitLegacy and WebKit (so you may evaluate possible false positive rate when reviewing):

$ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebCore
ERROR: Source/WebCore/bridge/npruntime_internal.h:28:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/network/HTTPStatusCodes.h:4:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/network/mac/WebCoreURLResponse.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/ios/wak/WebCoreThreadSystemInterface.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/mac/WebNSAttributedStringExtras.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/cocoa/SystemVersion.h:25:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.h:2:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxEncoderFactory.h:2:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/graphics/FormatConverter.h:28:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/graphics/ImageBufferData.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/editing/cocoa/AutofillElements.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/PAL/pal/ios/QuickLookSoftLink.h:25:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/PAL/pal/spi/ios/SQLite3SPI.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebCore/bindings/js/StructuredClone.h:27:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
Total errors found: 15 in 9934 files

$ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebKitLegacy
ERROR: Source/WebKitLegacy/WebCoreSupport/WebViewGroup.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebKitStatisticsPrivate.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/resource.h:33:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebDocumentLoader.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebPreferenceKeysPrivate.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebContextMenuClient.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebDesktopNotificationsDelegate.h:31:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.h:27:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/win/WebCoreSupport/WebDragClient.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/Misc/WebKitStatisticsPrivate.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/Misc/WebTypesInternal.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/Misc/WebLocalizableStrings.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/DOM/WebAutocapitalizeTypes.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:29:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebMediaPlaybackTargetPicker.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
Total errors found: 15 in 1178 files

$ ./Tools/Scripts/check-webkit-style --filter=-,+build/header_guard_missing Source/WebKit 2>&1 | grep -v Skipping
ERROR: Source/WebKit/UIProcess/API/C/WKPageRenderingProgressEventsInternal.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:2:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebArchive.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKNavigationData.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/Platform/IPC/Attachment.h:27:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/Platform/ios/AccessibilityIOS.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/Shared/ShareSheetCallbackID.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/Shared/mac/SecItemShim.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
ERROR: Source/WebKit/NetworkProcess/glib/WebKitCachedResolver.h:26:  Missing #pragma once for header guard.  [build/header_guard_missing] [5]
Total errors found: 9 in 3779 files

[Darin Adler]

Comment on attachment 388178 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:965
> +        if functools.reduce(lambda x, y: x or y, map(lambda x: x in line, ['@class', '@interface', '@protocol'])):
> +            has_objc_keywords = True

A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.

[David Kilzer (:ddkilzer)]

Comment on attachment 388178 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965
>> +            has_objc_keywords = True
> 
> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.

Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below.

I felt Comment #6 was a pretty good indication of a low false-positive rate.

[David Kilzer (:ddkilzer)]

Comment on attachment 388178 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review

>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965
>>> +            has_objc_keywords = True
>> 
>> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.
> 
> Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below.
> 
> I felt Comment #6 was a pretty good indication of a low false-positive rate.

For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources.

Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources.

[David Kilzer (:ddkilzer)]

Comment on attachment 388178 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review

>>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965
>>>> +            has_objc_keywords = True
>>> 
>>> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.
>> 
>> Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below.
>> 
>> I felt Comment #6 was a pretty good indication of a low false-positive rate.
> 
> For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources.
> 
> Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources.

Here's an example of a true positive from Comment #6 with an __OBJC__ check where the "#pragma once" statement is missing:

Source/WebCore/platform/network/mac/WebCoreURLResponse.h

[Darin Adler]

Comment on attachment 388178 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=388178&action=review

>>>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:965
>>>>> +            has_objc_keywords = True
>>>> 
>>>> A header with an Objective-C keyword is not necessarily an Objective-C-only header. There could be an #ifdef __OBJC__ guarding the Objective-C part.
>>> 
>>> Yep, see the `has_objc_check` check three lines above, and the logic for returning early two lines below.
>>> 
>>> I felt Comment #6 was a pretty good indication of a low false-positive rate.
>> 
>> For example, looking at Source/WebCore/platform/cocoa/SystemVersion.h (which was flagged as missing a "#pragma once" statement in Comment #6) without looking at where it's included, you might guess that it's an Objective-C++ header, but maybe it's relying on an `OBJC_CLASS NSString;` definition from another header in UnifiedSources.
>> 
>> Since it's currently ambiguous, I'd probably add an "@class NSString;" statement to the header to make it obvious that it's only used by Objective-C[++] sources.
> 
> Here's an example of a true positive from Comment #6 with an __OBJC__ check where the "#pragma once" statement is missing:
> 
> Source/WebCore/platform/network/mac/WebCoreURLResponse.h

I see. Had missed the other checks.

[WebKit Commit Bot]

Comment on attachment 388178 [details]
Patch v3

Clearing flags on attachment: 388178

Committed r254831: <https://trac.webkit.org/changeset/254831>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58740920>