Fix for hang on insert enter after "foo.bar"

If the user had code like "foo.bar" and then entered insert mode after the 'r'
in "bar", YCM would cause vim to hang.

The problem happened because a sorting task was created that would try to sort
on the latest clang result but none would be created because a clang task was
not created in this occasion. clang_data_ready_ would remain false and would
never be set to true, thus causing an infinite loop in SortingThreadMain since
the thread would forever wait on the mutex.

This was rectified with better handling of the clang results cache. Now the
cache is a full class and it also stores the line & column number of the
location for which the results were computed. Better logic is in place for the
cache invalidation.

cpp/ycm/ClangCompleter.cpp

@@ -18,9 +18,9 @@
 #include "ClangCompleter.h"
 #include "Candidate.h"
 #include "TranslationUnit.h"
-#include "CompletionData.h"
 #include "standard.h"
 #include "CandidateRepository.h"
+#include "CompletionData.h"
 #include "ConcurrentLatestValue.h"
 #include "Utils.h"
 #include "ClangUtils.h"
@@ -28,9 +28,6 @@
 #include <clang-c/Index.h>
 #include <boost/make_shared.hpp>
 
-// TODO: remove all explicit uses of the boost:: prefix by adding explicit using
-// directives for the stuff we need
-namespace fs = boost::filesystem;
 using boost::bind;
 using boost::cref;
 using boost::function;
@@ -52,9 +49,6 @@ using boost::unordered_map;
 namespace YouCompleteMe
 {
 
-typedef function< std::vector< CompletionData >() >
-  FunctionReturnsCompletionDataVector;
-
 extern const unsigned int MAX_ASYNC_THREADS;
 extern const unsigned int MIN_ASYNC_THREADS;
 
@@ -186,8 +180,6 @@ Future< void > ClangCompleter::UpdateTranslationUnitAsync(
   shared_ptr< ClangPackagedTask > clang_packaged_task =
     make_shared< ClangPackagedTask >();
 
-  // TODO: vim hangs when we just open it and try to enter insert mode after the
-  // last '_' in ->parsing_task_ here
   clang_packaged_task->parsing_task_ = packaged_task< void >( functor );
   unique_future< void > future =
     clang_packaged_task->parsing_task_.get_future();
@@ -217,18 +209,21 @@ ClangCompleter::CandidatesForLocationInFile(
 
 Future< AsyncCompletions >
 ClangCompleter::CandidatesForQueryAndLocationInFileAsync(
-    std::string query,
+    const std::string &query,
-    std::string filename,
+    const std::string &filename,
     int line,
     int column,
-    std::vector< UnsavedFile > unsaved_files,
+    const std::vector< UnsavedFile > &unsaved_files,
-    std::vector< std::string > flags )
+    const std::vector< std::string > &flags )
 {
   // TODO: throw exception when threading is not enabled and this is called
   if ( !threading_enabled_ )
     return Future< AsyncCompletions >();
 
-  if ( query.empty() )
+  bool skip_clang_result_cache = ShouldSkipClangResultCache( query,
+                                                             line,
+                                                             column );
+  if ( skip_clang_result_cache )
   {
     // The clang thread is busy, return nothing
     if ( UpdatingTranslationUnit( filename ) )
@@ -248,25 +243,71 @@ ClangCompleter::CandidatesForQueryAndLocationInFileAsync(
   // the sorting task needs to be set before the clang task (if any) just in
   // case the clang task finishes (and therefore notifies a sorting thread to
   // consume a sorting task) before the sorting task is set
+  unique_future< AsyncCompletions > future = CreateSortingTask( query );
 
-  FunctionReturnsCompletionDataVector sort_candidates_for_query_functor =
+  if ( skip_clang_result_cache )
+  {
+    CreateClangTask( filename, line, column, unsaved_files, flags );
+  }
+
+  return Future< AsyncCompletions >( boost::move( future ) );
+}
+
+
+bool ClangCompleter::ShouldSkipClangResultCache( const std::string &query,
+                                                 int line,
+                                                 int column )
+{
+  // We need query.empty() in addition to the second check because if we don't
+  // have it, then we have a problem in the following situation:
+  // The user has a variable 'foo' of type 'A' and a variable 'bar' of type 'B'.
+  // He then types in 'foo.' and we store the clang results and also return
+  // them. The user then deletes that code and types in 'bar.'. Without the
+  // query.empty() check we would return the results from the cache here (it's
+  // the same line and column!) which would be incorrect.
+  return query.empty() || latest_clang_results_
+    .NewPositionDifferentFromStoredPosition( line, column );
+}
+
+
+unique_future< AsyncCompletions > ClangCompleter::CreateSortingTask(
+    const std::string &query )
+{
+  // Careful! The code in this function may burn your eyes.
+
+  function< CompletionDatas( const CompletionDatas& ) >
+    sort_candidates_for_query_functor =
     bind( &ClangCompleter::SortCandidatesForQuery,
           boost::ref( *this ),
           query,
-          boost::cref( latest_clang_results_ ) );
+          _1 );
+
+  function< CompletionDatas() > operate_on_completion_data_functor =
+    bind( &ClangResultsCache::OperateOnCompletionDatas< CompletionDatas >,
+          boost::cref( latest_clang_results_ ),
+          boost::move( sort_candidates_for_query_functor ) );
 
   shared_ptr< packaged_task< AsyncCompletions > > task =
     make_shared< packaged_task< AsyncCompletions > >(
       bind( ReturnValueAsShared< std::vector< CompletionData > >,
-            sort_candidates_for_query_functor ) );
+            boost::move( operate_on_completion_data_functor ) ) );
 
   unique_future< AsyncCompletions > future = task->get_future();
   sorting_task_.Set( task );
+  return future;
+}
 
-  if ( query.empty() )
+
-  {
+void ClangCompleter::CreateClangTask(
-    FunctionReturnsCompletionDataVector
+    std::string filename,
-      candidates_for_location_functor =
+    int line,
+    int column,
+    std::vector< UnsavedFile > unsaved_files,
+    std::vector< std::string > flags )
+{
+    latest_clang_results_.ResetWithNewLineAndColumn( line, column );
+
+    function< CompletionDatas() > candidates_for_location_functor =
       bind( &ClangCompleter::CandidatesForLocationInFile,
             boost::ref( *this ),
             boost::move( filename ),
@@ -283,9 +324,6 @@ ClangCompleter::CandidatesForQueryAndLocationInFileAsync(
               candidates_for_location_functor ) );
 
     clang_task_.Set( clang_packaged_task );
-  }
-
-  return Future< AsyncCompletions >( boost::move( future ) );
 }
 
 
@@ -396,11 +434,7 @@ void ClangCompleter::ClangThreadMain()
 
       unique_future< AsyncCompletions > future =
         task->completions_task_.get_future();
-
+      latest_clang_results_.SetCompletionDatas( *future.get() );
-      {
-        unique_lock< shared_mutex > writer_lock( latest_clang_results_mutex_ );
-        latest_clang_results_ = *future.get();
-      }
 
       {
         lock_guard< mutex > lock( clang_data_ready_mutex_ );
@@ -439,11 +473,7 @@ void ClangCompleter::SortingThreadMain()
       shared_ptr< packaged_task< AsyncCompletions > > task =
         sorting_task_.Get();
 
-      {
+      ( *task )();
-        shared_lock< shared_mutex > reader_lock( latest_clang_results_mutex_ );
-
-        ( *task )();
-      }
     }
 
     catch ( thread_interrupted& )

cpp/ycm/ClangCompleter.h

@@ -22,8 +22,10 @@
 #include "Future.h"
 #include "UnsavedFile.h"
 #include "Diagnostic.h"
+#include "ClangResultsCache.h"
 
 #include <boost/utility.hpp>
+#include <boost/thread/future.hpp>
 #include <boost/unordered_map.hpp>
 
 #include <string>
@@ -38,7 +40,9 @@ class CandidateRepository;
 class TranslationUnit;
 struct CompletionData;
 
-typedef boost::shared_ptr< std::vector< CompletionData > > AsyncCompletions;
+typedef std::vector< CompletionData > CompletionDatas;
+
+typedef boost::shared_ptr< CompletionDatas > AsyncCompletions;
 
 typedef boost::unordered_map< std::string,
           boost::shared_ptr<
@@ -82,15 +86,13 @@ public:
       const std::vector< UnsavedFile > &unsaved_files,
       const std::vector< std::string > &flags );
 
-  // NOTE: params are taken by value on purpose! With a C++11 compiler we can
-  // avoid internal copies if params are taken by value (move ctors FTW)
   Future< AsyncCompletions > CandidatesForQueryAndLocationInFileAsync(
-      std::string query,
+      const std::string &query,
-      std::string filename,
+      const std::string &filename,
       int line,
       int column,
-      std::vector< UnsavedFile > unsaved_files,
+      const std::vector< UnsavedFile > &unsaved_files,
-      std::vector< std::string > flags );
+      const std::vector< std::string > &flags );
 
 private:
 
@@ -109,6 +111,22 @@ private:
   typedef ConcurrentLatestValue<
             boost::shared_ptr< ClangPackagedTask > > LatestClangTask;
 
+  bool ShouldSkipClangResultCache( const std::string &query,
+                                   int line,
+                                   int column );
+
+  boost::unique_future< AsyncCompletions > CreateSortingTask(
+      const std::string &query );
+
+  // NOTE: params are taken by value on purpose! With a C++11 compiler we can
+  // avoid internal copies if params are taken by value (move ctors FTW)
+  void CreateClangTask(
+      std::string filename,
+      int line,
+      int column,
+      std::vector< UnsavedFile > unsaved_files,
+      std::vector< std::string > flags );
+
   boost::shared_ptr< TranslationUnit > GetTranslationUnitForFile(
       const std::string &filename,
       const std::vector< UnsavedFile > &unsaved_files,
@@ -146,8 +164,7 @@ private:
   boost::mutex clang_data_ready_mutex_;
   boost::condition_variable clang_data_ready_condition_variable_;
 
-  std::vector< CompletionData > latest_clang_results_;
+  ClangResultsCache latest_clang_results_;
-  boost::shared_mutex latest_clang_results_mutex_;
 
   // Unfortunately clang is not thread-safe so we need to be careful when we
   // access it. Only one thread at a time is allowed to access any single

cpp/ycm/ClangResultsCache.cpp

@@ -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/>.
+
+#include "ClangResultsCache.h"
+#include "standard.h"
+
+using boost::shared_mutex;
+using boost::shared_lock;
+using boost::unique_lock;
+
+namespace YouCompleteMe
+{
+
+bool ClangResultsCache::NewPositionDifferentFromStoredPosition( int new_line,
+                                                                int new_colum )
+  const
+{
+  shared_lock< shared_mutex > reader_lock( access_mutex_ );
+  return line_ != new_line || column_ != new_colum;
+}
+
+void ClangResultsCache::ResetWithNewLineAndColumn( int new_line, int new_colum )
+{
+  unique_lock< shared_mutex > reader_lock( access_mutex_ );
+
+  line_ = new_line;
+  column_ = new_colum;
+  completion_datas_.clear();
+}
+
+} // namespace YouCompleteMe

cpp/ycm/ClangResultsCache.h

@@ -0,0 +1,83 @@
+// 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 CLANGRESULTSCACHE_H_REUWM3RU
+#define CLANGRESULTSCACHE_H_REUWM3RU
+
+#include "CompletionData.h"
+
+#include <vector>
+#include <boost/thread/locks.hpp>
+#include <boost/thread/shared_mutex.hpp>
+#include <boost/function.hpp>
+#include <boost/config.hpp>
+
+namespace YouCompleteMe
+{
+
+struct CompletionData;
+
+class ClangResultsCache
+{
+public:
+
+  ClangResultsCache() : line_( -1 ), column_( -1 ) {}
+
+  bool NewPositionDifferentFromStoredPosition( int new_line, int new_colum )
+    const;
+
+  void ResetWithNewLineAndColumn( int new_line, int new_colum );
+
+  void SetCompletionDatas( const std::vector< CompletionData > new_completions )
+  {
+    completion_datas_ = new_completions;
+  }
+
+#ifndef BOOST_NO_RVALUE_REFERENCES
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wc++98-compat"
+
+  void SetCompletionDatas( std::vector< CompletionData >&& new_completions )
+  {
+    completion_datas_ = new_completions;
+  }
+
+#pragma clang diagnostic pop
+#endif   //#ifndef BOOST_NO_RVALUE_REFERENCES
+
+  template< typename T >
+  T OperateOnCompletionDatas(
+      boost::function< T( const std::vector< CompletionData >& ) > operation )
+    const
+  {
+    boost::shared_lock< boost::shared_mutex > reader_lock( access_mutex_ );
+    return operation( completion_datas_ );
+  }
+
+private:
+
+  int line_;
+  int column_;
+  std::vector< CompletionData > completion_datas_;
+
+  mutable boost::shared_mutex access_mutex_;
+};
+
+} // namespace YouCompleteMe
+
+#endif /* end of include guard: CLANGRESULTSCACHE_H_REUWM3RU */
+

python/completers/cpp/clang_completer.py

@@ -124,6 +124,7 @@ class ClangCompleter( Completer ):
 
     return self.parse_future.ResultsReady()
 
+
   def GetDiagnosticsForCurrentFile( self ):
     if self.DiagnosticsForCurrentFileReady():
       self.last_diagnostics = [ DiagnosticToDict( x ) for x in