Add a Presubmit to check if a file must be included or imported.

PRESUBMIT.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 """
 
@@ skipping 40 matching lines @@
 )
 
 
 _TEST_ONLY_WARNING = (
     'You might be calling functions intended only for testing from\n'
     'production code.  It is OK to ignore this warning if you know what\n'
     'you are doing, as the heuristics used to detect the situation are\n'
     'not perfect.  The commit queue will not block on this warning.')
 
 
+_INCLUDE_IMPORT_WARNING = (
+    'You are including ObjC headers or importing C++ headers. Remember to use '
+    'the right #include/#import directive.')
+
+
 _INCLUDE_ORDER_WARNING = (
     'Your #include order seems to be broken. Remember to use the right '
     'collation (LC_COLLATE=C) and check\nhttps://google.github.io/styleguide/'
     'cppguide.html#Names_and_Order_of_Includes')
 
 
 _BANNED_OBJC_FUNCTIONS = (
     (
       'addTrackingRect:',
       (
@@ skipping 693 matching lines @@
       if pattern.match(line):
         errors.append('    %s:%d' % (f.LocalPath(), line_num))
 
   results = []
   if errors:
     results.append(output_api.PresubmitError(
       'Header files should not include ui/aura/window_property.h', errors))
   return results
 
 
+def _CheckIncludeImportInFile(input_api, f, changed_linenums):
+  """Checks the include/import coherence in file f."""

[Eugene But (OOO till 7-30), 2016/12/23 16:25:08]

Could you please document the rules, about @end/@class/@protocol #import. It
worth mentioning that script is neutral on blocks which are part of C as a
language extension.

[Olivier, 2016/12/26 14:49:29]

Done.

+  custom_include_pattern = input_api.re.compile(r'\s*#include "(?P<FILE>.*)"')
+  custom_import_pattern = input_api.re.compile(r'\s*#import "(?P<FILE>.*)"')
+  guard_pattern = input_api.re.compile(
+      r'\s*#if !?defined\(__OBJC__\).*')
+  objc_pattern = input_api.re.compile(
+      r'\s*(@interface|@end|@implementation|@protocol|#import).*')
+  contents = f.NewContents()
+  warnings = []
+  line_num = 0
+  for line in contents:

[Eugene But (OOO till 7-30), 2016/12/23 16:25:08]

Could you please add implementation comments. The logic is not trivial here.

[Olivier, 2016/12/26 14:49:29]

Done.

+    line_num += 1
+    is_imported = False
+    included_file = ""
+    match_cpp = custom_include_pattern.match(line)
+    if match_cpp:
+      match_dict = match_cpp.groupdict()
+      included_file = match_dict['FILE']
+    match_objc = custom_import_pattern.match(line)
+    if match_objc:
+      match_dict = match_objc.groupdict()
+      included_file = match_dict['FILE']
+      is_imported = True
+    if included_file == "":
+      continue
+    included_absolute_path = input_api.os_path.join(
+        input_api.change.RepositoryRoot(), included_file)
+    if not input_api.os_path.isfile(included_absolute_path):
+      # Included file may need a custom -I flag.
+      # Ignore it.
+      continue
+    has_objc = False
+    ignore_objc = False
+    for included_line in open(included_absolute_path, 'r'):
+      match_guard = guard_pattern.match(included_line)
+      if match_guard:
+        ignore_objc = True
+        break
+      match_objc = objc_pattern.match(included_line)
+      if match_objc:
+        has_objc = True
+        break

[Eugene But (OOO till 7-30), 2016/12/23 16:25:09]

This is a complex body with multiple loops continue and break statements. Is it
possible to extract the content into a separate function?

[Olivier, 2016/12/26 14:49:29]

Done.

+    if ignore_objc:
+      # There is an Objective C guard, the file is internded to be included or
+      # imported.
+      continue
+    if has_objc and not is_imported:
+      warnings.append('%s:%d %s' % (f.LocalPath(), line_num, line))
+      warnings.append('Header is included but has Objective C instructions.')
+    if not has_objc and is_imported:
+      warnings.append('%s:%d %s' % (f.LocalPath(), line_num, line))
+      warnings.append('Header is imported but has no Objective C instructions.')
+  return warnings
+
+
+def _CheckIncludeImport(input_api, output_api):
+  """Checks that C++ files are included, ObjC files imported

[Eugene But (OOO till 7-30), 2016/12/23 16:25:08]

nit: s/files/headers

[Olivier, 2016/12/26 14:49:29]

Done.

+  """
+  def FileFilterIncludeImport(affected_file):
+    black_list = (_EXCLUDED_PATHS + input_api.DEFAULT_BLACK_LIST)
+    return input_api.FilterSourceFile(affected_file, black_list=black_list)
+
+  warnings = []
+  for f in input_api.AffectedFiles(file_filter=FileFilterIncludeImport):

[Eugene But (OOO till 7-30), 2016/12/23 16:25:08]

Usage of filter is discouraged by Python Style Guide:
https://google.github.io/styleguide/pyguide.html#Deprecated_Language_Features

Could you please use comprehension lists.

[Olivier, 2016/12/26 14:49:29]

All other tests use this structure and I cannot change it to a satisfactory
comprehension list.
So letting as it is .

+    if f.LocalPath().endswith(('.cc', '.h', '.mm')):
+      changed_linenums = set(line_num for line_num, _ in f.ChangedContents())
+      warnings.extend(_CheckIncludeImportInFile(input_api, f, changed_linenums))
+
+  results = []
+  if warnings:
+    results.append(output_api.PresubmitPromptOrNotify(_INCLUDE_IMPORT_WARNING,
+                                                      warnings))
+  return results
+
+
 def _CheckIncludeOrderForScope(scope, input_api, file_path, changed_linenums):
   """Checks that the lines in scope occur in the right order.
 
   1. C system files in alphabetical order
   2. C++ system files in alphabetical order
   3. Project's .h files
   """
 
   c_system_include_pattern = input_api.re.compile(r'\s*#include <.*\.h>')
   cpp_system_include_pattern = input_api.re.compile(r'\s*#include <.*>')
@@ skipping 1255 matching lines @@
   results.extend(_CheckNoUNIT_TESTInSourceFiles(input_api, output_api))
   results.extend(_CheckDCHECK_IS_ONHasBraces(input_api, output_api))
   results.extend(_CheckNoNewWStrings(input_api, output_api))
   results.extend(_CheckNoDEPSGIT(input_api, output_api))
   results.extend(_CheckNoBannedFunctions(input_api, output_api))
   results.extend(_CheckNoPragmaOnce(input_api, output_api))
   results.extend(_CheckNoTrinaryTrueFalse(input_api, output_api))
   results.extend(_CheckUnwantedDependencies(input_api, output_api))
   results.extend(_CheckFilePermissions(input_api, output_api))
   results.extend(_CheckNoAuraWindowPropertyHInHeaders(input_api, output_api))
+  results.extend(_CheckIncludeImport(input_api, output_api))
   results.extend(_CheckIncludeOrder(input_api, output_api))
   results.extend(_CheckForVersionControlConflicts(input_api, output_api))
   results.extend(_CheckPatchFiles(input_api, output_api))
   results.extend(_CheckHardcodedGoogleHostsInLowerLayers(input_api, output_api))
   results.extend(_CheckNoAbbreviationInPngFileName(input_api, output_api))
   results.extend(_CheckForInvalidOSMacros(input_api, output_api))
   results.extend(_CheckForInvalidIfDefinedMacros(input_api, output_api))
   results.extend(_CheckFlakyTestUsage(input_api, output_api))
   results.extend(_CheckAddedDepsHaveTargetApprovals(input_api, output_api))
   results.extend(
@@ skipping 263 matching lines @@
   results.extend(input_api.canned_checks.CheckTreeIsOpen(
       input_api,
       output_api,
       json_url='http://chromium-status.appspot.com/current?format=json'))
 
   results.extend(input_api.canned_checks.CheckChangeHasBugField(
       input_api, output_api))
   results.extend(input_api.canned_checks.CheckChangeHasDescription(
       input_api, output_api))
   return results