diff --git a/lix/libexec/launch-builder-linux.cc b/lix/libexec/launch-builder-linux.cc index c12e414af..45c4086c1 100644 --- a/lix/libexec/launch-builder-linux.cc +++ b/lix/libexec/launch-builder-linux.cc @@ -59,7 +59,7 @@ static void setPersonality(std::string_view system) bool pathExists(const fs::path & path) { - return fs::exists(fs::symlink_status(path)); + return fs::exists(fs::symlink_status(path)); // NOLINT(lix-foreign-exceptions): caught by main } void bindPath(const fs::path & source, const fs::path & target, bool optional = false) @@ -72,7 +72,7 @@ void bindPath(const fs::path & source, const fs::path & target, bool optional = } }; - auto st = fs::symlink_status(source); + auto st = fs::symlink_status(source); // NOLINT(lix-foreign-exceptions): caught by main if (st.type() == fs::file_type::not_found) { if (optional) { return; @@ -82,14 +82,14 @@ void bindPath(const fs::path & source, const fs::path & target, bool optional = } if (st.type() == fs::file_type::directory) { - fs::create_directories(target); + fs::create_directories(target); // NOLINT(lix-foreign-exceptions): caught by main bindMount(); } else if (st.type() == fs::file_type::symlink) { // Symlinks can (apparently) not be bind-mounted, so just copy it - fs::create_directories(target.parent_path()); - fs::copy_symlink(source, target); + fs::create_directories(target.parent_path()); // NOLINT(lix-foreign-exceptions): caught by main + fs::copy_symlink(source, target); // NOLINT(lix-foreign-exceptions): caught by main } else { - fs::create_directories(target.parent_path()); + fs::create_directories(target.parent_path()); // NOLINT(lix-foreign-exceptions): caught by main if (kj::AutoCloseFd file{open(target.c_str(), O_RDWR | O_CREAT, 0644)}; file == nullptr) { throw SysError("could not create %s", target); } diff --git a/lix/libutil/file-system.cc b/lix/libutil/file-system.cc index 8702968a7..ca8f599f0 100644 --- a/lix/libutil/file-system.cc +++ b/lix/libutil/file-system.cc @@ -833,7 +833,9 @@ void moveFile(const Path & oldName, const Path & newName) // For the move to be as atomic as possible, copy to a temporary // directory fs::path temp = createTempSubdir(newPath.parent_path(), "rename-tmp"); - Finally removeTemp = [&]() { fs::remove(temp); }; + Finally removeTemp = [&]() { + fs::remove(temp); // NOLINT(lix-foreign-exceptions): lint doesn't see the outer try + }; auto tempCopyTarget = temp / "copy-target"; fs::remove(newPath); printTaggedWarning("Can’t rename %s as %s, copying instead", oldName, newName); diff --git a/subprojects/lix-clang-tidy/ForeignExceptions.cc b/subprojects/lix-clang-tidy/ForeignExceptions.cc index 7d440c87f..9437fe7f7 100644 --- a/subprojects/lix-clang-tidy/ForeignExceptions.cc +++ b/subprojects/lix-clang-tidy/ForeignExceptions.cc @@ -1,11 +1,13 @@ #include "ForeignExceptions.hh" #include #include +#include #include #include #include #include #include +#include #include namespace nix::clang_tidy { @@ -53,6 +55,47 @@ void ForeignExceptions::registerMatchers(ast_matchers::MatchFinder *Finder) { unless(anyOf(isDefaultConstructor(), isCopyConstructor(), isMoveConstructor()))))) .bind("bad-ctor")), this); + + auto std_path = cxxRecordDecl(hasName("std::filesystem::path")); + auto fs_error = cxxRecordDecl(hasName("std::filesystem::filesystem_error")); + auto std_exception = cxxRecordDecl(hasName("std::exception")); + Finder->addMatcher( + traverse( + clang::TK_AsIs, + callExpr( + forFunction(functionDecl().bind("fn")), + callee(functionDecl( + hasAncestor(namespaceDecl(hasName("std::filesystem"))), + unless(anyOf( + // noexcept functions are obviously fine + isNoThrow(), + // this wants to match ostream << path. templates! + allOf( + hasOverloadedOperatorName("<<"), + hasParameter(0, hasType(references(cxxRecordDecl( + hasName("std::basic_ostream"))))), + hasParameter(1, hasType(references(std_path)))), + // path / path is fine too, obviously + allOf(hasOverloadedOperatorName("/"), + hasParameter(0, hasType(references(std_path))), + hasParameter(1, hasType(references(std_path)))))))), + // we just allow fs::path because it is allowed to throw some + // implementation-defined exceptions, none of which we can in + // any reasonable way handle in generic code. c++ is *great*. + unless(callee(cxxMethodDecl(ofClass(std_path)))), + // allow calls if they're wrapped in a proper try/catch. this + // doesn't look through immediate-call lambdas by choice; the + // check would be incomprehensible if we checked lambdas too. + unless(hasAncestor(cxxTryStmt( + forFunction(functionDecl(equalsBoundNode("fn"))), + has(cxxCatchStmt(anyOf( + isCatchAll(), + // allow foreign catches to silence the call site. we + // will still warn for the *catch* site later though. + has(varDecl(hasType(references(fs_error)))), + has(varDecl(hasType(references(std_exception))))))))))) + .bind("bad-call")), + this); } void ForeignExceptions::check( @@ -81,6 +124,10 @@ void ForeignExceptions::check( "%0 throws non-Lix exceptions. Ensure that they are caught and wrapped " "properly, ideally by wrapping the constructor invocation itself.") << ctor->getConstructor()->getNameAsString(); + } else if (const auto *call = Result.Nodes.getNodeAs("bad-call")) { + diag(call->getExprLoc(), "throws non-Lix exceptions. Ensure that they are " + "caught and wrapped properly.") + << call->getSourceRange(); } else { llvm_unreachable("bad match"); }