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

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 32a16e1471d4f4ceca549c578b6aebae260fc88b..a622c95c93565c5bf468e715624f50028576f6fe 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -58,6 +58,11 @@ _TEST_ONLY_WARNING = (
     '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/'
@@ -771,6 +776,82 @@ def _CheckNoAuraWindowPropertyHInHeaders(input_api, output_api):
   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.
 
@@ -2046,6 +2127,7 @@ def _CommonChecks(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))