Add support for local language configuration #1249

kirawi · 2021-12-11T00:18:57Z

Resolves #1217, allowing users to have configuration for local directories. It defaults to looking for every .helix folder until it finds a .git folder or is at the root, and merges all the configs in order.

helix-term/src/application.rs

helix-core/src/lib.rs

helix-term/src/application.rs

helix-term/src/main.rs

helix-term/src/application.rs

kirawi · 2021-12-19T01:48:41Z

Tested and seems to work.

helix-core/src/lib.rs

helix-term/src/application.rs

elferherrera · 2022-04-16T15:34:56Z

It works for me. I added a new languages.toml file in a .helix folder and it loaded that config. What I dont understand from the PR is if it will override all of one language settings or just sections of it.

Also, could it be discussed to have a window to show LSP loaded parameters?

the-mikedavis · 2022-04-16T16:00:51Z

The merging behavior is on a per-key basis: if you want to turn LSP auto-format off for rust for example, you can have a languages.toml that says

[[language]]
name = "rust"
auto-format = false

And that will override that key but not change other keys like comment-token.

kirawi · 2022-04-17T15:07:46Z

I tested on:

a
├───.git
├───.helix
└───b
    ├───.helix
    └───c
        └───.helix

With:

a/.helix/languages.toml

[[language]]
name = "rust"
auto-format = false
comment-token = "//"
indent = { tab-width = 4, unit = "    " }

b/.helix/languages.toml

[[language]]
name = "rust"
comment-token = "#"
indent = { tab-width = 4, unit = "    " }

c/.helix/languages.toml

[[language]]
name = "rust"
indent = { tab-width = 2, unit = "  " }

correctly becoming

[[language]]
auto-format = false
comment-token = "#"
file-types = ["rs", "rs"]
injection-regex = "rust"
name = "rust"
roots = ["Cargo.toml", "Cargo.lock", "Cargo.toml", "Cargo.lock"]
scope = "source.rust"

[language.indent]
tab-width = 2
unit = "  "

elferherrera · 2022-04-17T16:50:59Z

@kirawi how did you check the final result?

kirawi · 2022-04-17T16:55:21Z

I added log::debug!("{}", config); at https://github.com/helix-editor/helix/pull/1249/files#diff-75a83fbc5b7b51ab917504525e40d867b69ccfc3102d7f7cc1d90281382f5f86R24

elferherrera · 2022-04-17T16:59:06Z

I see. I thought there was an option I didn't know

dead10ck · 2022-04-23T00:36:51Z

helix-term/src/application.rs

@@ -56,15 +56,33 @@ pub struct Application {
 }

 impl Application {
-    pub fn new(args: Args, config: Config) -> Result<Self, Error> {
+    pub fn new(args: Args) -> Result<Self, Error> {


I'm sorry I'm kind of late to the party here. Thanks for the change, @kirawi, this will be super useful!

But this particular part of the change actually presents a problem. I'm working on an integration testing branch, and this change makes it so that you can't pass in a pre-parsed Config object. This was very useful for testing, as it's super easy to just make the struct literal you want to describe the config option you want to test. Now it seems that the config can only come from a toml file. Could we please move this logic back into main?

The documentation of find_root described the following priority for detecting a project root: - Top-most folder containing a root marker in current git repository - Git repository root if no marker detected - Top-most folder containing a root marker if not git repository detected - Current working directory as fallback The commit contained in helix-editor#1249 extracted and changed the implementation of find_root in find_root_impl, actually reversing its result order (since that is the order that made sense for the local configuration merge, from innermost to outermost ancestors). Since the two uses of find_root_impl have different requirements (and it's not a matter of reversing the order of results since, e.g., the top repository dir should be used by find_root only if there's not marker in other dirs), this PR splits the two implementations in two different specialized functions. In doing so, find_root_impl is removed and the implementation is moved back in find_root, moving it closer to the documented behaviour thus making it easier to verify it's actually correct

…3929) * Split helix_core::find_root and helix_loader::find_local_config_dirs The documentation of find_root described the following priority for detecting a project root: - Top-most folder containing a root marker in current git repository - Git repository root if no marker detected - Top-most folder containing a root marker if not git repository detected - Current working directory as fallback The commit contained in #1249 extracted and changed the implementation of find_root in find_root_impl, actually reversing its result order (since that is the order that made sense for the local configuration merge, from innermost to outermost ancestors). Since the two uses of find_root_impl have different requirements (and it's not a matter of reversing the order of results since, e.g., the top repository dir should be used by find_root only if there's not marker in other dirs), this PR splits the two implementations in two different specialized functions. In doing so, find_root_impl is removed and the implementation is moved back in find_root, moving it closer to the documented behaviour thus making it easier to verify it's actually correct * helix-core: remove Option from find_root return type It always returns some result, so Option is not needed

…elix-editor#3929) * Split helix_core::find_root and helix_loader::find_local_config_dirs The documentation of find_root described the following priority for detecting a project root: - Top-most folder containing a root marker in current git repository - Git repository root if no marker detected - Top-most folder containing a root marker if not git repository detected - Current working directory as fallback The commit contained in helix-editor#1249 extracted and changed the implementation of find_root in find_root_impl, actually reversing its result order (since that is the order that made sense for the local configuration merge, from innermost to outermost ancestors). Since the two uses of find_root_impl have different requirements (and it's not a matter of reversing the order of results since, e.g., the top repository dir should be used by find_root only if there's not marker in other dirs), this PR splits the two implementations in two different specialized functions. In doing so, find_root_impl is removed and the implementation is moved back in find_root, moving it closer to the documented behaviour thus making it easier to verify it's actually correct * helix-core: remove Option from find_root return type It always returns some result, so Option is not needed

davidchisnall · 2023-05-27T10:47:47Z

[ Sorry for commenting here, but you appear not to have set up the security tab for reporting potential vulnerabilities ]

If I add a .helix folder in my git repo that sets the language server protocol command to rm -rf ~ then won't a Helix user that opens a file inside a clone of my git repo end up deleting their entire home directory? Most other systems that have similar behaviour have some mitigation such as allowing only commands from an allow list to run or requiring the user to inspect and authorise the config file every time that it changes. I didn't see anything here.

kirawi · 2023-05-27T15:27:36Z

This isn't the appropriate place to make that comment. You should create a separate issue instead.

davidchisnall · 2023-05-28T13:18:17Z

If you do not configure the security settings on GitHub or provide a SECURITY.me, external people have no idea how to report suspected security issues. There was no coordinated disclosure process documented for this project, so I filed it on the place closest to where the problem was introduced. If you want security issues filed in a specific place then I suggest that you document this. Either way, I have reported it and it’s now up to you to choose how to handle it.

pascalkuthe · 2023-05-28T13:26:11Z

I dont think editors have a particular high thread model that would warrant a security policy like that. If you clone untrusted code and run any language server on it you are already vulnerable (rust analyzer runs cargo check which runs build scripts which can run arbitrary untrusted code, same for many other LS) so really any editor that runs language servers is gulernabke. So emacs-lsp mode, nvim and kakune can all be attacked if you open files from a compromised repo.

Either way just like any other problem it should be reported as an issue instead of a random comment on old PR that is likely easily forgotten.

archseer · 2023-05-28T13:33:18Z

I'm currently working on a patch to explicitly approve each new workspace.

kirawi marked this pull request as draft December 11, 2021 00:19

kirawi commented Dec 11, 2021

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch 4 times, most recently from e5eefae to d34a7d9 Compare December 11, 2021 02:45

kirawi changed the title ~~Add support for project-specific language configuration~~ Add support for per-project language configuration Dec 11, 2021

kirawi force-pushed the local branch from d34a7d9 to f6627f5 Compare December 11, 2021 03:59

sudormrfbin reviewed Dec 11, 2021

View reviewed changes

helix-core/src/lib.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch 2 times, most recently from 390792c to c5682a0 Compare December 11, 2021 17:03

kirawi marked this pull request as ready for review December 11, 2021 18:26

pickfire reviewed Dec 12, 2021

View reviewed changes

helix-core/src/lib.rs Outdated Show resolved Hide resolved

pickfire reviewed Dec 12, 2021

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

pickfire reviewed Dec 12, 2021

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch from e8660b8 to 0c2a03e Compare December 12, 2021 21:41

archseer reviewed Dec 13, 2021

View reviewed changes

helix-term/src/main.rs Outdated Show resolved Hide resolved

archseer reviewed Dec 13, 2021

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch from 0c2a03e to d35c15c Compare December 14, 2021 03:48

kirawi commented Dec 14, 2021

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch from d35c15c to 9b00960 Compare December 18, 2021 04:48

sudormrfbin reviewed Dec 19, 2021

View reviewed changes

helix-core/src/lib.rs Outdated Show resolved Hide resolved

helix-term/src/application.rs Outdated Show resolved Hide resolved

kirawi force-pushed the local branch 2 times, most recently from 4460580 to 19a3ca1 Compare December 28, 2021 00:01

kirawi requested a review from archseer December 28, 2021 01:08

pickfire reviewed Jan 2, 2022

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

pickfire reviewed Jan 2, 2022

View reviewed changes

helix-term/src/application.rs Outdated Show resolved Hide resolved

archseer mentioned this pull request Mar 3, 2022

per-project lsp config #1738

Closed

kirawi force-pushed the local branch from 19a3ca1 to 4a33837 Compare March 5, 2022 23:52

archseer approved these changes Apr 18, 2022

View reviewed changes

archseer merged commit c2a40d9 into helix-editor:master Apr 18, 2022

the-mikedavis mentioned this pull request Apr 18, 2022

Override nested arrays when merging TOML #2145

Merged

dead10ck reviewed Apr 23, 2022

View reviewed changes

This was referenced Apr 25, 2022

Write path fixes #2267

Merged

Integration testing harness #2359

Merged

the-mikedavis mentioned this pull request May 12, 2022

feat(languages): add odin language #2399

Merged

rbino mentioned this pull request Sep 21, 2022

Split helix_core::find_root and helix_loader::find_local_config_dirs #3929

Merged

poliorcetics mentioned this pull request Jan 23, 2023

Supports <local path>/.config/helix/... in addition to <local path>/.helix/... #5653

Closed

kirawi mentioned this pull request Jun 9, 2023

Explicit approval for new workspaces #7304

Closed

the-mikedavis mentioned this pull request Feb 3, 2024

Stop silently executing arbitrary code from the current directory by default #9514

Closed

the-dipsy mentioned this pull request Feb 3, 2024

[Security] workspace trust: language servers & tree sitter #2697

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for local language configuration #1249

Add support for local language configuration #1249

kirawi commented Dec 11, 2021 •

edited

Loading

kirawi commented Dec 19, 2021

elferherrera commented Apr 16, 2022 •

edited

Loading

the-mikedavis commented Apr 16, 2022

kirawi commented Apr 17, 2022 •

edited

Loading

elferherrera commented Apr 17, 2022

kirawi commented Apr 17, 2022

elferherrera commented Apr 17, 2022

dead10ck Apr 23, 2022 •

edited

Loading

davidchisnall commented May 27, 2023

kirawi commented May 27, 2023

davidchisnall commented May 28, 2023

pascalkuthe commented May 28, 2023

archseer commented May 28, 2023

Add support for local language configuration #1249

Add support for local language configuration #1249

Conversation

kirawi commented Dec 11, 2021 • edited Loading

kirawi commented Dec 19, 2021

elferherrera commented Apr 16, 2022 • edited Loading

the-mikedavis commented Apr 16, 2022

kirawi commented Apr 17, 2022 • edited Loading

elferherrera commented Apr 17, 2022

kirawi commented Apr 17, 2022

elferherrera commented Apr 17, 2022

dead10ck Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

davidchisnall commented May 27, 2023

kirawi commented May 27, 2023

davidchisnall commented May 28, 2023

pascalkuthe commented May 28, 2023

archseer commented May 28, 2023

kirawi commented Dec 11, 2021 •

edited

Loading

elferherrera commented Apr 16, 2022 •

edited

Loading

kirawi commented Apr 17, 2022 •

edited

Loading

dead10ck Apr 23, 2022 •

edited

Loading