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.

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

Incorrectly merging built-in and user config languages.toml files for typescript lsp #1000

Closed
sno2 opened this issue Nov 7, 2021 · 3 comments · Fixed by #1004, #2145 or #2215
Closed

Incorrectly merging built-in and user config languages.toml files for typescript lsp #1000

sno2 opened this issue Nov 7, 2021 · 3 comments · Fixed by #1004, #2145 or #2215
Assignees
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@sno2
Copy link
Contributor

sno2 commented Nov 7, 2021

Reproduction steps

You need to have deno installed to be able to reproduce and consult wiki for helix config/log directories.

  1. Create an empty languages.toml in the Helix config directory.
  2. Copy in the text from helix/languages.toml
  3. Navigate to the language entry that has a name of typescript
  4. Change the language-server value to be of { command = "deno", args = ["lsp"] } and save
  5. Create a dummy file with a ts file extension
  6. Insert the following code:
console.log("Hello, world!");
  1. Close helix
  2. Open the file in helix again with the -v flag
  3. Open the helix log file and see the errors listed below.

Environment

  • Platform: Windows
  • Helix version: 0.5
C:\Users\carte\AppData\Local\helix\helix.log
2021-11-06T21:20:10.786 helix_lsp::transport [INFO] -> {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["","quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}}},"completion":{"completionItem":{"snippetSupport":false},"completionItemKind":{}},"hover":{"contentFormat":["markdown"]}},"window":{"workDoneProgress":true}},"processId":20380,"rootUri":null},"id":0}
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "\u{1b}[0m\u{1b}[1m\u{1b}[31merror\u{1b}[0m: Found argument '--stdio' which wasn't expected, or isn't valid in this context\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "USAGE:\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "    deno [OPTIONS] [SUBCOMMAND]\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "For more information try --help\n"
2021-11-06T21:20:10.870 helix_lsp::transport [ERROR] err <- "\n"
2021-11-06T21:20:10.876 helix_lsp::transport [ERROR] err: <- StreamClosed
2021-11-06T21:20:10.876 helix_lsp::transport [ERROR] err: <- StreamClosed
@sno2 sno2 added the C-bug Category: This is a bug label Nov 7, 2021
@kirawi kirawi added the A-helix-term Area: Helix term improvements label Nov 7, 2021
@sno2
Copy link
Contributor Author

sno2 commented Nov 7, 2021

You can confirm this bug by replacing the merge_toml_tests module in helix-core/lib.rs with the following code:

#[cfg(test)]
mod merge_toml_tests {
    use super::merge_toml_values;

    #[test]
    fn language_tomls() {
        use toml::Value;

        const USER: &str = r#"
        [[language]]
        name = "typescript"
        test = "bbb"
        indent = { tab-width = 4, unit = "    ", test = "aaa" }
        language-server = { command = "deno", args = ["lsp"] }
        "#;

        let base: Value = toml::from_slice(include_bytes!("../../languages.toml"))
            .expect("Couldn't parse built-in langauges config");
        let user: Value = toml::from_str(USER).unwrap();

        let merged = merge_toml_values(base, user);
        let languages = merged.get("language").unwrap().as_array().unwrap();
        let ts = languages
            .iter()
            .find(|v| v.get("name").unwrap().as_str().unwrap() == "typescript")
            .unwrap();
        let ts_indent = ts.get("indent").unwrap();

        // We changed tab-width and unit in indent so check them if they are the new values
        assert_eq!(ts_indent.get("tab-width").unwrap().as_integer().unwrap(), 4);
        assert_eq!(ts_indent.get("unit").unwrap().as_str().unwrap(), "    ");
        // We added a new keys, so check them
        assert_eq!(ts.get("test").unwrap().as_str().unwrap(), "bbb");
        assert_eq!(ts_indent.get("test").unwrap().as_str().unwrap(), "aaa");
        assert_eq!(
            ts.get("language-server")
                .unwrap()
                .get("args")
                .unwrap()
                .as_array()
                .unwrap(),
            &vec![Value::String("lsp".into())],
        );
    }
}

It will then give you this error in the tests:

failures:

---- merge_toml_tests::language_tomls stdout ----
thread 'merge_toml_tests::language_tomls' panicked at 'assertion failed: `(left == right)`
  left: `[String("--stdio"), String("lsp")]`,
 right: `[String("lsp")]`', helix-core\src\lib.rs:178:9


failures:
    merge_toml_tests::language_tomls

@archseer
Copy link
Member

archseer commented Nov 9, 2021

Need to reopen this sadly as I had to revert #1004. It fixed the bug, but it stopped merging the built in languages.toml with the user overrides, so all grammars had to be specified in the user config. We want to merge the toplevel languages array, but replace any nested arrays.

@archseer archseer closed this as completed Nov 9, 2021
@kirawi kirawi reopened this Nov 10, 2021
the-mikedavis added a commit to the-mikedavis/helix that referenced this issue Apr 17, 2022
We merge the elements of arrays for the top-level array. For
`languages.toml`, this is the array of languages. For any nested
arrays, we simply take the `right` array as-is instead of using
the union of `left` and `right`.

closes helix-editor#1000
the-mikedavis added a commit to the-mikedavis/helix that referenced this issue Apr 17, 2022
We merge the elements of arrays for the top-level array. For
`languages.toml`, this is the array of languages. For any nested
arrays, we simply take the `right` array as-is instead of using
the union of `left` and `right`.

closes helix-editor#1000
the-mikedavis added a commit to the-mikedavis/helix that referenced this issue Apr 18, 2022
We merge the elements of arrays for the top-level array. For
`languages.toml`, this is the array of languages. For any nested
arrays, we simply take the `right` array as-is instead of using
the union of `left` and `right`.

closes helix-editor#1000
archseer pushed a commit that referenced this issue Apr 20, 2022
We merge the elements of arrays for the top-level array. For
`languages.toml`, this is the array of languages. For any nested
arrays, we simply take the `right` array as-is instead of using
the union of `left` and `right`.

closes #1000
@the-mikedavis the-mikedavis reopened this Apr 21, 2022
@the-mikedavis
Copy link
Member

#2145 had to be reverted as well for other merging bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
4 participants