Change extensions enum presubmit checks to run on upload only

tools/strict_enum_value_checker/strict_enum_value_checker.py

+# Copyright 2014 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.
+
+class StrictEnumValueChecker(object):
+  """Verify that changes to enums are valid.
+
+  This class is used to check enums where reordering or deletion is not allowed,
+  and additions must be at the end of the enum, just prior to some "boundary"
+  entry. See comments at the top of the "extension_function_histogram_value.h"
+  file in chrome/browser/extensions for an example what are considered valid
+  changes. There are situations where this class gives false positive warnings,
+  i.e. it warns even though the edit is legitimate. Since the class warns using
+  prompt warnings, the user can always choose to continue. The main point is to
+  attract the attention to all (potentially or not) invalid edits.
+
+  """
+  def __init__(self, input_api, output_api, start_marker, end_marker, path):
+    self.input_api = input_api
+    self.output_api = output_api
+    self.start_marker = start_marker
+    self.end_marker = end_marker
+    self.path = path
+    self.results = []
+
+  class EnumRange(object):
+    """Represents a range of line numbers (1-based)"""
+    def __init__(self, first_line, last_line):
+      self.first_line = first_line
+      self.last_line = last_line
+
+    def Count(self):
+      return self.last_line - self.first_line + 1
+
+    def Contains(self, line_num):
+      return self.first_line <= line_num and line_num <= self.last_line
+
+  def LogInfo(self, message):
+    self.input_api.logging.info(message)
+    return
+
+  def LogDebug(self, message):
+    self.input_api.logging.debug(message)
+    return
+
+  def ComputeEnumRangeInContents(self, contents):
+    """Returns an |EnumRange| object representing the line extent of the
+    enum members in |contents|. The line numbers are 1-based,
+    compatible with line numbers returned by AffectedFile.ChangeContents().
+    |contents| is a list of strings reprenting the lines of a text file.
+
+    If either start_marker or end_marker cannot be found in
+    |contents|, returns None and emits detailed warnings about the problem.
+
+    """
+    first_enum_line = 0
+    last_enum_line = 0
+    line_num = 1  # Line numbers are 1-based
+    for line in contents:
+      if line.startswith(self.start_marker):
+        first_enum_line = line_num + 1
+      elif line.startswith(self.end_marker):
+        last_enum_line = line_num
+      line_num += 1
+
+    if first_enum_line == 0:
+      self.EmitWarning("The presubmit script could not find the start of the "
+                       "enum definition (\"%s\"). Did the enum definition "
+                       "change?" % self.start_marker)
+      return None
+
+    if last_enum_line == 0:
+      self.EmitWarning("The presubmit script could not find the end of the "
+                       "enum definition (\"%s\"). Did the enum definition "
+                       "change?" % self.end_marker)
+      return None
+
+    if first_enum_line >= last_enum_line:
+      self.EmitWarning("The presubmit script located the start of the enum "
+                       "definition (\"%s\" at line %d) *after* its end "
+                       "(\"%s\" at line %d). Something is not quite right."
+                       % (self.start_marker, first_enum_line,
+                          self.end_marker, last_enum_line))
+      return None
+
+    self.LogInfo("Line extent of (\"%s\") enum definition: "
+                 "first_line=%d, last_line=%d."
+                 % (self.start_marker, first_enum_line, last_enum_line))
+    return self.EnumRange(first_enum_line, last_enum_line)
+
+  def ComputeEnumRangeInNewFile(self, affected_file):
+    return self.ComputeEnumRangeInContents(affected_file.NewContents())
+
+  def GetLongMessage(self, local_path):
+    return str("The file \"%s\" contains the definition of the "
+               "(\"%s\") enum which should be edited in specific ways "
+               "only - *** read the comments at the top of the header file ***"
+               ". There are changes to the file that may be incorrect and "
+               "warrant manual confirmation after review. Note that this "
+               "presubmit script can not reliably report the nature of all "
+               "types of invalid changes, especially when the diffs are "
+               "complex. For example, an invalid deletion may be reported "
+               "whereas the change contains a valid rename."
+               % (local_path, self.start_marker))
+
+  def EmitWarning(self, message, line_number=None, line_text=None):
+    """Emits a presubmit prompt warning containing the short message
+    |message|. |item| is |LOCAL_PATH| with optional |line_number| and
+    |line_text|.
+
+    """
+    if line_number is not None and line_text is not None:
+      item = "%s(%d): %s" % (self.path, line_number, line_text)
+    elif line_number is not None:
+      item = "%s(%d)" % (self.path, line_number)
+    else:
+      item = self.path
+    long_message = self.GetLongMessage(self.path)
+    self.LogInfo(message)
+    self.results.append(
+      self.output_api.PresubmitPromptWarning(message, [item], long_message))
+
+  def CollectRangesInsideEnumDefinition(self, affected_file,
+                                        first_line, last_line):
+    """Returns a list of triplet (line_start, line_end, line_text) of ranges of
+    edits changes. The |line_text| part is the text at line |line_start|.
+    Since it used only for reporting purposes, we do not need all the text
+    lines in the range.
+
+    """
+    results = []
+    previous_line_number = 0
+    previous_range_start_line_number = 0
+    previous_range_start_text = ""
+
+    def addRange():
+      tuple = (previous_range_start_line_number,
+               previous_line_number,
+               previous_range_start_text)
+      results.append(tuple)
+
+    for line_number, line_text in affected_file.ChangedContents():
+      if first_line <= line_number and line_number <= last_line:
+        self.LogDebug("Line change at line number " + str(line_number) + ": " +
+                      line_text)
+        # Start a new interval if none started
+        if previous_range_start_line_number == 0:
+          previous_range_start_line_number = line_number
+          previous_range_start_text = line_text
+        # Add new interval if we reached past the previous one
+        elif line_number != previous_line_number + 1:
+          addRange()
+          previous_range_start_line_number = line_number
+          previous_range_start_text = line_text
+        previous_line_number = line_number
+
+    # Add a last interval if needed
+    if previous_range_start_line_number != 0:
+        addRange()
+    return results
+
+  def CheckForFileDeletion(self, affected_file):
+    """Emits a warning notification if file has been deleted """
+    if not affected_file.NewContents():
+      self.EmitWarning("The file seems to be deleted in the changelist. If "
+                       "your intent is to really delete the file, the code in "
+                       "PRESUBMIT.py should be updated to remove the "
+                       "|StrictEnumValueChecker| class.");
+      return False
+    return True
+
+  def GetDeletedLinesFromScmDiff(self, affected_file):
+    """Return a list of of line numbers (1-based) corresponding to lines
+    deleted from the new source file (if they had been present in it). Note
+    that if multiple contiguous lines have been deleted, the returned list will
+    contain contiguous line number entries. To prevent false positives, we
+    return deleted line numbers *only* from diff chunks which decrease the size
+    of the new file.
+
+    Note: We need this method because we have access to neither the old file
+    content nor the list of "delete" changes from the current presubmit script
+    API.
+
+    """
+    results = []
+    line_num = 0
+    deleting_lines = False
+    for line in affected_file.GenerateScmDiff().splitlines():
+      # Parse the unified diff chunk optional section heading, which looks like
+      # @@ -l,s +l,s @@ optional section heading
+      m = self.input_api.re.match(
+        r"^@@ \-([0-9]+)\,([0-9]+) \+([0-9]+)\,([0-9]+) @@", line)
+      if m:
+        old_line_num = int(m.group(1))
+        old_size = int(m.group(2))
+        new_line_num = int(m.group(3))
+        new_size = int(m.group(4))
+        line_num = new_line_num
+        # Return line numbers only from diff chunks decreasing the size of the
+        # new file
+        deleting_lines = old_size > new_size
+        continue
+      if not line.startswith("-"):
+        line_num += 1
+      if deleting_lines and line.startswith("-") and not line.startswith("--"):
+        results.append(line_num)
+    return results
+
+  def CheckForEnumEntryDeletions(self, affected_file):
+    """Look for deletions inside the enum definition. We currently use a
+    simple heuristics (not 100% accurate): if there are deleted lines inside
+    the enum definition, this might be a deletion.
+
+    """
+    range_new = self.ComputeEnumRangeInNewFile(affected_file)
+    if not range_new:
+      return False
+
+    is_ok = True
+    for line_num in self.GetDeletedLinesFromScmDiff(affected_file):
+      if range_new.Contains(line_num):
+        self.EmitWarning("It looks like you are deleting line(s) from the "
+                         "enum definition. This should never happen.",
+                         line_num)
+        is_ok = False
+    return is_ok
+
+  def CheckForEnumEntryInsertions(self, affected_file):
+    range = self.ComputeEnumRangeInNewFile(affected_file)
+    if not range:
+      return False
+
+    first_line = range.first_line
+    last_line = range.last_line
+
+    # Collect the range of changes inside the enum definition range.
+    is_ok = True
+    for line_start, line_end, line_text in \
+          self.CollectRangesInsideEnumDefinition(affected_file,
+                                                 first_line,
+                                                 last_line):
+      # The only edit we consider valid is adding 1 or more entries *exactly*
+      # at the end of the enum definition. Every other edit inside the enum
+      # definition will result in a "warning confirmation" message.
+      #
+      # TODO(rpaquay): We currently cannot detect "renames" of existing entries
+      # vs invalid insertions, so we sometimes will warn for valid edits.
+      is_valid_edit = (line_end == last_line - 1)
+
+      self.LogDebug("Edit range in new file at starting at line number %d and "
+                    "ending at line number %d: valid=%s"
+                    % (line_start, line_end, is_valid_edit))
+
+      if not is_valid_edit:
+        self.EmitWarning("The change starting at line %d and ending at line "
+                         "%d is *not* located *exactly* at the end of the "
+                         "enum definition. Unless you are renaming an "
+                         "existing entry, this is not a valid change, as new "
+                         "entries should *always* be added at the end of the "
+                         "enum definition, right before the \"%s\" "
+                         "entry." % (line_start, line_end, self.end_marker),
+                         line_start,
+                         line_text)
+        is_ok = False
+    return is_ok
+
+  def PerformChecks(self, affected_file):
+    if not self.CheckForFileDeletion(affected_file):
+      return
+    if not self.CheckForEnumEntryDeletions(affected_file):
+      return
+    if not self.CheckForEnumEntryInsertions(affected_file):
+      return
+
+  def ProcessHistogramValueFile(self, affected_file):
+    self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath())
+    self.PerformChecks(affected_file)
+    self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath())
+
+  def Run(self):
+    for file in self.input_api.AffectedFiles(include_deletes=True):
+      if file.LocalPath() == self.path:
+        self.ProcessHistogramValueFile(file)
+    return self.results