[Vim/YCM] Fix hang/crash when no Clang command line is available.

Previously, we tried to determine the command line for building a source
file by looking at the first output of said source file. This doesn't
work if the first output doesn't yield a clang command line. This patch
attempts to resolve this issue by going through all the build outputs of
a source file until one is found that yields a clang command line.

It is still possible to not find a Clang command line. In this case,
patch causes a graceful failure, rather than a crash.

R=jbroman@chromium.org,johnme@chromium.org
BUG=497787

Review URL: https://codereview.chromium.org/1156223007

Cr-Commit-Position: refs/heads/master@{#336467}

5 files changed, 477 insertions, 27 deletions

diff --git a/tools/vim/PRESUBMIT.py b/tools/vim/PRESUBMIT.py
new file mode 100644
index 0000000..8b0e945
--- /dev/null
+++ b/tools/vim/PRESUBMIT.py
@@ -0,0 +1,12 @@
+# Copyright 2015 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.
+
+"""Presubmit tests for /tools/vim.
+
+Runs Python unit tests in /tools/vim/tests on upload.
+"""
+
+def CheckChangeOnUpload(input_api, output_api):
+  return input_api.canned_checks.RunUnitTestsInDirectory(
+      input_api, output_api, 'tests', whitelist=r'.*test.py')
diff --git a/tools/vim/chromium.ycm_extra_conf.py b/tools/vim/chromium.ycm_extra_conf.py
index 5715e29..2145000 100644
--- a/tools/vim/chromium.ycm_extra_conf.py
+++ b/tools/vim/chromium.ycm_extra_conf.py
@@ -169,19 +169,18 @@ def FindChromeSrcFromFilename(filename):
   return curdir
 
 
-def GetDefaultCppFile(chrome_root, filename):
-  """Returns the default target file to use for |filename|.
+def GetDefaultSourceFile(chrome_root, filename):
+  """Returns the default source file to use as an alternative to |filename|.
 
-  The default target is some source file that is known to exist and loosely
-  related to |filename|. Compile flags used to build the default target is
-  assumed to be a close-enough approximation for building |filename|.
+  Compile flags used to build the default source file is assumed to be a
+  close-enough approximation for building |filename|.
 
   Args:
     chrome_root: (String) Absolute path to the root of Chromium checkout.
-    filename: (String) Absolute path to the target source file.
+    filename: (String) Absolute path to the source file.
 
   Returns:
-    (String) Absolute path to substitute target file.
+    (String) Absolute path to substitute source file.
   """
   blink_root = os.path.join(chrome_root, 'third_party', 'WebKit')
   if filename.startswith(blink_root):
@@ -190,15 +189,20 @@ def GetDefaultCppFile(chrome_root, filename):
     return os.path.join(chrome_root, 'base', 'logging.cc')
 
 
-def GetBuildTargetForSourceFile(chrome_root, filename):
-  """Returns a build target corresponding to |filename|.
+def GetBuildableSourceFile(chrome_root, filename):
+  """Returns a buildable source file corresponding to |filename|.
+
+  A buildable source file is one which is likely to be passed into clang as a
+  source file during the build. For .h files, returns the closest matching .cc,
+  .cpp or .c file. If no such file is found, returns the same as
+  GetDefaultSourceFile().
 
   Args:
     chrome_root: (String) Absolute path to the root of Chromium checkout.
     filename: (String) Absolute path to the target source file.
 
   Returns:
-    (String) Absolute path to build target.
+    (String) Absolute path to source file.
   """
   if filename.endswith('.h'):
     # Header files can't be built. Instead, try to match a header file to its
@@ -209,46 +213,104 @@ def GetBuildTargetForSourceFile(chrome_root, filename):
       if os.path.exists(alt_name):
         return alt_name
 
-    # Failing that, build a default file instead and assume that the resulting
-    # commandline options are valid for the .h file.
-    return GetDefaultCppFile(chrome_root, filename)
+    return GetDefaultSourceFile(chrome_root, filename)
 
   return filename
 
 
-def GetClangCommandLineFromNinjaForFilename(out_dir, filename):
-  """Returns the Clang command line for building |filename|
+def GetNinjaBuildOutputsForSourceFile(out_dir, filename):
+  """Returns a list of build outputs for filename.
 
-  Asks ninja for the list of commands used to build |filename| and returns the
-  final Clang invocation.
+  The list is generated by invoking 'ninja -t query' tool to retrieve a list of
+  inputs and outputs of |filename|. This list is then filtered to only include
+  .o and .obj outputs.
 
   Args:
     out_dir: (String) Absolute path to ninja build output directory.
     filename: (String) Absolute path to source file.
 
   Returns:
-    (String) Clang command line or None if command line couldn't be determined.
+    (List of Strings) List of target names. Will return [] if |filename| doesn't
+        yield any .o or .obj outputs.
   """
   # Ninja needs the path to the source file relative to the output build
   # directory.
   rel_filename = os.path.relpath(os.path.realpath(filename), out_dir)
 
-  # Ask ninja how it would build our source file.
-  p = subprocess.Popen(['ninja', '-v', '-C', out_dir, '-t',
-                        'commands', rel_filename + '^'],
+  p = subprocess.Popen(['ninja', '-C', out_dir, '-t', 'query', rel_filename],
+                       stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+  stdout, _ = p.communicate()
+  if p.returncode:
+    return []
+
+  # The output looks like:
+  #   ../../relative/path/to/source.cc:
+  #     outputs:
+  #       obj/reative/path/to/target.source.o
+  #       obj/some/other/target2.source.o
+  #       another/target.txt
+  #
+  outputs_text = stdout.partition('\n  outputs:\n')[2]
+  output_lines = [line.strip() for line in outputs_text.split('\n')]
+  return [target for target in output_lines
+          if target and (target.endswith('.o') or target.endswith('.obj'))]
+
+
+def GetClangCommandLineForNinjaOutput(out_dir, build_target):
+  """Returns the Clang command line for building |build_target|
+
+  Asks ninja for the list of commands used to build |filename| and returns the
+  final Clang invocation.
+
+  Args:
+    out_dir: (String) Absolute path to ninja build output directory.
+    build_target: (String) A build target understood by ninja
+
+  Returns:
+    (String or None) Clang command line or None if a Clang command line couldn't
+        be determined.
+  """
+  p = subprocess.Popen(['ninja', '-v', '-C', out_dir,
+                        '-t', 'commands', build_target],
                        stdout=subprocess.PIPE)
   stdout, stderr = p.communicate()
   if p.returncode:
     return None
 
-  # Ninja might execute several commands to build something. We want the last
-  # clang command.
+  # Ninja will return multiple build steps for all dependencies up to
+  # |build_target|. The build step we want is the last Clang invocation, which
+  # is expected to be the one that outputs |build_target|.
   for line in reversed(stdout.split('\n')):
     if 'clang' in line:
       return line
   return None
 
 
+def GetClangCommandLineFromNinjaForSource(out_dir, filename):
+  """Returns a Clang command line used to build |filename|.
+
+  The same source file could be built multiple times using different tool
+  chains. In such cases, this command returns the first Clang invocation. We
+  currently don't prefer one toolchain over another. Hopefully the tool chain
+  corresponding to the Clang command line is compatible with the Clang build
+  used by YCM.
+
+  Args:
+    out_dir: (String) Absolute path to Chromium checkout.
+    filename: (String) Absolute path to source file.
+
+  Returns:
+    (String or None): Command line for Clang invocation using |filename| as a
+        source. Returns None if no such command line could be found.
+  """
+  build_targets = GetNinjaBuildOutputsForSourceFile(out_dir, filename)
+  for build_target in build_targets:
+    command_line = GetClangCommandLineForNinjaOutput(out_dir, build_target)
+    if command_line:
+      return command_line
+  return None
+
+
 def GetNormalizedClangCommand(command, out_dir):
   """Gets the normalized Clang binary path if |command| is a Clang command.
 
@@ -363,14 +425,17 @@ def GetClangOptionsFromNinjaForFilename(chrome_root, filename):
   from ninja_output import GetNinjaOutputDirectory
   out_dir = os.path.realpath(GetNinjaOutputDirectory(chrome_root))
 
-  clang_line = GetClangCommandLineFromNinjaForFilename(
-      out_dir, GetBuildTargetForSourceFile(chrome_root, filename))
+  clang_line = GetClangCommandLineFromNinjaForSource(
+      out_dir, GetBuildableSourceFile(chrome_root, filename))
   if not clang_line:
     # If ninja didn't know about filename or it's companion files, then try a
     # default build target. It is possible that the file is new, or build.ninja
     # is stale.
-    clang_line = GetClangCommandLineFromNinjaForFilename(
-        out_dir, GetDefaultCppFile(chrome_root, filename))
+    clang_line = GetClangCommandLineFromNinjaForSource(
+        out_dir, GetDefaultSourceFile(chrome_root, filename))
+
+  if not clang_line:
+    return (additional_flags, [])
 
   return GetClangOptionsFromCommandLine(clang_line, out_dir, additional_flags)
 
diff --git a/tools/vim/tests/chromium.ycm_extra_conf_unittest.py b/tools/vim/tests/chromium.ycm_extra_conf_unittest.py
new file mode 100755
index 0000000..b02dcf5
--- /dev/null
+++ b/tools/vim/tests/chromium.ycm_extra_conf_unittest.py
@@ -0,0 +1,320 @@
+#!/usr/bin/env python
+
+# Copyright 2015 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.
+
+"""Tests for chromium.ycm_extra_conf.
+
+These tests should be getting picked up by the PRESUBMIT.py in /tools/vim.
+Currently the tests only run on Linux and require 'ninja' to be available on
+PATH. Due to these requirements, the tests should only be run on upload.
+"""
+
+import imp
+import os
+import shutil
+import stat
+import string
+import subprocess
+import sys
+import tempfile
+import unittest
+
+def CreateFile(path,
+               copy_from = None,
+               format_with = None,
+               make_executable = False):
+  """Creates a file.
+
+  If a file already exists at |path|, it will be overwritten.
+
+  Args:
+    path: (String) Absolute path for file to be created.
+    copy_from: (String or None) Absolute path to source file. If valid, the
+               contents of this file will be written to |path|.
+    format_with: (Dictionary or None) Only valid if |copy_from| is also valid.
+               The contents of the file at |copy_from| will be passed through
+               string.Formatter.vformat() with this parameter as the dictionary.
+    make_executable: (Boolean) If true, |file| will be made executable.
+  """
+  if not os.path.isabs(path):
+    raise Exception(
+        'Argument |path| needs to be an absolute path. Got: "{}"'.format(path))
+  with open(path, 'w') as f:
+    if copy_from:
+      with open(copy_from, 'r') as source:
+        contents = source.read()
+        if format_with:
+          formatter = string.Formatter()
+          contents = formatter.vformat(contents, None, format_with)
+        f.write(contents)
+  if make_executable:
+    statinfo = os.stat(path)
+    os.chmod(path, statinfo.st_mode | stat.S_IXUSR)
+
+@unittest.skipIf(sys.platform.startswith('linux'),
+                 'Tests are only valid on Linux.')
+class Chromium_ycmExtraConfTest_NotOnLinux(unittest.TestCase):
+  def testAlwaysFailsIfNotRunningOnLinux(self):
+    self.fail('Changes to chromium.ycm_extra_conf.py currently need to be ' \
+              'uploaded from Linux since the tests only run on Linux.')
+
+@unittest.skipUnless(sys.platform.startswith('linux'),
+                     'Tests are only valid on Linux.')
+class Chromium_ycmExtraConfTest(unittest.TestCase):
+
+  def SetUpFakeChromeTreeBelowPath(self):
+    """Create fake Chromium source tree under self.test_root.
+
+    The fake source tree has the following contents:
+
+    <self.test_root>
+      |  .gclient
+      |
+      +-- src
+      |   |  DEPS
+      |   |  three.cc
+      |   |
+      |   +-- .git
+      |   |
+      |   +-- include
+      |       |
+      |       +-- a
+      |       |
+      |       +-- b
+      |
+      +-- out
+          |
+          +-- Debug
+                build.ninja
+    """
+    self.chrome_root = os.path.abspath(os.path.normpath(
+        os.path.join(self.test_root, 'src')))
+    self.out_dir = os.path.join(self.chrome_root, 'out', 'Debug')
+
+    os.makedirs(self.chrome_root)
+    os.makedirs(os.path.join(self.chrome_root, '.git'))
+    os.makedirs(self.out_dir)
+    os.makedirs(os.path.join(self.chrome_root, 'include', 'a'))
+    os.makedirs(os.path.join(self.chrome_root, 'include', 'b'))
+
+    CreateFile(os.path.join(self.test_root, '.gclient'))
+    CreateFile(os.path.join(self.chrome_root, 'DEPS'))
+    CreateFile(os.path.join(self.chrome_root, 'three.cc'))
+
+    # Fake ninja build file. Applications of 'cxx' rule are tagged by which
+    # source file was used as input so that the test can verify that the correct
+    # build dependency was used.
+    CreateFile(os.path.join(self.out_dir, 'build.ninja'),
+               copy_from=os.path.join(self.test_data_path,
+                                      'fake_build_ninja.txt'))
+
+    # This is a fake clang++ like script that spits out some directories that
+    # will be interpreted to be system include paths.
+    CreateFile(os.path.join(self.chrome_root, 'fake-clang++'),
+               copy_from=os.path.join(self.test_data_path,
+                                      'fake-clang++.sh'),
+               format_with={'chrome_root': self.chrome_root},
+               make_executable=True)
+
+  def NormalizeString(self, string):
+    return string.replace(self.out_dir, '[OUT]').\
+        replace(self.chrome_root, '[SRC]')
+
+  def NormalizeStringsInList(self, list_of_strings):
+    return [self.NormalizeString(s) for s in list_of_strings]
+
+  def setUp(self):
+    self.actual_chrome_root = os.path.normpath(
+        os.path.join(os.path.dirname(os.path.abspath(__file__)), '../../..'))
+    sys.path.append(os.path.join(self.actual_chrome_root, 'tools', 'vim'))
+    self.test_data_path = os.path.join(self.actual_chrome_root, 'tools', 'vim',
+                                       'tests', 'data')
+    self.ycm_extra_conf = imp.load_source('ycm_extra_conf',
+                                          'chromium.ycm_extra_conf.py')
+    self.test_root = tempfile.mkdtemp()
+    self.SetUpFakeChromeTreeBelowPath()
+
+  def tearDown(self):
+    if self.test_root:
+      shutil.rmtree(self.test_root)
+
+  def testNinjaIsAvailable(self):
+    p = subprocess.Popen(['ninja', '--version'], stdout=subprocess.PIPE)
+    _, _ = p.communicate()
+    self.assertFalse(p.returncode)
+
+  def testFindChromeSrc(self):
+    chrome_source = self.ycm_extra_conf.FindChromeSrcFromFilename(
+        os.path.join(self.chrome_root, 'chrome', 'one.cpp'))
+    self.assertEquals(chrome_source, self.chrome_root)
+
+    chrome_source = self.ycm_extra_conf.FindChromeSrcFromFilename(
+        os.path.join(self.chrome_root, 'one.cpp'))
+    self.assertEquals(chrome_source, self.chrome_root)
+
+  def testCommandLineForKnownCppFile(self):
+    command_line = self.ycm_extra_conf.GetClangCommandLineFromNinjaForSource(
+        self.out_dir, os.path.join(self.chrome_root, 'one.cpp'))
+    self.assertEquals(
+        command_line,
+        '../../fake-clang++ -Ia -Itag-one ../../one.cpp -o obj/one.o')
+
+  def testCommandLineForUnknownCppFile(self):
+    command_line = self.ycm_extra_conf.GetClangCommandLineFromNinjaForSource(
+        self.out_dir, os.path.join(self.chrome_root, 'unknown.cpp'))
+    self.assertEquals(command_line, None)
+
+  def testSystemIncludeDirectoryFlags(self):
+    flags = self.ycm_extra_conf.SystemIncludeDirectoryFlags(
+        os.path.join(self.chrome_root, 'fake-clang++'), [])
+    flags = self.NormalizeStringsInList(flags)
+    self.assertEquals(
+        flags,
+        ['-isystem', '[SRC]/include/a', '-isystem', '[SRC]/include/b'])
+
+  def testGetClangOptionsForKnownCppFile(self):
+    clang_options, sys_includes = \
+        self.ycm_extra_conf.GetClangOptionsFromNinjaForFilename(
+            self.chrome_root, os.path.join(self.chrome_root, 'one.cpp'))
+    self.assertEquals(self.NormalizeStringsInList(clang_options), [
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-one'
+        ])
+    self.assertEquals(self.NormalizeStringsInList(sys_includes), [
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testGetFlagsForFileForKnownCppFile(self):
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'one.cpp'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-one',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testGetFlagsForFileForUnknownCppFile(self):
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'nonexistent.cpp'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-default',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testGetFlagsForFileForUnknownHeaderFile(self):
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'nonexistent.h'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-default',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testGetFlagsForFileForKnownHeaderFileWithAssociatedCppFile(self):
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'three.h'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-three',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testSourceFileWithNonClangOutputs(self):
+    # Verify assumption that four.cc has non-compiler-output listed as the first
+    # output.
+    p = subprocess.Popen(['ninja', '-C', self.out_dir, '-t',
+                          'query', '../../four.cc'],
+                         stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    stdout, _ = p.communicate()
+    self.assertFalse(p.returncode)
+    self.assertEquals(stdout,
+                      '../../four.cc:\n'
+                      '  outputs:\n'
+                      '    obj/linker-output.o\n'
+                      '    obj/four.o\n')
+
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'four.cc'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-four',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+  def testSourceFileWithOnlyNonClangOutputs(self):
+    result = self.ycm_extra_conf.FlagsForFile(
+        os.path.join(self.chrome_root, 'five.cc'))
+    self.assertTrue(result)
+    self.assertTrue('do_cache' in result)
+    self.assertTrue(result['do_cache'])
+    self.assertTrue('flags' in result)
+    self.assertEquals(self.NormalizeStringsInList(result['flags']), [
+        '-DUSE_CLANG_COMPLETER',
+        '-std=c++11',
+        '-x', 'c++',
+        '-I[SRC]',
+        '-Wno-unknown-warning-option',
+        '-I[OUT]/a',
+        '-I[OUT]/tag-default',
+        '-isystem', '[SRC]/include/a',
+        '-isystem', '[SRC]/include/b'
+        ])
+
+if __name__ == '__main__':
+  unittest.main()
diff --git a/tools/vim/tests/data/fake-clang++.sh b/tools/vim/tests/data/fake-clang++.sh
new file mode 100755
index 0000000..0af02ea
--- /dev/null
+++ b/tools/vim/tests/data/fake-clang++.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+# Copyright 2015 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.
+
+echo 'Fake clang'
+echo '#include <...> search starts here:'
+echo ' {chrome_root}/include/a'
+echo ' {chrome_root}/include/b'
+echo 'End of search list.'
+echo 'Some other random junk.'
diff --git a/tools/vim/tests/data/fake_build_ninja.txt b/tools/vim/tests/data/fake_build_ninja.txt
new file mode 100644
index 0000000..0a5aeab
--- /dev/null
+++ b/tools/vim/tests/data/fake_build_ninja.txt
@@ -0,0 +1,41 @@
+# Copyright 2015 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.
+#
+# Fake build.ninja file used by ../chromium.ycm_extra_conf_unittest.py
+
+cxx = ../../fake-clang++
+cxx2 = echo ../../fake-clang++
+link = echo Link
+default_file = ../../base/logging.cc
+
+rule cxx
+  command = $cxx -Ia -Itag-$tag $in -o $out
+
+rule link
+  command = $link $in $out
+
+build obj/one.o: cxx ../../one.cpp
+  tag = one
+
+build extra: phony obj/extra.o
+
+build obj/linker-output.o: link ../../three.cc | ../../four.cc $default_file
+
+build obj/extra.o: cxx ../../extra.cpp
+  tag = extra
+
+build obj/two.o: cxx ../../two.cpp | extra
+  tag = two
+
+build obj/three.o: cxx ../../three.cc
+  tag = three
+
+build obj/four.o: cxx ../../four.cc
+  tag = four
+
+build obj/base.logging.o: cxx $default_file
+  tag = default
+
+build obj/five.o: link ../../five.cc
+