Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixd/Server/configuration: use .nixd.json for workspace/configuration defaults #261

Merged
merged 9 commits into from
Oct 7, 2023
Merged

nixd/Server/configuration: use .nixd.json for workspace/configuration defaults #261

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae7f63f
nixd/Server/configuration: use `nixd.json` for `workspace/configurati…
jonathanjameswatson Sep 18, 2023
8183ba4
lsp: Restore previous `parseParam` behaviour
jonathanjameswatson Oct 6, 2023
500b0b4
lsp: Fix `parseParamWithOptionalDefault` when no default exists
jonathanjameswatson Oct 6, 2023
92a2465
nixd/Server/configuration: Remove `Base` from `mkOutMethod`
jonathanjameswatson Oct 6, 2023
2597d4d
lsp: Fix default constructor being used instead of uninitializing
jonathanjameswatson Oct 6, 2023
80a8716
nixd/Server/configuration: Add wrapper type for JSON value defaulting
jonathanjameswatson Oct 6, 2023
ed79ff5
lsp: Separate includes and use enable_if_t
jonathanjameswatson Oct 7, 2023
d56b61b
nixd/Server/configuration: Organise import of `LSPBinder`
jonathanjameswatson Oct 7, 2023
122d2c2
nixd/Server/configuration: Improve the naming of default config symbols
jonathanjameswatson Oct 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 42 additions & 3 deletions lspserver/include/lspserver/LSPBinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,33 @@
#include <llvm/ADT/FunctionExtras.h>
#include <llvm/ADT/StringMap.h>
#include <llvm/Support/JSON.h>
#include <type_traits>
jonathanjameswatson marked this conversation as resolved.
Show resolved Hide resolved

namespace lspserver {

namespace detail {

template <typename T>
llvm::Expected<T> parseParam(const llvm::json::Value &Raw,
llvm::StringRef PayloadName,
llvm::StringRef PayloadKind) {
typename std::enable_if<std::is_default_constructible_v<T>, T>::type
jonathanjameswatson marked this conversation as resolved.
Show resolved Hide resolved
valueOrUninitialized(const std::optional<T> &OptionalDefault) {
T Result;
if (OptionalDefault) {
Result = OptionalDefault.value();
}
return Result;
}

template <typename T>
typename std::enable_if<!std::is_default_constructible_v<T>, T>::type
jonathanjameswatson marked this conversation as resolved.
Show resolved Hide resolved
valueOrUninitialized(const std::optional<T> &OptionalDefault) {
return OptionalDefault.value();
}

template <typename T>
llvm::Expected<T> parseParamWithOptionalDefault(
const llvm::json::Value &Raw, llvm::StringRef PayloadName,
llvm::StringRef PayloadKind, std::optional<T> OptionalDefault = {}) {
T Result = valueOrUninitialized(OptionalDefault);
llvm::json::Path::Root Root;
if (!fromJSON(Raw, Result, Root)) {
elog("Failed to decode {0} {1}: {2}", PayloadName, PayloadKind,
Expand All @@ -31,6 +50,26 @@ llvm::Expected<T> parseParam(const llvm::json::Value &Raw,
}
return Result;
}

} // namespace detail

template <typename T>
typename std::enable_if<std::is_default_constructible_v<T>,
jonathanjameswatson marked this conversation as resolved.
Show resolved Hide resolved
llvm::Expected<T>>::type
parseParam(const llvm::json::Value &Raw, llvm::StringRef PayloadName,
llvm::StringRef PayloadKind) {
return detail::parseParamWithOptionalDefault<T>(Raw, PayloadName,
PayloadKind);
}

template <typename T>
llvm::Expected<T>
parseParamWithDefault(const llvm::json::Value &Raw, llvm::StringRef PayloadName,
llvm::StringRef PayloadKind, T Default) {
return detail::parseParamWithOptionalDefault<T>(Raw, PayloadName, PayloadKind,
Default);
}

struct HandlerRegistry {
using JSON = llvm::json::Value;
template <typename HandlerT>
Expand Down
9 changes: 8 additions & 1 deletion nixd/include/nixd/Server/Controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@

namespace nixd {

struct OptionalValue {
inclyc marked this conversation as resolved.
Show resolved Hide resolved
std::optional<llvm::json::Value> Value;
};

bool fromJSON(const llvm::json::Value &E, OptionalValue &R, llvm::json::Path P);

/// The server instance, nix-related language features goes here
class Controller : public lspserver::LSPServer {
public:
Expand Down Expand Up @@ -122,6 +128,7 @@ class Controller : public lspserver::LSPServer {

std::mutex ConfigLock;

configuration::TopLevel JSONConfig;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can add some comment here, saying that it is the default value of config

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thoughts, I think I should call it DefaultConfig and then add a comment that says it is set to the contents of .nixd.json if it exists.

configuration::TopLevel Config; // GUARDED_BY(ConfigLock)
inclyc marked this conversation as resolved.
Show resolved Hide resolved

std::shared_ptr<const std::string> getDraft(lspserver::PathRef File) const;
Expand All @@ -135,7 +142,7 @@ class Controller : public lspserver::LSPServer {
PublishDiagnostic;

llvm::unique_function<void(const lspserver::ConfigurationParams &,
lspserver::Callback<configuration::TopLevel>)>
lspserver::Callback<OptionalValue>)>
WorkspaceConfiguration;

std::mutex DiagStatusLock;
Expand Down
37 changes: 27 additions & 10 deletions nixd/lib/Server/Controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "lspserver/LSPBinder.h"
jonathanjameswatson marked this conversation as resolved.
Show resolved Hide resolved
#include "nixd-config.h"

#include "nixd/AST/AttrLocator.h"
Expand Down Expand Up @@ -204,9 +205,15 @@ void Controller::fetchConfig() {
lspserver::ConfigurationParams{
std::vector<lspserver::ConfigurationItem>{
lspserver::ConfigurationItem{.section = "nixd"}}},
[this](llvm::Expected<configuration::TopLevel> Response) {
[this](llvm::Expected<OptionalValue> Response) {
if (Response) {
updateConfig(std::move(Response.get()));
llvm::Expected<configuration::TopLevel> ResponseConfig =
inclyc marked this conversation as resolved.
Show resolved Hide resolved
lspserver::parseParamWithDefault<configuration::TopLevel>(
Response.get().Value.value(), "workspace/configuration",
"reply", JSONConfig);
if (ResponseConfig) {
updateConfig(std::move(ResponseConfig.get()));
}
}
});
}
Expand All @@ -233,9 +240,9 @@ void Controller::readJSONConfig(lspserver::PathRef File) noexcept {
std::ifstream In(File.str(), std::ios::in);
SS << In.rdbuf();

if (auto NewConfig = parseConfig(SS.str()))
updateConfig(std::move(NewConfig.get()));
else {
if (auto NewConfig = parseConfig(SS.str())) {
JSONConfig = std::move(NewConfig.get());
inclyc marked this conversation as resolved.
Show resolved Hide resolved
} else {
throw nix::Error("configuration cannot be parsed");
}
} catch (std::exception &E) {
Expand All @@ -257,6 +264,14 @@ Controller::Controller(std::unique_ptr<lspserver::InboundPort> In,
: LSPServer(std::move(In), std::move(Out)), WaitWorker(WaitWorker),
ASTMgr(Pool) {

// JSON Config, run before initialize
readJSONConfig();
configuration::TopLevel JSONConfigCopy = JSONConfig;
updateConfig(std::move(JSONConfigCopy));
WorkspaceConfiguration =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know why we need to move this statement here? It is just an assignment to WorkspaceConfiguration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have probably overestimated how concurrent the code is, but the reasoning was that the callback to initialized could run before the assignment to WorkspaceConfiguration, making the resulting fetchConfig call fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inclyc Could you confirm whether or not this could happen?

mkOutMethod<lspserver::ConfigurationParams, OptionalValue>(
"workspace/configuration", nullptr);

// Life Cycle
Registry.addMethod("initialize", this, &Controller::onInitialize);
Registry.addNotification("initialized", this, &Controller::onInitialized);
Expand Down Expand Up @@ -293,17 +308,12 @@ Controller::Controller(std::unique_ptr<lspserver::InboundPort> In,
// Workspace
Registry.addNotification("workspace/didChangeConfiguration", this,
&Controller::onWorkspaceDidChangeConfiguration);
WorkspaceConfiguration =
mkOutMethod<lspserver::ConfigurationParams, configuration::TopLevel>(
"workspace/configuration");

/// IPC
Registry.addNotification("nixd/ipc/diagnostic", this,
&Controller::onEvalDiagnostic);

Registry.addNotification("nixd/ipc/finished", this, &Controller::onFinished);

readJSONConfig();
}

//-----------------------------------------------------------------------------/
Expand Down Expand Up @@ -790,4 +800,11 @@ void Controller::onFormat(
};
boost::asio::post(Pool, std::move(Task));
}

bool fromJSON(const llvm::json::Value &E, OptionalValue &R,
llvm::json::Path P) {
R.Value = E;
return true;
}

} // namespace nixd