util: Enhanced config_option_check.py.

This commit enhances the config option check python script to
significantly reduce the number of false positives.

 - Now only checks committed changes.
 - Only checks additions, not the whole file.
 - Only checks uses of CONFIG_* not in a comment for both C-style and
   Make-style files.
 - Suports a whitelist.

BUG=chromium:510672
BRANCH=None
TEST=Detected missing configs in Makefiles and C source files.
TEST=/* CONFIG_FOO */ was not detected.
TEST=' board-$(CONFIG_OPT)=board.o # CONFIG_OPT2' only CONFIG_OPT was
detected.
TEST=Changes in working dir were not detected.
TEST=Changes to config_option_check.py were not detected.
TEST=cros lint --debug util/config_option_check.py
TEST=CONFIG_FOO in a multi-line C comment was not detected.

Change-Id: I5fc2ccc77bb4f319a3c85b7d81c83027959dc96b
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/287519
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@google.com>
Commit-Queue: Aseda Aboagye <aaboagye@chromium.org>

1 files changed, 225 insertions, 74 deletions

diff --git a/util/config_option_check.py b/util/config_option_check.py
index 4ce6364d13..e114780df1 100755
--- a/util/config_option_check.py
+++ b/util/config_option_check.py
@@ -8,35 +8,55 @@ Script to ensure that all configuration options for the Chrome EC are defined
 in config.h.
 """
 from __future__ import print_function
-import re
 import os
-import argparse
+import re
+import subprocess
+
+
+class Line(object):
+  """Class for each changed line in diff output.
+
+  Attributes:
+    line_num: The integer line number that this line appears in the file.
+    string: The literal string of this line.
+  """
+
+  def __init__(self, line_num, string):
+    """Inits Line with the line number and the actual string."""
+    self.line_num = line_num
+    self.string = string
+
+
+class Hunk(object):
+  """Class for a git diff hunk.
+
+  Attributes:
+    filename: The name of the file that this hunk belongs to.
+    lines: A list of Line objects that are a part of this hunk.
+  """
+
+  def __init__(self, filename, lines):
+    """Inits Hunk with the filename and the list of lines of the hunk."""
+    self.filename = filename
+    self.lines = lines
+
 
 # Master file which is supposed to include all CONFIG_xxxx descriptions.
 CONFIG_FILE = 'include/config.h'
 
-def find_files_to_check(args):
-  """Returns a list of files to check."""
-  file_list = []
-  if args.all_files:
-    cwd = os.getcwd()
-    for (dirpath, dirnames, filenames) in os.walk(cwd, topdown=True):
-      # Ignore the build and private directories (taken from .gitignore)
-      if 'build' in dirnames:
-        dirnames.remove('build')
-      if 'private' in dirnames:
-        dirnames.remove('private')
-      for f in filenames:
-        # Only consider C source and assembler files.
-        if f.endswith(('.c', '.h', '.inc', '.S')):
-          file_list.append(os.path.join(dirpath, f))
-  else:
-    # Form list from presubmit environment variable.
-    file_list = os.environ['PRESUBMIT_FILES'].split()
-  return file_list
+# Specific files which the checker should ignore.
+WHITELIST = [CONFIG_FILE, 'util/config_option_check.py']
 
 def obtain_current_config_options():
-  """Obtains current config options from include/config.h"""
+  """Obtains current config options from include/config.h.
+
+  Scans through the master config file defined in CONFIG_FILE for all CONFIG_*
+  options.
+
+  Returns:
+    A list of all the config options in the master CONFIG_FILE.
+  """
+
   config_options = []
   config_option_re = re.compile(r'^#(define|undef)\s+(CONFIG_[A-Z0-9_]+)')
   with open(CONFIG_FILE, 'r') as config_file:
@@ -49,72 +69,203 @@ def obtain_current_config_options():
         config_options.append(word)
   return config_options
 
-def print_missing_config_options(file_list, config_options):
-  """Searches through all files in file_list for missing config options.
+def print_missing_config_options(hunks, config_options):
+  """Searches thru all the changes in hunks for missing options and prints them.
 
-  TODO: make it search only added lines instead.
+  TODO(aaboagye): Improve upon this to detect when files are being removed and a
+  CONFIG_* option is no longer used elsewhere in the repo.
+
+  Args:
+    hunks: A list of Hunk objects which represent the hunks from the git
+      diff output.
+    config_options: A list of all the config options in the master CONFIG_FILE.
+
+  Returns:
+    missing_config_option: A boolean indicating if any CONFIG_* options
+      are missing from the master CONFIG_FILE in this commit.
   """
   missing_config_option = False
   print_banner = True
   # Determine longest CONFIG_* length to be used for formatting.
   max_option_length = max(len(option) for option in config_options)
-  config_option_re = re.compile(r'\s+(CONFIG_[a-zA-Z0-9_]*)\s*')
-  for f in file_list:
-    if os.path.realpath(f) == os.path.realpath(CONFIG_FILE):
-      continue
-    extension = os.path.splitext(f)[1]
-    # Do not examine files which are not likely to actually use CONFIG_xxx.
-    # TODO: this list should be fine tuned.
-    if not extension in ('.c', '.h', '.mk', '.inc') and not 'Makefile' in f:
-      continue
-    with open(f, 'r') as cur_file:
-      line_num = 0
-      for line in cur_file:
-        line_num += 1
-        if extension == '.mk' and line.startswith('#'):
-          # Ignore pattern found in comments. TODO: this needs to be way more
-          # robust: different file extensions require different comment
-          # encapsulation logic, the comment could be not starting in the
-          # first column, etc.
-          continue
-        match = re.search(config_option_re, line)
-        if match:
-          if match.group(1) not in config_options:
+  config_option_re = re.compile(r'\b(CONFIG_[a-zA-Z0-9_]+)')
+  c_style_ext = ('.c', '.h', '.inc', '.S')
+  make_style_ext = ('.mk')
+
+  # Check each hunk's line for a missing config option.
+  for h in hunks:
+    for l in h.lines:
+      # Check for the existence of a CONFIG_* in the line.
+      match = config_option_re.findall(l.string)
+      if not match:
+        continue
+
+      # At this point, an option was found in the string.  However, we need to
+      # verify that it is not within a comment.  Assume every option is a
+      # violation until proven otherwise.
+
+      violations = set()
+      for option in match:
+        violations.add(option)
+
+      # Different files have different comment syntax;  Handle appropriately.
+      extension = os.path.splitext(h.filename)[1]
+      if extension in c_style_ext:
+        beg_comment_idx = l.string.find('/*')
+        end_comment_idx = l.string.find('*/')
+        if end_comment_idx == -1:
+          end_comment_idx = len(l.string)
+        for option in match:
+          option_idx = l.string.find(option)
+          if beg_comment_idx == -1:
+            # Check to see if this line is from a multi-line comment.
+            if l.string.lstrip()[0] == '*':
+              # It _seems_ like it is, therefore ignore this instance.
+              violations.remove(option)
+          else:
+            # Check to see if its actually inside the comment.
+            if beg_comment_idx < option_idx < end_comment_idx:
+              # The config option is in the comment.  Ignore it.
+              violations.remove(option)
+      elif extension in make_style_ext or 'Makefile' in h.filename:
+        beg_comment_idx = l.string.find('#')
+        for option in match:
+          option_idx = l.string.find(option)
+          # Ignore everything to the right of the hash.
+          if beg_comment_idx < option_idx and beg_comment_idx != -1:
+            # The option is within a comment.  Ignore it.
+            violations.remove(option)
+
+      # Check to see if the CONFIG_* option is in the config file and print the
+      # violations.
+      for option in match:
+        if option not in config_options and option in violations:
+          # Print the banner once.
+          if print_banner:
+            print('The following config options were found to be missing '
+                  'from %s.\n'
+                  'Please add new config options there along with '
+                  'descriptions.\n\n' % CONFIG_FILE)
+            print_banner = False
             missing_config_option = True
-            # Print the banner once.
-            if print_banner:
-              print('The following config options were found to be missing '
-                    'from %s.\n'
-                    'Please add new config options there along with '
-                    'descriptions.\n\n' % CONFIG_FILE)
-              print_banner = False
-            # Print the misssing config option.
-            print('> %-*s %s:%s' % (max_option_length, match.group(1), f,
-                                    line_num))
+          # Print the misssing config option.
+          print('> %-*s %s:%s' % (max_option_length, option,
+                                  h.filename,
+                                  l.line_num))
   return missing_config_option
 
+def get_hunks():
+  """Gets the hunks of the most recent commit.
+
+  States:
+    new_file: Searching for a new file in the git diff.
+    filename_search: Searching for the filename of this hunk.
+    hunk: Searching for the beginning of a new hunk.
+    lines: Counting line numbers and searching for changes.
+
+  Returns:
+    hunks: A list of Hunk objects which represent the hunks in the git diff
+      output.
+  """
+
+  # Get the diff output.
+  diff = []
+  hunks = []
+  hunk_lines = []
+  line = ''
+  filename = ''
+  i = 0
+  line_num = 0
+
+  # Regex patterns
+  new_file_re = re.compile(r'^diff --git')
+  filename_re = re.compile(r'^[+]{3} (.*)')
+  hunk_line_num_re = re.compile(r'^@@ -[0-9]+,[0-9]+ \+([0-9]+),[0-9]+ @@.*')
+  line_re = re.compile(r'^([+| ])(.*)')
+
+  cmd = 'git diff --cached -GCONFIG_* --no-prefix --no-ext-diff HEAD~1'
+  diff = subprocess.check_output(cmd.split()).split('\n')
+  line = diff[0]
+  current_state = 'new_file'
+
+  while True:
+    # Search for the beginning of a new file.
+    if current_state is 'new_file':
+      match = new_file_re.search(line)
+      if match:
+        current_state = 'filename_search'
+
+    # Search the diff output for a file name.
+    elif current_state is 'filename_search':
+      # If we're deleting a file, ignore it entirely.
+      if 'deleted' in line:
+        current_state = 'new_file'
+        continue
+
+      # Search for a file name.
+      match = filename_re.search(line)
+      if match:
+        filename = match.groups(1)[0]
+        if filename in WHITELIST:
+          # Skip the file if it's whitelisted.
+          current_state = 'new_file'
+        else:
+          current_state = 'hunk'
+
+    # Search for a hunk.  Each hunk starts with a line describing the line
+    # numbers in the file.
+    elif current_state is 'hunk':
+      hunk_lines = []
+      match = hunk_line_num_re.search(line)
+      if match:
+        # Extract the line number offset.
+        line_num = int(match.groups(1)[0])
+        current_state = 'lines'
+
+    # Start looking for changes.
+    elif current_state is 'lines':
+      # Check if state needs updating.
+      new_hunk = hunk_line_num_re.search(line)
+      new_file = new_file_re.search(line)
+      if new_hunk:
+        current_state = 'hunk'
+        hunks.append(Hunk(filename, hunk_lines))
+        continue
+      elif new_file:
+        current_state = 'new_file'
+        hunks.append(Hunk(filename, hunk_lines))
+        continue
+
+      match = line_re.search(line)
+      if match:
+        # Consider this line iff it's an addition.
+        if match.groups(1)[0] == '+':
+          hunk_lines.append(Line(line_num, match.groups(2)[1]))
+        line_num += 1
+
+    # Advance to the next line
+    try:
+      i += 1
+      line = diff[i]
+    except IndexError:
+      # We've reached the end of the diff.  Return what we have.
+      if hunk_lines:
+        hunks.append(Hunk(filename, hunk_lines))
+      return hunks
+
 def main():
-  """Searches through specified source files for missing config options.
+  """Searches through committed changes for missing config options.
 
-  Checks through specified C source and assembler file (not in the build/ and
-  private/ directories) for CONFIG_* options.  Then checks to make sure that
-  all CONFIG_* options are defined in include/config.h.  Finally, reports any
-  missing config options.  By default, it will check just the files in the CL.
-  To check all files in EC code base, run with the flag --all_files.
+  Checks through committed changes for CONFIG_* options.  Then checks to make
+  sure that all CONFIG_* options used are defined in include/config.h.  Finally,
+  reports any missing config options.
   """
-  # Create argument options to specify checking either all the files in the EC
-  # code base or just the files in the CL.
-  parser = argparse.ArgumentParser(description='configuration option checker.')
-  parser.add_argument('--all_files', help='check all files in EC code base',
-                      action='store_true')
-  args = parser.parse_args()
-
-  # Create list of files to search.
-  file_list = find_files_to_check(args)
+  # Obtain the hunks of the commit to search through.
+  hunks = get_hunks()
   # Obtain config options from include/config.h.
   config_options = obtain_current_config_options()
-  # Find any missing config options from file list and print them.
-  missing_opts = print_missing_config_options(file_list, config_options)
+  # Find any missing config options from the hunks and print them.
+  missing_opts = print_missing_config_options(hunks, config_options)
 
   if missing_opts:
     print('\nIt may also be possible that you have a typo.')