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

Allow tabs for the indentation added by code actions. #684

Merged

Conversation

IntegratedQuantum
Copy link
Contributor

I just tested #673 in my codebase and the indentation was messed up:
Before:

								var index = getIndex(x, y, z);
								var otherIndex = getIndex(otherX, otherY, otherZ);

After:

								var index = getIndex(x, y, z);
        _ = index;
								var otherIndex = getIndex(otherX, otherY, otherZ);

The thing is: My codebase uses tabs (which is allowed as of stage2).

Normally I would have just made an issue about it, but since this is quite easy to fix, I thought I might just do it myself.

…tools#673).

The previous implementation always assumed spaces for indentation.
But in stage2 code may contain tabs for indentation as well.
@nektro
Copy link
Contributor

nektro commented Sep 28, 2022

if tabs are allowed by stage2 then its a bug, theyre supposed to be a hard error

@leecannon
Copy link
Member

https://github.com/ziglang/zig/wiki/FAQ#why-does-zig-force-me-to-use-spaces-instead-of-tabs

Currently, the stage1 parser rejects tabs and the stage2 parser accepts them. What will make it into the final language specification? It isn't decided yet and it doesn't really matter. Just run zig fmt on save.

@nektro
Copy link
Contributor

nektro commented Sep 28, 2022

@llogick
Copy link
Contributor

llogick commented Sep 28, 2022

the spec aside, wouldn't this mean compilation errors for the many who still use stage1 ?

@nektro
Copy link
Contributor

nektro commented Sep 28, 2022

stage1 isnt a concern since stage2 is now the default compiler. tabs do not align to zig fmt as mentioned above so zls has no reason to support this.

@IntegratedQuantum
Copy link
Contributor Author

Andrew stated in #544: "The Zig language accepts hard tabs[...]. The self-hosted compiler implements the Zig language correctly; accepting hard tabs [...]"
And he also stated in zig-spec issue #38: "TAB used as whitespace is unambiguous. It is accepted by the grammar[...]"

So I think it is safe to assumes that tabs for indentation will be allowed and used by some in the foreseeable future.

While zig fmt removes tabs, that's no reason to not support tabs in zls.
zig fmt offers a language-wide style suggestion, which is a good thing, but we must not assume everyone uses the default style. And I think we shouldn't make life harder for people who don't.

@nullptrdevs
This will not cause issues for people who still use stage1. The code changes I made will only add tabs if the programmer already used tabs for indentation in the same file. And if they already used tabs, then they either already have a compiler error, or they are using stage2.

@SuperAuguste
Copy link
Member

I'll weigh in here; I frankly could not care less about what indentation someone uses in Zig. If this improves user experience then I'm all good with it.

Copy link
Member

@SuperAuguste SuperAuguste left a comment

Choose a reason for hiding this comment

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

I really dislike the approach taken here; wouldn't the line slice be guaranteed to simply be the tabs needed for indentation if your document's indentation is set to tabs removing the need for the additional_indent hack? From a cursory glance I believe a solution that like should be effective. :)

@IntegratedQuantum
Copy link
Contributor Author

Yeah I don't really like it either. But in this case I think it is needed:

    fn(unusedVar: xy) void {
        _ = unusedVar; // ← needs more indentation then the previous line.

I could put this inside a function that returns the full indentation, but that would require additional allocations.
So just using 2 separate strings seems like the best solution to me.

@llogick
Copy link
Contributor

llogick commented Sep 30, 2022

Yeah I don't really like it either. But in this case I think it is needed:

    fn(unusedVar: xy) void {
        _ = unusedVar; // ← needs more indentation then the previous line.

I could put this inside a function that returns the full indentation, but that would require additional allocations. So just using 2 separate strings seems like the best solution to me.

That does make sense.. what if the last param of createDiscardText be a bool, do_add_block_indent or block_indent_needed
and have the logic for that there? (not allocations, the var additional_indent = ..

@IntegratedQuantum
Copy link
Contributor Author

That does make sense.. what if the last param of createDiscardText be a bool, do_add_block_indent or block_indent_needed and have the logic for that there? (not allocations, the var additional_indent = ..

Yeah, I thought about that, but to figure out if tabs are used I need the entire file(The function in question might be in file scope, without indentation, so I cannot determine if tabs are used from just the indentation of the function declaration). So in that case I would need two new parameters instead of one.

But now that I think about it, I might just pass the builder in there instead of passing only the allocator.

I also noticed another issue, that I'll try to fix:

someOtherCode();var x: u8 = 0; // ← sadly this is allowed in zig.
                _ = x; // ← previous implementation (not that bad)
someOtherCode();_ = x; // ← my implementation (bad)

Even worse for const z = struct{fn x(y: u8) void {}};

…) and move the indentation code inside `createDiscardText()`.
@IntegratedQuantum
Copy link
Contributor Author

Done.
I also made it work with other indentations (some people prefer 2 spaces for indentation).

@SuperAuguste
Copy link
Member

Please resolve conflicts :)

@IntegratedQuantum
Copy link
Contributor Author

I fixed the merge conflicts like a week ago. I'm guessing you didn't get a notification for that?

@SuperAuguste
Copy link
Member

Sorry in the middle of midterms and work, LGTM.

@SuperAuguste SuperAuguste merged commit 6286119 into zigtools:master Oct 13, 2022
@IntegratedQuantum IntegratedQuantum deleted the fix_code_actions_indentation branch October 13, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants