🐛 lsp-proxy ignores configuration

igorlfs commented 1 year ago

Environment information

CLI:
  Version:              11.0.0
  Color support:        true

Platform:
  CPU Architecture:     x86_64
  OS:                   linux

Environment:
  ROME_LOG_DIR:         unset
  NO_COLOR:             unset
  TERM:                 "xterm-256color"

Rome Configuration:
  Status:               loaded
  Formatter disabled:   false
  Linter disabled:      false

Workspace:
  Open Documents:       0

Discovering running Rome servers...

Running Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

i The client isn't connected to any server but rage discovered this running Rome server.

Server:
  Version:              11.0.0
  Name:                 rome_lsp
  CPU Architecture:     x86_64
  OS:                   linux

Workspace:
  Open Documents:       0

Other Active Server Workspaces:

Workspace:
  Open Documents:       2
  Client Name:          Neovim
  Client Version:       0.9.0

Rome Server Log:

! Please review the content of the log file before sharing it publicly as it may contain sensitive information:
  * Path names that may reveal your name, a project name, or the name of your employer.
  * Source code

├─┐rome_lsp::handlers::text_document::did_open{params=DidOpenTextDocumentParams { text_document: TextDocumentItem { uri: Url { scheme: "file", cannot_be_a_base: false, username: "", password: None, host: None, port: None, path: "/home/user/project/file.js", query: None, fragment: None }, language_id: "javascript", version: 0, text: "" } }}
│ ├─┐rome_lsp::session::update_diagnostics{url=file:///home/user/project/file.js}
│ │ ├─┐rome_js_parser::parse::parse{file_id=FileId(1)}
│ │ ├─┘
│ ├─┘
├─┘
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─0ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::rome/rage{params=RageParams}
├─┘
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─0ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::rome/rage{params=RageParams}
├─┘
├─┐rome_lsp::handlers::formatting::format{params=DocumentFormattingParams { text_document: TextDocumentIdentifier { uri: Url { scheme: "file", cannot_be_a_base: false, username: "", password: None, host: None, port: None, path: "/home/user/project/file.js", query: None, fragment: None } }, options: FormattingOptions { tab_size: 4, insert_spaces: true, properties: {}, trim_trailing_whitespace: None, insert_final_newline: None, trim_final_newlines: None }, work_done_progress_params: WorkDoneProgressParams { work_done_token: None } }}
│ ├─0ms DEBUG rome_lsp::handlers::formatting Formatting...
│ ├─┐rome_service::file_handlers::javascript::format{rome_path=RomePath { path: "/home/user/project/file.js", id: FileId(1) }, settings=SettingsHandle { inner: WorkspaceSettings { formatter: FormatSettings { enabled: true, format_with_errors: false, indent_style: Some(Tab), line_width: Some(LineWidth(80)), ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/home/user/project/file.js": false}, poisoned: false, .. } } }, linter: LinterSettings { enabled: true, rules: Some(Rules { recommended: Some(true), a11y: None, complexity: None, correctness: None, nursery: None, performance: None, security: None, style: None, suspicious: None }), ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/home/user/project/file.js": false, "/home/user/project/rome.json": false}, poisoned: false, .. } } }, languages: LanguagesSettings { javascript: LanguageSettings { formatter: JsFormatterSettings { quote_style: None, quote_properties: None, trailing_comma: None, semicolons: None }, linter: JsLinterSettings { globals: [] }, globals: None }, json: LanguageSettings { formatter: (), linter: (), globals: None } }, files: FilesSettings { max_size: 1048576, ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/home/user/project/file.js": false, "/home/user/project/rome.json": false}, poisoned: false, .. } } } } }}
│ │ ├─0ms DEBUG rome_service::file_handlers::javascript Format with the following options: 
│ │ │ Indent style: Tab
│ │ │ Line width: 80
│ │ │ Quote style: Double Quotes
│ │ │ Quote properties: As needed
│ │ │ Trailing comma: All
│ │ │ Semicolons: Always
│ │ │ 
│ │ ├─┐rome_formatter::printer::Printer::print{}
│ │ ├─┘
│ ├─┘
├─┘
├─┐rome_lsp::handlers::text_document::did_change{url=file:///home/user/project/file.js, version=4}
│ ├─0ms ERROR rome_lsp::handlers::text_document error=position Position { line: 247, character: 0 } is out of range
├─┘
├─┐rome_lsp::handlers::text_document::did_change{url=file:///home/user/project/file.js, version=72}
│ ├─0ms ERROR rome_lsp::handlers::text_document error=position Position { line: 245, character: 0 } is out of range
├─┘
├─┐rome_lsp::handlers::text_document::did_change{url=file:///home/user/project/file.js, version=153}
│ ├─0ms ERROR rome_lsp::handlers::text_document error=position Position { line: 117, character: 3 } is out of range
├─
├─260987ms WARN tower_lsp Got a textDocument/didSave notification, but it is not implemented
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─0ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::rome/rage{params=RageParams}
├─┘

What happened?

Create a custom config, in my case:

{
"linter": {
    "enabled": true,
    "rules": {
        "recommended": true
    }
},
"formatter": {
    "enabled": true,
    "indentStyle": "space",
    "indentSize": 4,
    "lineWidth": 120
}
}

Launch neovim with lspconfig (I can provide a minimal configuration if necessary) and try to format the document with :lua vim.lsp.buf.format(). The formatting settings will be ignored.

Expected result

Using rome via lsp-proxy should load the configuration file.

Code of Conduct

[X] I agree to follow Rome's Code of Conduct

benjamineskola commented 1 year ago

I'd been trying to track this down recently too, though I haven't gotten very far (I had initially also confused it with #3507).

I'm also using the Neovim configuration from lspconfig, with no additional configuration options passed.

For comparison, I've found that it does work as a simple filter (:%!rome format --stdin-file-path % or using formatter.nvim or similar), and it also works using coc-rome, though on further investigation I see that the latter doesn't seem to use lsp-proxy.

leops commented 1 year ago

This seems to be an issue with the editor LSP integration, the InitializeParams for the session do not include any of the root_path, root_uri or workspace_folders fields so the server doesn't know where the configuration should be loaded from

benjamineskola commented 1 year ago

I encounter the same issue even when InitializeParams contains correct details. e.g., root_path: Some("/Users/ben/.config"), followed by

├─┐rome_lsp::server::initialized{params=InitializedParams}
│ ├─0ms INFO rome_lsp::server Attempting to load the configuration from 'rome.json' file
│ ├─0ms INFO rome_service::configuration Attempting to load the configuration file at path "/Users/ben/.config/rome.json"
│ ├─┐rome_fs::fs::os::OsFileSystem::open_with_options{path="/Users/ben/.config/rome.json", options=OpenOptions { read: true, write: false, truncate: false, create: false, create_new: false }}
│ ├─┘
│ ├─┐rome_fs::fs::os::OsFile::read_to_string{}
│ ├─┘
│ ├─0ms INFO rome_lsp::session Configuration found, and it is valid!
├─┐rome_lsp::handlers::text_document::did_open{params=DidOpenTextDocumentParams { text_document: TextDocumentItem { uri: Url { scheme: "file", cannot_be_a_base: false, username: "", password: None, host: None, port: None, path: "/Users/ben/.config/test.js", query: None, fragment: None }, language_id: "javascript", version: 0, text: "function foo() {\n\tconsole.log(\"foo\");\n}\n" } }}
│ ├─┐rome_lsp::session::update_diagnostics{url=file:///Users/ben/.config/test.js}
│ │ ├─┐rome_js_parser::parse::parse{file_id=FileId(0)}
│ │ ├─┘
│ ├─┘
├─┘

Yet despite setting indent_style to "space", quote style to single quotes, etc, in /Users/ben/.config/rome.json, I can see in the logs it's ignoring this:

├─┐rome_lsp::handlers::formatting::format{params=DocumentFormattingParams { text_document: TextDocumentIdentifier { uri: Url { scheme: "file", cannot_be_a_base: false, username: "", password: None, host: None, port: None, path: "/Users/ben/.config/test.js", query: None, fragment: None } }, options: FormattingOptions { tab_size: 2, insert_spaces: false, properties: {}, trim_trailing_whitespace: None, insert_final_newline: None, trim_final_newlines: None }, work_done_progress_params: WorkDoneProgressParams { work_done_token: None } }}
│ ├─0ms DEBUG rome_lsp::handlers::formatting Formatting...
│ ├─┐rome_service::file_handlers::javascript::format{rome_path=RomePath { path: "/Users/ben/.config/test.js", id: FileId(0) }, settings=SettingsHandle { inner: WorkspaceSettings { formatter: FormatSettings { enabled: true, format_with_errors: false, indent_style: Some(Tab), line_width: Some(LineWidth(80)), ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/Users/ben/.config/test.js": false}, poisoned: false, .. } } }, linter: LinterSettings { enabled: true, rules: Some(Rules { recommended: Some(true), a11y: None, complexity: None, correctness: None, nursery: None, performance: None, security: None, style: None, suspicious: None }), ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/Users/ben/.config/test.js": false}, poisoned: false, .. } } }, languages: LanguagesSettings { javascript: LanguageSettings { formatter: JsFormatterSettings { quote_style: None, quote_properties: None, trailing_comma: None, semicolons: None }, linter: JsLinterSettings { globals: [] }, globals: None }, json: LanguageSettings { formatter: (), linter: (), globals: None } }, files: FilesSettings { max_size: 1048576, ignored_files: Matcher { patterns: [], options: MatchOptions { case_sensitive: true, require_literal_separator: false, require_literal_leading_dot: false }, already_ignored: RwLock { data: {"/Users/ben/.config/test.js": false}, poisoned: false, .. } } } } }}
│ │ ├─0ms DEBUG rome_service::file_handlers::javascript Format with the following options:
│ │ │ Indent style: Tab
│ │ │ Line width: 80
│ │ │ Quote style: Double Quotes
│ │ │ Quote properties: As needed
│ │ │ Trailing comma: All
│ │ │ Semicolons: Always
│ │ │
│ │ ├─┐rome_formatter::printer::Printer::print{}
│ │ ├─┘
│ ├─┘
├─┘

leops commented 1 year ago

If the configuration file is being loaded correctly, only reason I could think of that would cause it to not be applied is if the editor integration returns an empty response to the workspace/configuration request. Unfortunately that request isn't traced so it doesn't show up in logs, but it looks like a flaw in our configuration loading logic that should be fixed anyway.

xiaoxin-sky commented 1 year ago

lsp-proxy in lapce work normally. see lapce-rome

leops commented 1 year ago

The latest nightly release (11.0.0-nightly.fab5440) contains a refactor of the loading of the configuration among other things, which may fix this issue. If you could confirm that the issue is indeed fixed we could release it on stable too as a new patch version, otherwise it should at least provide more information in the output logs to help debug the problem.

benjamineskola commented 1 year ago

That appears to have worked for me, thanks!

igorlfs commented 1 year ago

The latest nightly also has worked for me!

keturiosakys commented 1 year ago

I'm still getting this issue on 11.0.0-nightly.fab5440 :/. I'm using a workspace setup which I think is part of the problem.

My project looks something like this:

.
├── package-lock.json
├── package.json
├── packages
│  ├── sample
│  └── sample-docs
├── README.md
├── rome.json
└── tsconfig.common.json

as you can see the rome.json sits at the root of the workspace. However, as I run formatting through lsp-proxy in neovim it searches for rome.json in respective packages.

benjamineskola commented 1 year ago

as you can see the rome.json sits at the root of the workspace. However, as I run formatting through lsp-proxy in neovim it searches for rome.json in respective packages.

I wonder if this might actually be the fault of nvim-lspconfig, which looks for rome.json based on the location of package.json. Modifying this function to work based on the location of rome.json might help.

keturiosakys commented 1 year ago

Hmm I don't think so. I was just able to recreate the issue by creating a simple something.ts file at the root of the project and it still did not pick up the rome.json configuration.

keturiosakys commented 1 year ago

Hmm I don't think so. I was just able to recreate the issue by creating a simple something.ts file at the root of the project and it still did not pick up the rome.json configuration.

Scratch that! I was able to fix it by tweaking exactly what @benjamineskola suggested, thank you 🙌.

The following config to the rome lsp worked:

require("lspconfig").rome.setup({
    root_dir = util.root_pattern("rome.json"),
})

I told it to simply look for a rome.json in the root directory as opposed to being clever about figuring it out based on package.json or node_modules, which in a workspaces setup tripped it up.

rome / tools