Handling clang parse errors instead of segfaulting

cpp/ycm/ClangCompleter.cpp

@@ -16,6 +16,7 @@
 // along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
 
 #include "ClangCompleter.h"
+#include "exceptions.h"
 #include "Candidate.h"
 #include "TranslationUnit.h"
 #include "standard.h"
@@ -159,9 +160,24 @@ void ClangCompleter::UpdateTranslationUnit(
   shared_ptr< TranslationUnit > unit = GetTranslationUnitForFile( filename,
                                                                   unsaved_files,
                                                                   flags );
-  X_ASSERT( unit );
-  // TODO: only do this if the unit was not just created
-  unit->Reparse( unsaved_files );
+  if ( !unit )
+    return;
+
+  try
+  {
+    // TODO: only do this if the unit was not just created
+    unit->Reparse( unsaved_files );
+  }
+
+  catch ( ClangParseError& )
+  {
+    lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+
+    // If unit->Reparse fails, then the underlying TranslationUnit object is not
+    // valid anymore and needs to be destroyed and removed from the filename ->
+    // TU map.
+    Erase( filename_to_translation_unit_, filename );
+  }
 }
 
 
@@ -200,7 +216,9 @@ ClangCompleter::CandidatesForLocationInFile(
   shared_ptr< TranslationUnit > unit = GetTranslationUnitForFile( filename,
                                                                   unsaved_files,
                                                                   flags );
-  X_ASSERT( unit );
+  if ( !unit )
+    return std::vector< CompletionData >();
+
   return unit->CandidatesForLocation( line,
                                       column,
                                       unsaved_files );
@@ -349,9 +367,18 @@ shared_ptr< TranslationUnit > ClangCompleter::GetTranslationUnitForFile(
       return it->second;
   }
 
+  shared_ptr< TranslationUnit > unit;
 
-  shared_ptr< TranslationUnit > unit = make_shared< TranslationUnit >(
-      filename, unsaved_files, flags, clang_index_ );
+  try
+  {
+    unit = make_shared< TranslationUnit >(
+        filename, unsaved_files, flags, clang_index_ );
+  }
+
+  catch ( ClangParseError& )
+  {
+    return unit;
+  }
 
   {
     lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );

cpp/ycm/TranslationUnit.cpp

@@ -18,6 +18,7 @@
 #include "TranslationUnit.h"
 #include "CompletionData.h"
 #include "standard.h"
+#include "exceptions.h"
 #include "ClangUtils.h"
 
 #include <clang-c/Index.h>
@@ -35,7 +36,8 @@ TranslationUnit::TranslationUnit(
     const std::vector< UnsavedFile > &unsaved_files,
     const std::vector< std::string > &flags,
     CXIndex clang_index )
-  : filename_( filename )
+  : filename_( filename ),
+    clang_translation_unit_( NULL )
 {
   std::vector< const char* > pointer_flags;
   pointer_flags.reserve( flags.size() );
@@ -48,8 +50,6 @@ TranslationUnit::TranslationUnit(
   std::vector< CXUnsavedFile > cxunsaved_files = ToCXUnsavedFiles(
       unsaved_files );
 
-  // TODO: check return value of the parse* and reparse* functions! Throw an
-  // exception on problems.
   clang_translation_unit_ = clang_parseTranslationUnit(
       clang_index,
       filename.c_str(),
@@ -59,19 +59,19 @@ TranslationUnit::TranslationUnit(
       cxunsaved_files.size(),
       clang_defaultEditingTranslationUnitOptions() );
 
+  if ( !clang_translation_unit_ )
+    boost_throw( ClangParseError() );
+
   // Only with a reparse is the preable precompiled. I do not know why...
   // TODO: report this bug on the clang tracker
-  clang_reparseTranslationUnit(
-      clang_translation_unit_,
-      cxunsaved_files.size(),
-      &cxunsaved_files[ 0 ],
-      clang_defaultEditingTranslationUnitOptions() );
+  Reparse( cxunsaved_files );
 }
 
 
 TranslationUnit::~TranslationUnit()
 {
-  clang_disposeTranslationUnit( clang_translation_unit_ );
+  if ( clang_translation_unit_ )
+    clang_disposeTranslationUnit( clang_translation_unit_ );
 }
 
 
@@ -104,18 +104,10 @@ bool TranslationUnit::IsCurrentlyUpdating() const
 
 void TranslationUnit::Reparse( const std::vector< UnsavedFile > &unsaved_files )
 {
-  unique_lock< mutex > lock( clang_access_mutex_ );
-
   std::vector< CXUnsavedFile > cxunsaved_files = ToCXUnsavedFiles(
       unsaved_files );
 
-  clang_reparseTranslationUnit(
-      clang_translation_unit_,
-      cxunsaved_files.size(),
-      &cxunsaved_files[ 0 ],
-      clang_defaultEditingTranslationUnitOptions() );
-
-  UpdateLatestDiagnostics();
+  Reparse( cxunsaved_files );
 }
 
 
@@ -126,6 +118,9 @@ std::vector< CompletionData > TranslationUnit::CandidatesForLocation(
 {
   unique_lock< mutex > lock( clang_access_mutex_ );
 
+  if ( !clang_translation_unit_ )
+    return std::vector< CompletionData >();
+
   std::vector< CXUnsavedFile > cxunsaved_files = ToCXUnsavedFiles(
       unsaved_files );
 
@@ -154,6 +149,34 @@ std::vector< CompletionData > TranslationUnit::CandidatesForLocation(
 }
 
 
+// Argument taken as non-const ref because we need to be able to pass a
+// non-const pointer to clang. This function (and clang too) will not modify the
+// param though.
+void TranslationUnit::Reparse(
+    std::vector< CXUnsavedFile > &unsaved_files )
+{
+  unique_lock< mutex > lock( clang_access_mutex_ );
+
+  if ( !clang_translation_unit_ )
+    return;
+
+  int failure = clang_reparseTranslationUnit(
+      clang_translation_unit_,
+      unsaved_files.size(),
+      &unsaved_files[ 0 ],
+      clang_defaultEditingTranslationUnitOptions() );
+
+  if ( failure )
+  {
+    clang_disposeTranslationUnit( clang_translation_unit_ );
+    clang_translation_unit_ = NULL;
+    boost_throw( ClangParseError() );
+  }
+
+  UpdateLatestDiagnostics();
+}
+
+
 // Should only be called while holding the clang_access_mutex_
 void TranslationUnit::UpdateLatestDiagnostics()
 {

cpp/ycm/TranslationUnit.h

@@ -31,6 +31,7 @@
 
 typedef void *CXIndex;
 typedef struct CXTranslationUnitImpl *CXTranslationUnit;
+struct CXUnsavedFile;
 
 namespace YouCompleteMe
 {
@@ -63,6 +64,8 @@ public:
 
 private:
 
+  void Reparse( std::vector< CXUnsavedFile > &unsaved_files );
+
   void UpdateLatestDiagnostics();
 
   /////////////////////////////

cpp/ycm/Utils.h

@@ -64,6 +64,19 @@ FindWithDefault( Container &container,
   return it != container.end() ? it->second : value;
 }
 
+
+template <class Container, class Key>
+bool Erase( Container &container, const Key &key )
+{
+  typename Container::iterator it = container.find( key );
+  if ( it != container.end() )
+  {
+    container.erase( it );
+    return true;
+  }
+  return false;
+}
+
 } // namespace YouCompleteMe
 
 #endif /* end of include guard: UTILS_H_KEPMRPBH */

cpp/ycm/exceptions.h

@@ -0,0 +1,45 @@
+// Copyright (C) 2011, 2012  Strahinja Val Markovic  <val@markovic.io>
+//
+// This file is part of YouCompleteMe.
+//
+// YouCompleteMe is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// YouCompleteMe is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
+
+#ifndef EXCEPTIONS_H_3PHJ9YOB
+#define EXCEPTIONS_H_3PHJ9YOB
+
+#include <boost/exception/all.hpp>
+
+namespace YouCompleteMe
+{
+
+#define boost_throw(x) BOOST_THROW_EXCEPTION(x)
+
+// YouCompleteMe uses the "Exception types as semantic tags" idiom.
+// For more information, see this link:
+//   http://www.boost.org/doc/libs/1_50_0/libs/exception/doc/exception_types_as_simple_semantic_tags.html
+
+/**
+ * The common base for all exceptions.
+ */
+struct ExceptionBase: virtual std::exception, virtual boost::exception {};
+
+/**
+ * Thrown when a file does not exist.
+ */
+struct ClangParseError : virtual ExceptionBase {};
+
+} // namespace YouCompleteMe
+
+#endif /* end of include guard: EXCEPTIONS_H_3PHJ9YOB */
+

cpp/ycm/tests/ClangCompleter_test.cpp

@@ -70,5 +70,3 @@ TEST( ClangCompleterTest, CandidatesForQueryAndLocationInFileAsync )
 }
 
 } // namespace YouCompleteMe
-
-

cpp/ycm/tests/TranslationUnit_test.cpp

@@ -0,0 +1,50 @@
+// Copyright (C) 2011, 2012  Strahinja Val Markovic  <val@markovic.io>
+//
+// This file is part of YouCompleteMe.
+//
+// YouCompleteMe is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// YouCompleteMe is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
+
+#include "TranslationUnit.h"
+#include "exceptions.h"
+#include "Utils.h"
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <boost/filesystem.hpp>
+namespace fs = boost::filesystem;
+
+using ::testing::ElementsAre;
+using ::testing::WhenSorted;
+
+namespace YouCompleteMe
+{
+
+TEST( TranslationUnitTest, ExceptionThrownOnParseFailure )
+{
+  fs::path test_file = fs::temp_directory_path() / fs::unique_path();
+  std::string junk = "#&9112(^(^#>@(^@!@(&#@a}}}}{nthoeu\n&&^^&^&!#%%@@!aeu";
+  WriteUtf8File( test_file, junk );
+
+  std::vector< std::string > flags;
+  flags.push_back( junk );
+
+  EXPECT_THROW( TranslationUnit( test_file.string(),
+                                 std::vector< UnsavedFile >(),
+                                 flags,
+                                 NULL ),
+                ClangParseError );
+}
+
+
+} // namespace YouCompleteMe