From 61e88737c5c11fbc51671e786663cfc5dcc770e5 Mon Sep 17 00:00:00 2001 From: micbou Date: Thu, 21 Sep 2017 21:00:20 +0200 Subject: [PATCH] Translate libclang error codes to exceptions Do not silence the exceptions. --- cpp/ycm/ClangCompleter/ClangCompleter.cpp | 38 +------------ cpp/ycm/ClangCompleter/ClangUtils.cpp | 24 ++++++++ cpp/ycm/ClangCompleter/ClangUtils.h | 11 ++++ cpp/ycm/ClangCompleter/TranslationUnit.cpp | 39 ++++++------- .../ClangCompleter/TranslationUnitStore.cpp | 5 +- cpp/ycm/exceptions.h | 40 -------------- .../ClangCompleter/ClangCompleter_test.cpp | 55 ------------------- .../ClangCompleter/TranslationUnit_test.cpp | 36 +++++++----- cpp/ycm/ycm_core.cpp | 24 ++++++++ ycmd/tests/clang/get_completions_test.py | 29 +++++++++- 10 files changed, 132 insertions(+), 169 deletions(-) delete mode 100644 cpp/ycm/exceptions.h diff --git a/cpp/ycm/ClangCompleter/ClangCompleter.cpp b/cpp/ycm/ClangCompleter/ClangCompleter.cpp index f43271b88d..618e40dcd0 100644 --- a/cpp/ycm/ClangCompleter/ClangCompleter.cpp +++ b/cpp/ycm/ClangCompleter/ClangCompleter.cpp @@ -16,7 +16,6 @@ // along with ycmd. If not, see . #include "ClangCompleter.h" -#include "exceptions.h" #include "Result.h" #include "Candidate.h" #include "TranslationUnit.h" @@ -83,21 +82,15 @@ std::vector< Diagnostic > ClangCompleter::UpdateTranslationUnit( flags, translation_unit_created ); - if ( !unit ) - return std::vector< Diagnostic >(); - try { return unit->Reparse( unsaved_files ); - } - - catch ( ClangParseError & ) { + } catch ( const ClangParseError & ) { // 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. translation_unit_store_.Remove( filename ); + throw; } - - return std::vector< Diagnostic >(); } @@ -112,9 +105,6 @@ ClangCompleter::CandidatesForLocationInFile( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) - return std::vector< CompletionData >(); - return unit->CandidatesForLocation( line, column, unsaved_files ); @@ -132,10 +122,6 @@ Location ClangCompleter::GetDeclarationLocation( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return Location(); - } - return unit->GetDeclarationLocation( line, column, unsaved_files, reparse ); } @@ -151,10 +137,6 @@ Location ClangCompleter::GetDefinitionLocation( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return Location(); - } - return unit->GetDefinitionLocation( line, column, unsaved_files, reparse ); } @@ -170,10 +152,6 @@ std::string ClangCompleter::GetTypeAtLocation( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return "no unit"; - } - return unit->GetTypeAtLocation( line, column, unsaved_files, reparse ); } @@ -189,10 +167,6 @@ std::string ClangCompleter::GetEnclosingFunctionAtLocation( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return "no unit"; - } - return unit->GetEnclosingFunctionAtLocation( line, column, unsaved_files, @@ -213,10 +187,6 @@ ClangCompleter::GetFixItsForLocationInFile( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return std::vector< FixIt >(); - } - return unit->GetFixItsForLocationInFile( line, column, unsaved_files, @@ -237,10 +207,6 @@ DocumentationData ClangCompleter::GetDocsForLocationInFile( shared_ptr< TranslationUnit > unit = translation_unit_store_.GetOrCreate( filename, unsaved_files, flags ); - if ( !unit ) { - return DocumentationData(); - } - return unit->GetDocsForLocationInFile( line, column, unsaved_files, diff --git a/cpp/ycm/ClangCompleter/ClangUtils.cpp b/cpp/ycm/ClangCompleter/ClangUtils.cpp index 42d6c6365a..46e49ce6d2 100644 --- a/cpp/ycm/ClangCompleter/ClangUtils.cpp +++ b/cpp/ycm/ClangCompleter/ClangUtils.cpp @@ -43,4 +43,28 @@ std::string ClangVersion() { return CXStringToString( clang_getClangVersion() ); } +const char *CXErrorCodeToString( CXErrorCode code ) { + switch ( code ) { + case CXError_Success: + return "No error encountered while parsing the translation unit."; + case CXError_Failure: + return "Failed to parse the translation unit."; + case CXError_Crashed: + return "Libclang crashed while parsing the translation unit."; + case CXError_InvalidArguments: + return "Parsing the translation unit with invalid arguments."; + case CXError_ASTReadError: + return "An AST deserialization error occurred " + "while parsing the translation unit."; + } +} + +ClangParseError::ClangParseError( const char *what_arg ) + : std::runtime_error( what_arg ) { +}; + +ClangParseError::ClangParseError( CXErrorCode code ) + : ClangParseError( CXErrorCodeToString( code ) ) { +}; + } // namespace YouCompleteMe diff --git a/cpp/ycm/ClangCompleter/ClangUtils.h b/cpp/ycm/ClangCompleter/ClangUtils.h index 85260c3e0d..092771d194 100644 --- a/cpp/ycm/ClangCompleter/ClangUtils.h +++ b/cpp/ycm/ClangCompleter/ClangUtils.h @@ -19,6 +19,7 @@ #define CLANGUTILS_H_9MVHQLJS #include +#include #include namespace YouCompleteMe { @@ -37,6 +38,16 @@ std::string CXFileToFilepath( CXFile file ); std::string ClangVersion(); +const char *CXErrorCodeToString( CXErrorCode code ); + +/** + * Thrown when libclang fails to parse (or reparse) the translation unit. + */ +struct ClangParseError : virtual std::runtime_error { + ClangParseError( const char *what_arg ); + ClangParseError( CXErrorCode code ); +}; + } // namespace YouCompleteMe #endif /* end of include guard: CLANGUTILS_H_9MVHQLJS */ diff --git a/cpp/ycm/ClangCompleter/TranslationUnit.cpp b/cpp/ycm/ClangCompleter/TranslationUnit.cpp index 680d8d4e06..4759e472cb 100644 --- a/cpp/ycm/ClangCompleter/TranslationUnit.cpp +++ b/cpp/ycm/ClangCompleter/TranslationUnit.cpp @@ -17,7 +17,6 @@ #include "TranslationUnit.h" #include "CompletionData.h" -#include "exceptions.h" #include "ClangUtils.h" #include "ClangHelpers.h" @@ -94,18 +93,17 @@ TranslationUnit::TranslationUnit( ? &cxunsaved_files[ 0 ] : NULL; // Actually parse the translation unit. - CXErrorCode result = clang_parseTranslationUnit2FullArgv( - clang_index, - filename.c_str(), - &pointer_flags[ 0 ], - pointer_flags.size(), - const_cast( unsaved ), - cxunsaved_files.size(), - EditingOptions(), - &clang_translation_unit_ ); - - if ( result != CXError_Success ) - throw( ClangParseError() ); + CXErrorCode failure = clang_parseTranslationUnit2FullArgv( + clang_index, + filename.c_str(), + &pointer_flags[ 0 ], + pointer_flags.size(), + const_cast( unsaved ), + cxunsaved_files.size(), + EditingOptions(), + &clang_translation_unit_ ); + if ( failure ) + throw ClangParseError( failure ); } @@ -352,7 +350,7 @@ void TranslationUnit::Reparse( // param though. void TranslationUnit::Reparse( std::vector< CXUnsavedFile > &unsaved_files, size_t parse_options ) { - int failure = 0; + CXErrorCode failure; { unique_lock< mutex > lock( clang_access_mutex_ ); @@ -362,15 +360,18 @@ void TranslationUnit::Reparse( std::vector< CXUnsavedFile > &unsaved_files, CXUnsavedFile *unsaved = unsaved_files.size() > 0 ? &unsaved_files[ 0 ] : NULL; - failure = clang_reparseTranslationUnit( clang_translation_unit_, - unsaved_files.size(), - unsaved, - parse_options ); + // This function should technically return a CXErrorCode enum but return an + // int instead. + failure = static_cast< CXErrorCode >( + clang_reparseTranslationUnit( clang_translation_unit_, + unsaved_files.size(), + unsaved, + parse_options ) ); } if ( failure ) { Destroy(); - throw( ClangParseError() ); + throw ClangParseError( failure ); } UpdateLatestDiagnostics(); diff --git a/cpp/ycm/ClangCompleter/TranslationUnitStore.cpp b/cpp/ycm/ClangCompleter/TranslationUnitStore.cpp index f037785054..1c4ce519a1 100644 --- a/cpp/ycm/ClangCompleter/TranslationUnitStore.cpp +++ b/cpp/ycm/ClangCompleter/TranslationUnitStore.cpp @@ -18,7 +18,6 @@ #include "TranslationUnitStore.h" #include "TranslationUnit.h" #include "Utils.h" -#include "exceptions.h" #include @@ -106,9 +105,9 @@ shared_ptr< TranslationUnit > TranslationUnitStore::GetOrCreate( unsaved_files, flags, clang_index_ ); - } catch ( ClangParseError & ) { + } catch ( const ClangParseError & ) { Remove( filename ); - return unit; + throw; } { diff --git a/cpp/ycm/exceptions.h b/cpp/ycm/exceptions.h deleted file mode 100644 index 4d65deed94..0000000000 --- a/cpp/ycm/exceptions.h +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright (C) 2011, 2012 Google Inc. -// -// This file is part of ycmd. -// -// ycmd 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. -// -// ycmd 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 ycmd. If not, see . - -#ifndef EXCEPTIONS_H_3PHJ9YOB -#define EXCEPTIONS_H_3PHJ9YOB - -namespace YouCompleteMe { - -// 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 {}; - -/** - * Thrown when a file does not exist. - */ -struct ClangParseError : virtual ExceptionBase {}; - -} // namespace YouCompleteMe - -#endif /* end of include guard: EXCEPTIONS_H_3PHJ9YOB */ - diff --git a/cpp/ycm/tests/ClangCompleter/ClangCompleter_test.cpp b/cpp/ycm/tests/ClangCompleter/ClangCompleter_test.cpp index 67572503e4..5a1b69d5b3 100644 --- a/cpp/ycm/tests/ClangCompleter/ClangCompleter_test.cpp +++ b/cpp/ycm/tests/ClangCompleter/ClangCompleter_test.cpp @@ -204,59 +204,4 @@ TEST( ClangCompleterTest, GetDocString ) { } } - -TEST( ClangCompleterTest, NoTranslationUnit ) { - ClangCompleter completer; - - const std::string filename; - const std::vector< UnsavedFile > unsaved_files; - const std::vector< std::string > flags; - - EXPECT_EQ( std::vector< Diagnostic >(), - completer.UpdateTranslationUnit( filename, unsaved_files, flags) ); - - EXPECT_EQ( std::vector< CompletionData >(), - completer.CandidatesForLocationInFile( filename, - 1, - 1, - unsaved_files, - flags ) ); - - EXPECT_EQ( Location(), completer.GetDeclarationLocation( filename, - 1, - 1, - unsaved_files, - flags ) ); - EXPECT_EQ( Location(), completer.GetDefinitionLocation( filename, - 1, - 1, - unsaved_files, - flags ) ); - EXPECT_EQ( std::string( "no unit" ), - completer.GetTypeAtLocation( filename, - 1, - 1, - unsaved_files, - flags ) ); - EXPECT_EQ( std::string( "no unit" ), - completer.GetEnclosingFunctionAtLocation( filename, - 1, - 1, - unsaved_files, - flags ) ); - EXPECT_EQ( std::vector< FixIt >(), - completer.GetFixItsForLocationInFile( filename, - 1, - 1, - unsaved_files, - flags ) ); - - EXPECT_EQ( DocumentationData(), - completer.GetDocsForLocationInFile( filename, - 1, - 1, - unsaved_files, - flags ) ); -} - } // namespace YouCompleteMe diff --git a/cpp/ycm/tests/ClangCompleter/TranslationUnit_test.cpp b/cpp/ycm/tests/ClangCompleter/TranslationUnit_test.cpp index 42ce404584..39d6723b22 100644 --- a/cpp/ycm/tests/ClangCompleter/TranslationUnit_test.cpp +++ b/cpp/ycm/tests/ClangCompleter/TranslationUnit_test.cpp @@ -15,9 +15,8 @@ // You should have received a copy of the GNU General Public License // along with ycmd. If not, see . -#include "TranslationUnitStore.h" #include "CompletionData.h" -#include "exceptions.h" +#include "TranslationUnitStore.h" #include "Utils.h" #include #include @@ -54,11 +53,18 @@ TEST_F( TranslationUnitTest, ExceptionThrownOnParseFailure ) { std::vector< std::string > flags; flags.push_back( junk ); - EXPECT_THROW( TranslationUnit( test_file.string(), - std::vector< UnsavedFile >(), - flags, - NULL ), - ClangParseError ); + try { + TranslationUnit( test_file.string(), + std::vector< UnsavedFile >(), + flags, + NULL ); + FAIL() << "Expected ClangParseError exception."; + } catch ( const ClangParseError &error ) { + EXPECT_STREQ( error.what(), + "Parsing the translation unit with invalid arguments." ); + } catch ( ... ) { + FAIL() << "Expected ClangParseError exception."; + } } TEST_F( TranslationUnitTest, GoToDefinitionWorks ) { @@ -142,12 +148,16 @@ TEST_F( TranslationUnitTest, InvalidTranslationUnitStore ) { std::vector< std::string > flags; TranslationUnitStore translation_unit_store{ clang_index_ }; - std::shared_ptr< TranslationUnit > unit = translation_unit_store - .GetOrCreate( filename, - unsaved_files, - flags ); - - EXPECT_EQ( std::shared_ptr< TranslationUnit >(), unit ); + try { + translation_unit_store.GetOrCreate( filename, unsaved_files, flags ); + FAIL() << "Expected ClangParseError exception."; + } catch ( const ClangParseError &error ) { + EXPECT_STREQ( error.what(), + "An AST deserialization error occurred while parsing " + "the translation unit." ); + } catch ( ... ) { + FAIL() << "Expected ClangParseError exception."; + } } diff --git a/cpp/ycm/ycm_core.cpp b/cpp/ycm/ycm_core.cpp index 8016dbd61f..d07135fd48 100644 --- a/cpp/ycm/ycm_core.cpp +++ b/cpp/ycm/ycm_core.cpp @@ -55,6 +55,27 @@ bool HasClangSupport() { } +PyObject* CreatePythonException( const char* name, + PyObject* base_exception = PyExc_Exception ) { + std::string scope_name = extract< std::string >( scope().attr( "__name__" ) ); + std::string qualified_name = scope_name + "." + name; + char *raw_name = const_cast< char * >( qualified_name.c_str() ); + PyObject* object = PyErr_NewException( raw_name, base_exception, NULL ); + scope().attr( name ) = handle<>( borrowed( object ) ); + return object; +} + + +#ifdef USE_CLANG_COMPLETER +PyObject* PyExc_ClangParseError = NULL; + + +void TranslateClangParseError( const ClangParseError &error ) { + PyErr_SetString( PyExc_ClangParseError, error.what() ); +} +#endif // USE_CLANG_COMPLETER + + BOOST_PYTHON_FUNCTION_OVERLOADS( FilterAndSortCandidatesOverloads, FilterAndSortCandidates, 3, 4 ) @@ -95,6 +116,9 @@ BOOST_PYTHON_MODULE(ycm_core) .def( vector_indexing_suite< std::vector< std::string > >() ); #ifdef USE_CLANG_COMPLETER + PyExc_ClangParseError = CreatePythonException( "ClangParseError" ); + register_exception_translator< ClangParseError >( &TranslateClangParseError ); + def( "ClangVersion", ClangVersion ); // CAREFUL HERE! For filename and contents we are referring directly to diff --git a/ycmd/tests/clang/get_completions_test.py b/ycmd/tests/clang/get_completions_test.py index 72d59a60b7..9c30f7c3ba 100644 --- a/ycmd/tests/clang/get_completions_test.py +++ b/ycmd/tests/clang/get_completions_test.py @@ -26,6 +26,7 @@ import json import requests +import ycm_core from nose.tools import eq_ from hamcrest import ( assert_that, contains, contains_inanyorder, empty, has_item, has_items, has_entry, has_entries, @@ -69,7 +70,9 @@ def RunTest( app, test ): 'filepath': PathToTestFile( *extra_conf ) } ) - contents = ReadFile( test[ 'request' ][ 'filepath' ] ) + request = test[ 'request' ] + contents = ( request[ 'contents' ] if 'contents' in request else + ReadFile( request[ 'filepath' ] ) ) def CombineRequest( request, data ): kw = request @@ -81,7 +84,7 @@ def CombineRequest( request, data ): # throws an exception and the easiest way to do that is to throw from # within the FlagsForFile function. app.post_json( '/event_notification', - CombineRequest( test[ 'request' ], { + CombineRequest( request, { 'event_name': 'FileReadyToParse', 'contents': contents, } ), @@ -90,7 +93,7 @@ def CombineRequest( request, data ): # We also ignore errors here, but then we check the response code ourself. # This is to allow testing of requests returning errors. response = app.post_json( '/completions', - CombineRequest( test[ 'request' ], { + CombineRequest( request, { 'contents': contents } ), expect_errors = True ) @@ -962,3 +965,23 @@ def GetCompletions_BracketInclude_AtDirectorySeparator_test( app ): } ) }, } ) + + +@SharedYcmd +def GetCompletions_TranslateClangExceptionToPython_test( app ): + RunTest( app, { + 'description': 'The ClangParseError C++ exception is properly translated ' + 'to a Python exception', + 'extra_conf': [ '.ycm_extra_conf.py' ], + 'request': { + 'filetype' : 'cpp', + 'filepath' : PathToTestFile( 'invalid_file_name' ), + 'contents' : '', + 'force_semantic': True + }, + 'expect': { + 'response': requests.codes.internal_server_error, + 'data': ErrorMatcher( ycm_core.ClangParseError, + "Failed to parse the translation unit." ) + }, + } )