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

Add not zero check on tab-width. #7094

Closed
So1aric opened this issue May 21, 2023 · 10 comments · Fixed by #7178
Closed

Add not zero check on tab-width. #7094

So1aric opened this issue May 21, 2023 · 10 comments · Fixed by #7178
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much E-good-first-issue Call for participation: Issues suitable for new contributors

Comments

@So1aric
Copy link

So1aric commented May 21, 2023

If set tab-width to zero, helix will crash (divided by zero). The error might occured from the helix-core/grapheme.rs.
Although it's a bit weird to set tab-width to zero (or even negative numbers), presenting the tab-width as 1 and alerting instead of panic would be better, IMO.

@So1aric So1aric added the C-enhancement Category: Improvements label May 21, 2023
@kirawi kirawi added E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements E-good-first-issue Call for participation: Issues suitable for new contributors C-bug Category: This is a bug and removed C-enhancement Category: Improvements labels May 26, 2023
@Niketin
Copy link

Niketin commented May 27, 2023

Hi!

I am trying to get to a point where I could replicate the crash and debug the editor, but I am facing a problem. I am unable to change the tab-width. I am new to Helix, so I might do something wrong.

I configured tab-width to 2 in the ~/.config/helix/languages.toml.

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

Then I reopened the editor (built in debug mode, if it matters), then I opened some Rust source file. The problem is that when I try to indent some selection using the < or >, it still uses 4 spaces. What am I doing wrong here?

@ravsii
Copy link

ravsii commented May 28, 2023

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

@Niketin If this is your EXACT config, then your rust string is opened with a double quotation, but closed with a single one, making the config invalid

@Niketin
Copy link

Niketin commented May 29, 2023

I am sorry @ravsii, it is not the exact config because something weird happened to the text when I copy&pasted it from terminal.

The config should be this, with correct double quotations and without that weird brown highlighted text (if you can see it).

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

@ravsii
Copy link

ravsii commented May 29, 2023

@Niketin I guess this is a rust-only issue, probably because of the cargo fmt whatever default formatter is used for rust, as it uses its own settings and probably should be changed inside the formatter option.

I'd suggest trying js,ts or some other web-related stuff. Working fine in svelte for example. (reproduced the crash as well)
image

@kirawi
Copy link
Member

kirawi commented May 30, 2023

I am sorry @ravsii, it is not the exact config because something weird happened to the text when I copy&pasted it from terminal.

The config should be this, with correct double quotations and without that weird brown highlighted text (if you can see it).

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

It's likely that your languages.toml isn't in the right place for Helix to pick it up.

@gabydd
Copy link
Member

gabydd commented May 30, 2023

it might be picking it up and the auto-indentation detection is just overriding the setting cause the document uses spaces to indent, see src:

/// Detect the indentation used in the file, or otherwise defaults to the language indentation
/// configured in `languages.toml`, with a fallback to tabs if it isn't specified. Line ending
/// is likewise auto-detected, and will fallback to the default OS line ending.
pub fn detect_indent_and_line_ending(&mut self) {

@apanloco
Copy link

you can reproduce this by:
echo "\t" > file.rs
then putting the above languages.toml configuration:

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

followed by
cargo run -- file.rs


 16:        0x103c1c408 - helix_core::graphemes::tab_width_at::hb5c054a65728acf8
                               at /Users/da/code/helix/helix-core/src/graphemes.rs:20:26
image

if the alerting is not important we could do a max(1, tab_width).

@apanloco
Copy link

apanloco commented May 31, 2023

we could nip it in the bud here in document.rs:
image

@gabydd
Copy link
Member

gabydd commented May 31, 2023

The linked pr (#7178) makes it an error to have a non zero tab-width in your config which solves this issue without needing any custom logic

@Niketin
Copy link

Niketin commented Jun 3, 2023

I think it is in the right place, @kirawi. It's located in ~/.config/helix/languages.toml. I have the config file in the same directory and it works fine.

it might be picking it up and the auto-indentation detection is just overriding the setting cause the document uses spaces to indent, see src:

/// Detect the indentation used in the file, or otherwise defaults to the language indentation
/// configured in `languages.toml`, with a fallback to tabs if it isn't specified. Line ending
/// is likewise auto-detected, and will fallback to the default OS line ending.
pub fn detect_indent_and_line_ending(&mut self) {

I believe @gabydd this is why I couldn't reproduce the problem.

you can reproduce this by: echo "\t" > file.rs then putting the above languages.toml configuration:

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

followed by cargo run -- file.rs


 16:        0x103c1c408 - helix_core::graphemes::tab_width_at::hb5c054a65728acf8
                               at /Users/da/code/helix/helix-core/src/graphemes.rs:20:26

@apanloco's instructions worked!

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 E-easy Call for participation: Experience needed to fix: Easy / not much E-good-first-issue Call for participation: Issues suitable for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants