From 3642aa8b5bd249f22f6bd96289fddc62a479d354 Mon Sep 17 00:00:00 2001 From: csh Date: Sat, 11 Jul 2026 18:07:22 +0800 Subject: [PATCH] :recycle: refactor(cli): validate startup arguments --- lsp-server/src/cli/launcher.cppm | 47 ++++- lsp-server/src/utils/args_parser.cppm | 182 +++++++++--------- lsp-server/test/CMakeLists.txt | 8 + .../test/test_args_parser/CMakeLists.txt | 40 ++++ lsp-server/test/test_args_parser/main.cc | 6 + .../test_args_parser/test_args_parser.cppm | 131 +++++++++++++ lsp-server/test/test_cli_startup.py | 60 ++++++ 7 files changed, 368 insertions(+), 106 deletions(-) create mode 100644 lsp-server/test/test_args_parser/CMakeLists.txt create mode 100644 lsp-server/test/test_args_parser/main.cc create mode 100644 lsp-server/test/test_args_parser/test_args_parser.cppm create mode 100644 lsp-server/test/test_cli_startup.py diff --git a/lsp-server/src/cli/launcher.cppm b/lsp-server/src/cli/launcher.cppm index e4361ec..0c4b735 100644 --- a/lsp-server/src/cli/launcher.cppm +++ b/lsp-server/src/cli/launcher.cppm @@ -8,18 +8,43 @@ import std; import lsp.core.server; import lsp.utils.args_parser; +namespace +{ + void SetupLogger(const lsp::utils::ServerConfig& config) + { + spdlog::set_pattern("%Y-%m-%d %H:%M:%S.%e [%t] [%^%l%$] %v"); + auto logger = config.log_file.empty() ? spdlog::stderr_logger_mt("console_logger") : spdlog::basic_logger_mt("file_logger", config.log_file); + logger->set_level(config.log_level); + spdlog::set_default_logger(logger); + spdlog::set_level(config.log_level); + } +} + export int Run(int argc, char* argv[]) { - lsp::utils::ArgsParser& args_parser = lsp::utils::ArgsParser::Instance(); - auto& config = args_parser.Parse(argc, argv); - - if (config.show_help) + auto parsed = lsp::utils::ParseArgs(argc, argv); + if (!parsed) { - lsp::utils::ArgsParser::PrintHelp(argv[0]); + std::cerr << "[TSL-LSP] Argument error: " << parsed.error() << '\n'; + return 2; + } + + if (parsed->action == lsp::utils::ParseAction::kShowHelp) + { + lsp::utils::PrintHelp(std::cout, argv[0]); return 0; } - lsp::utils::ArgsParser::SetupLogger(config); + const auto& config = parsed->config; + try + { + SetupLogger(config); + } + catch (const std::exception& error) + { + std::cerr << "[TSL-LSP] Failed to initialize logger: " << error.what() << '\n'; + return 1; + } try { @@ -27,16 +52,18 @@ export int Run(int argc, char* argv[]) lsp::core::LspServer server(config.thread_count, config.interpreter_path); server.Run(); } - catch (const std::exception& e) + catch (const std::exception& error) { - std::cerr << "[TSL-LSP] Server fatal error: " << e.what() << std::endl; - spdlog::error("Server fatal error: {}", e.what()); + std::cerr << "[TSL-LSP] Server fatal error: " << error.what() << '\n'; + spdlog::error("Server fatal error: {}", error.what()); + spdlog::shutdown(); return 1; } catch (...) { - std::cerr << "[TSL-LSP] Server unknown fatal error" << std::endl; + std::cerr << "[TSL-LSP] Server unknown fatal error\n"; spdlog::error("Server unknown fatal error"); + spdlog::shutdown(); return 1; } diff --git a/lsp-server/src/utils/args_parser.cppm b/lsp-server/src/utils/args_parser.cppm index 98de145..bce8936 100644 --- a/lsp-server/src/utils/args_parser.cppm +++ b/lsp-server/src/utils/args_parser.cppm @@ -1,141 +1,131 @@ module; export module lsp.utils.args_parser; -import spdlog; +import spdlog; import std; export namespace lsp::utils { struct ServerConfig { - bool use_stderr = false; - bool show_help = false; std::size_t thread_count = 4; spdlog::level::level_enum log_level = spdlog::level::info; std::string log_file; std::string interpreter_path; }; - class ArgsParser + enum class ParseAction { - public: - ArgsParser(const ArgsParser&) = delete; - ArgsParser& operator=(const ArgsParser&) = delete; - static ArgsParser& Instance(); - - const ServerConfig& Parse(int argc, char* argv[]); - const ServerConfig& GetConfig() const; - - static void SetupLogger(const ServerConfig& config); - static void PrintHelp(const std::string& program_name); - - private: - ArgsParser() = default; - ~ArgsParser() = default; - - ServerConfig config_; + kRun, + kShowHelp, }; + + struct ParseResult + { + ParseAction action = ParseAction::kRun; + ServerConfig config; + }; + + std::expected ParseArgs(int argc, char* const argv[]); + void PrintHelp(std::ostream& output, std::string_view program_name); } namespace lsp::utils { - ArgsParser& ArgsParser::Instance() + namespace { - static ArgsParser instance; - return instance; + constexpr std::size_t kMinThreadCount = 1; + constexpr std::size_t kMaxThreadCount = 256; + + std::optional ParseLogLevel(std::string_view value) + { + if (value == "trace") + return spdlog::level::trace; + if (value == "debug") + return spdlog::level::debug; + if (value == "info") + return spdlog::level::info; + if (value == "warn") + return spdlog::level::warn; + if (value == "error") + return spdlog::level::err; + if (value == "off") + return spdlog::level::off; + return std::nullopt; + } } - const ServerConfig& ArgsParser::Parse(int argc, char* argv[]) + std::expected ParseArgs(int argc, char* const argv[]) { - config_ = ServerConfig{}; - // Default to stderr so LSP stdio (stdout) stays clean. - config_.use_stderr = true; + for (int i = 1; i < argc; ++i) + { + if (std::string_view(argv[i]) == "--help") + return ParseResult{ .action = ParseAction::kShowHelp, .config = {} }; + } + ParseResult result; + constexpr std::string_view kLogPrefix = "--log="; constexpr std::string_view kLogFilePrefix = "--log-file="; constexpr std::string_view kThreadsPrefix = "--threads="; constexpr std::string_view kInterpreterPrefix = "--interpreter="; for (int i = 1; i < argc; ++i) { - std::string arg = argv[i]; - - if (arg == "--help") + const std::string_view argument = argv[i]; + if (argument.starts_with(kLogPrefix)) { - config_.show_help = true; - return config_; + const auto value = argument.substr(kLogPrefix.size()); + auto level = ParseLogLevel(value); + if (!level) + return std::unexpected("Invalid value for --log: '" + + std::string(value) + "'"); + result.config.log_level = *level; } - - if (arg == "--log=trace") - config_.log_level = spdlog::level::trace; - else if (arg == "--log=debug") - config_.log_level = spdlog::level::debug; - else if (arg == "--log=info") - config_.log_level = spdlog::level::info; - else if (arg == "--log=warn") - config_.log_level = spdlog::level::warn; - else if (arg == "--log=error") - config_.log_level = spdlog::level::err; - else if (arg == "--log=off") - config_.log_level = spdlog::level::off; - else if (arg == "--log-stderr") - config_.use_stderr = true; - else if (arg == "--log-stdout") - config_.use_stderr = false; - else if (arg.starts_with(kLogFilePrefix)) - config_.log_file = arg.substr(kLogFilePrefix.size()); - else if (arg == "--use-stdio") - config_.use_stderr = true; - else if (arg.starts_with(kThreadsPrefix)) + else if (argument.starts_with(kLogFilePrefix)) { - auto value = arg.substr(kThreadsPrefix.size()); - config_.thread_count = std::max(1, static_cast(std::stoi(value))); + const auto value = argument.substr(kLogFilePrefix.size()); + if (value.empty()) + return std::unexpected("--log-file requires a non-empty path"); + result.config.log_file = value; } - else if (arg.starts_with(kInterpreterPrefix)) + else if (argument.starts_with(kThreadsPrefix)) { - config_.interpreter_path = arg.substr(kInterpreterPrefix.size()); + const auto value = argument.substr(kThreadsPrefix.size()); + std::size_t count = 0; + const auto [end, error] = + std::from_chars(value.data(), value.data() + value.size(), count); + if (error != std::errc{} || end != value.data() + value.size() || + count < kMinThreadCount || count > kMaxThreadCount) + { + return std::unexpected("Invalid value for --threads: '" + + std::string(value) + "' (expected 1..256)"); + } + result.config.thread_count = count; + } + else if (argument.starts_with(kInterpreterPrefix)) + { + const auto value = argument.substr(kInterpreterPrefix.size()); + if (value.empty()) + return std::unexpected("--interpreter requires a non-empty path"); + result.config.interpreter_path = value; + } + else + { + return std::unexpected("Unknown argument: " + std::string(argument)); } } - - return config_; + return result; } - const ServerConfig& ArgsParser::GetConfig() const + void PrintHelp(std::ostream& output, std::string_view program_name) { - return config_; - } - - void ArgsParser::SetupLogger(const ServerConfig& config) - { - spdlog::set_pattern("%Y-%m-%d %H:%M:%S.%e [%t] [%^%l%$] %v"); - - if (!config.log_file.empty()) - { - auto file_logger = spdlog::basic_logger_mt("file_logger", config.log_file); - file_logger->set_level(config.log_level); - spdlog::set_default_logger(file_logger); - } - else - { - auto console_logger = config.use_stderr ? spdlog::stderr_logger_mt("console_logger") : spdlog::stdout_logger_mt("console_logger"); - console_logger->set_level(config.log_level); - spdlog::set_default_logger(console_logger); - } - - spdlog::set_level(config.log_level); - } - - void ArgsParser::PrintHelp(const std::string& program_name) - { - std::cout << "Usage: " << program_name << " [options]\\n\\n" - << "Options:\\n" - << " --help Show this help message\\n" - << " --log= Set log level (trace, debug, info, warn, error, off)\\n" - << " --log-stderr Output logs to stderr (default)\\n" - << " --log-stdout Output logs to stdout\\n" - << " --log-file= Output logs to specified file\\n" - << " --use-stdio Alias for --log-stderr (keep stdout clean for LSP)\\n" - << " --threads= Number of worker threads\\n" - << " --interpreter= Custom interpreter path\\n"; + output << "Usage: " << program_name << " [options]\n\n" + << "Options:\n" + << " --help Show this help message\n" + << " --log= Set log level (trace, debug, info, warn, error, off)\n" + << " --log-file= Output logs to specified file\n" + << " --threads= Number of worker threads (1-256)\n" + << " --interpreter= Custom interpreter path\n"; } } diff --git a/lsp-server/test/CMakeLists.txt b/lsp-server/test/CMakeLists.txt index 5606d29..f5c6bb4 100644 --- a/lsp-server/test/CMakeLists.txt +++ b/lsp-server/test/CMakeLists.txt @@ -28,6 +28,10 @@ if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_scheduler/CMakeLists.txt) add_subdirectory(test_scheduler) endif() +if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_args_parser/CMakeLists.txt) + add_subdirectory(test_args_parser) +endif() + if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/test_semantic/CMakeLists.txt) add_subdirectory(test_semantic) endif() @@ -48,6 +52,10 @@ if(BUILD_TESTS) COMMAND ${PYTHON3_EXECUTABLE} ${CMAKE_CURRENT_LIST_DIR}/run_lsp_json_tests.py --server $) + add_test(NAME test_cli_startup + COMMAND ${PYTHON3_EXECUTABLE} + ${CMAKE_CURRENT_LIST_DIR}/test_cli_startup.py + --server $) else() message(WARNING "python3 not found; skipping test_lsp_json registration") endif() diff --git a/lsp-server/test/test_args_parser/CMakeLists.txt b/lsp-server/test/test_args_parser/CMakeLists.txt new file mode 100644 index 0000000..1c0e155 --- /dev/null +++ b/lsp-server/test/test_args_parser/CMakeLists.txt @@ -0,0 +1,40 @@ +cmake_minimum_required(VERSION 4.0) + +project(test_args_parser LANGUAGES CXX) + +set(CMAKE_CXX_STANDARD 23) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_EXPERIMENTAL_CXX_MODULE_DYNDEP 1) + +find_package(spdlog CONFIG REQUIRED) +find_package(fmt CONFIG REQUIRED) + +add_executable(test_args_parser main.cc test_args_parser.cppm) +if(TARGET std_module) + add_dependencies(test_args_parser std_module) +endif() + +target_sources( + test_args_parser + PRIVATE + FILE_SET cxx_modules TYPE CXX_MODULES + BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/../../src + FILES test_args_parser.cppm + ../../src/bridge/spdlog.cppm + ../../src/utils/args_parser.cppm) + +target_compile_definitions(test_args_parser PRIVATE SPDLOG_HEADER_ONLY + FMT_HEADER_ONLY) +target_link_libraries(test_args_parser PRIVATE spdlog::spdlog_header_only + fmt::fmt-header-only) +target_compile_options( + test_args_parser + PRIVATE -Wall -Wextra -Wpedantic + -Wno-import-implementation-partition-unit-in-interface-unit + $<$:-g -O0> + $<$:-O3>) + +if(BUILD_TESTS) + add_test(NAME test_args_parser COMMAND $) +endif() diff --git a/lsp-server/test/test_args_parser/main.cc b/lsp-server/test/test_args_parser/main.cc new file mode 100644 index 0000000..e073a4a --- /dev/null +++ b/lsp-server/test/test_args_parser/main.cc @@ -0,0 +1,6 @@ +import lsp.test.args_parser; + +int main() +{ + return Run(); +} diff --git a/lsp-server/test/test_args_parser/test_args_parser.cppm b/lsp-server/test/test_args_parser/test_args_parser.cppm new file mode 100644 index 0000000..82d9861 --- /dev/null +++ b/lsp-server/test/test_args_parser/test_args_parser.cppm @@ -0,0 +1,131 @@ +module; + +export module lsp.test.args_parser; + +import spdlog; +import std; + +import lsp.utils.args_parser; + +namespace +{ + void Expect(bool condition, std::string_view message) + { + if (!condition) + throw std::runtime_error(std::string(message)); + } + + auto Parse(std::initializer_list arguments) + { + std::vector storage{ "tsl-server" }; + storage.reserve(arguments.size() + 1); + for (auto argument : arguments) + storage.emplace_back(argument); + + std::vector argv; + argv.reserve(storage.size()); + for (auto& argument : storage) + argv.push_back(argument.data()); + + return lsp::utils::ParseArgs(static_cast(argv.size()), argv.data()); + } + + void ExpectError(std::initializer_list arguments, + std::string_view fragment) + { + auto result = Parse(arguments); + Expect(!result.has_value(), "arguments should be rejected"); + Expect(result.error().find(fragment) != std::string::npos, + "error should identify the invalid argument"); + } +} + +export int Run() +{ + int failures = 0; + auto RunCase = [&failures](std::string_view name, auto test) { + try + { + test(); + std::cout << "[PASS] " << name << '\n'; + } + catch (const std::exception& error) + { + ++failures; + std::cout << "[FAIL] " << name << ": " << error.what() << '\n'; + } + }; + + RunCase("defaults", [] { + auto result = Parse({}); + Expect(result.has_value(), "default arguments should parse"); + Expect(result->action == lsp::utils::ParseAction::kRun, + "default action should run"); + Expect(result->config.thread_count == 4, "default thread count should be 4"); + Expect(result->config.log_level == spdlog::level::info, + "default log level should be info"); + }); + + RunCase("valid values and last occurrence", [] { + auto result = Parse({ "--threads=8", "--threads=16", "--log=debug", "--log-file=server.log", "--interpreter=/opt/tsl" }); + Expect(result.has_value(), "valid arguments should parse"); + Expect(result->config.thread_count == 16, "last thread count should win"); + Expect(result->config.log_level == spdlog::level::debug, + "debug log level should parse"); + Expect(result->config.log_file == "server.log", "log file should parse"); + Expect(result->config.interpreter_path == "/opt/tsl", + "interpreter path should parse"); + }); + + RunCase("all log levels", [] { + const std::array levels{ + std::pair{ "trace", spdlog::level::trace }, + std::pair{ "debug", spdlog::level::debug }, + std::pair{ "info", spdlog::level::info }, + std::pair{ "warn", spdlog::level::warn }, + std::pair{ "error", spdlog::level::err }, + std::pair{ "off", spdlog::level::off }, + }; + for (const auto& [name, level] : levels) + { + const std::string argument = std::string("--log=") + name; + auto result = Parse({ argument }); + Expect(result.has_value(), "known log level should parse"); + Expect(result->config.log_level == level, "log level should match"); + } + }); + + RunCase("help takes precedence", [] { + auto result = Parse({ "--unknown", "--help", "--threads=bad" }); + Expect(result.has_value(), "help should bypass other validation"); + Expect(result->action == lsp::utils::ParseAction::kShowHelp, + "help action should be returned"); + }); + + RunCase("thread validation", [] { + Expect(Parse({ "--threads=1" }).has_value(), "one thread should parse"); + Expect(Parse({ "--threads=256" }).has_value(), "256 threads should parse"); + for (auto argument : { "--threads=", "--threads=0", "--threads=-1", "--threads=257", "--threads=12junk", "--threads=18446744073709551616" }) + ExpectError({ argument }, "--threads"); + }); + + RunCase("other invalid values", [] { + for (auto argument : { "--log=verbose", "--log-file=", "--interpreter=", "--log-stdout", "--log-stderr", "--use-stdio", "--unknown" }) + ExpectError({ argument }, "--"); + }); + + RunCase("help formatting", [] { + std::ostringstream output; + lsp::utils::PrintHelp(output, "tsl-server"); + const auto text = output.str(); + Expect(text.contains("Usage: tsl-server [options]\n\nOptions:\n"), + "help should contain physical line breaks"); + Expect(!text.contains("\\n"), "help should not contain escaped newline text"); + Expect(!text.contains("--log-stdout"), "removed stdout flag should be absent"); + Expect(!text.contains("--log-stderr"), "removed stderr flag should be absent"); + Expect(!text.contains("--use-stdio"), "removed stdio alias should be absent"); + }); + + std::cout << "Failures: " << failures << '\n'; + return failures == 0 ? 0 : 1; +} diff --git a/lsp-server/test/test_cli_startup.py b/lsp-server/test/test_cli_startup.py new file mode 100644 index 0000000..6b83826 --- /dev/null +++ b/lsp-server/test/test_cli_startup.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 + +import argparse +import subprocess +import tempfile +import unittest +from pathlib import Path + + +SERVER: Path + + +def run_server(*arguments: str) -> subprocess.CompletedProcess[bytes]: + return subprocess.run( + [str(SERVER), *arguments], + input=b"", + capture_output=True, + check=False, + ) + + +class CliStartupTest(unittest.TestCase): + def test_help_uses_real_newlines(self) -> None: + result = run_server("--help") + self.assertEqual(0, result.returncode) + self.assertIn(b"\n\nOptions:\n", result.stdout) + self.assertNotIn(b"\\n", result.stdout) + + def test_invalid_threads_return_argument_error(self) -> None: + for argument in ("--threads=", "--threads=-1", "--threads=12junk"): + with self.subTest(argument=argument): + result = run_server(argument) + self.assertEqual(2, result.returncode) + self.assertIn(b"--threads", result.stderr) + self.assertNotIn(b"TSL-LSP server starting", result.stderr) + + def test_removed_stdout_flag_is_rejected(self) -> None: + result = run_server("--log-stdout") + self.assertEqual(2, result.returncode) + self.assertEqual(b"", result.stdout) + + def test_default_logger_never_writes_stdout(self) -> None: + result = run_server() + self.assertEqual(0, result.returncode) + self.assertEqual(b"", result.stdout) + self.assertIn(b"TSL-LSP server starting", result.stderr) + + def test_log_file_failure_is_controlled(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir: + result = run_server(f"--log-file={temp_dir}") + self.assertEqual(1, result.returncode) + self.assertIn(b"Failed to initialize logger", result.stderr) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + parser.add_argument("--server", type=Path, required=True) + args, unittest_args = parser.parse_known_args() + SERVER = args.server.resolve() + unittest.main(argv=[__file__, *unittest_args])