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

error for incorrectly indented code / misleading indentation #35

Open
andrewrk opened this issue Dec 15, 2015 · 13 comments
Open

error for incorrectly indented code / misleading indentation #35

andrewrk opened this issue Dec 15, 2015 · 13 comments
Labels
accepted This proposal is planned. frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Dec 15, 2015

If any line in a block has indentation, then all lines in the block must have the same indentation.

Additionally, the indentation level of a block must be greater than or equal to indentation level of the parent block.

@andrewrk andrewrk added the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label Dec 15, 2015
@andrewrk andrewrk changed the title error for misaligned code error for incorrectly indented code Jan 2, 2016
@andrewrk andrewrk mentioned this issue Oct 26, 2016
12 tasks
@andrewrk andrewrk added this to the 0.2.0 milestone Mar 26, 2017
@andrewrk andrewrk modified the milestones: 0.2.0, 0.3.0 Nov 14, 2017
@andrewrk andrewrk modified the milestones: 0.3.0, 0.4.0 Feb 28, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Sep 28, 2018
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Apr 9, 2019
@andrewrk andrewrk added accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. and removed enhancement Solving this issue will likely involve adding new logic or components to the codebase. labels Jul 6, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Oct 2, 2019
@andrewrk andrewrk added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Nov 27, 2019
@hryx
Copy link
Contributor

hryx commented Jan 21, 2020

While I agree with the outcome, wouldn't this mean the language becomes whitespace-sensitive for the first time? That is, if this is part of the spec and not just a compiler feature.

It's different from disallowing hard tabs because \t is just an illegal byte. zig fmt accepts and fixes tabs so that the parser can remain simpler, whereas it seems like this will introduce more bookkeeping into the parser.

I wonder if zig fmt or zig fmt --exit-non-zero-when-code-aint-pretty should be in charge of this?

@andrewrk
Copy link
Member Author

@hryx I think it's important to make this part of the language spec. It makes this example invalid according to the language specification:

if (match_cond)
    x.removeThing(y);
    x.swap(z);

Which means that the above code is not valid Zig.

@pixelherodev
Copy link
Contributor

Is this meant to be implemented for stage1 as well?

@jakwings
Copy link

jakwings commented May 17, 2020

Quoted from #4294

I'm confident in this decision.

Any reason for the public eyes?

If I have to choose both terseness and safety, I would prefer enforcing brackets on multi-line if(), which is much better for reading code although one may make the same old mistake when writing code and you may wonder wheter it was caused by code reformater or something else.

@emekoi
Copy link
Contributor

emekoi commented May 19, 2020

this sounds like something that zig fmt should be in charge of. while zig doesn't have warnings,

if (match_cond)
    x.removeThing(y);
    x.swap(z);

seems more appropriate as a warning (or a note) rather than an error, or the sort of thing that should go in a zig style guide.

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 9, 2020
andrewrk added a commit that referenced this issue Feb 22, 2021
After #35 is implemented,
we should be able to recover from this *at any indentation level*,
reporting a parse error and yet also parsing all the decls even
inside structs. Until then, I don't want to add any hacks to make
this work.
@Hadron67
Copy link
Contributor

One non-problem problem: Could this be unfriendly for code generators that output zig code since they have to take care of the identation?

@andrewrk
Copy link
Member Author

No because they can completely omit indentation without breaking this rule. The error only happens if the indentation is incorrectly out dented.

But also, Zig is primarily meant to be a human friendly source format. The use case of generated zig code takes a back seat to human maintenance.

@pfgithub
Copy link
Contributor

An alternative that would fix this same issue as @iology pointed out: requiring the expression of an if statement to start on the same line as the ) of the if statement

if(…) …; // allowed
if(…)
    …; // not allowed
if(…) ( // allowed
    …
);
if(…) { // allowed
    …
}
if(…)
{ // not allowed

}

and the same with other blocks

for(…) |value, i| …; // allowed
for(…) |value, i|
    …; // not allowed
for(…)
    |value, i| …; // not allowed
for(…) |value, i| { // allowed
    …
}
for(…) |value, i|
{ // not allowed
    …
}

This code would be rejected:

if (match_cond)
    x.removeThing(y);
    x.swap(z);

as it would have to be changed to either

if (match_cond) {
    x.removeThing(y);
    x.swap(z);
}

or

if (match_cond) x.removeThing(y);
x.swap(z);

Current code that uses if expressions may need parenthesis added

const value = if(condition)
    1
else
    2;

const value = if(condition) (
    1
) else (
    2
);

Both of these proposals still leave an issue with:

if (match_cond) x.removeThing(y); x.swap(z);

that shouldn't be too difficult to disallow if necessary.

@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@igotfr
Copy link

igotfr commented Jun 5, 2021

it would be interesting the developer choose between {} and identation level

if (match_cond)
    x.removeThing(y);
    x.swap(z);

// is equivalent to

if (match_cond) {
    x.removeThing(y);
    x.swap(z);
}

if (match_cond)
    x.removeThing(y);
  x.swap(z);

// is equivalent to

if (match_cond) {
    x.removeThing(y);
    x.swap(z);
}

if (match_cond) x.removeThing(y);
    x.swap(z);

// is equivalent to

if (match_cond) {
    x.removeThing(y);
    x.swap(z);
}

if (match_cond) x.removeThing(y); x.swap(z);

// is equivalent to

if (match_cond) {
    x.removeThing(y);
    x.swap(z);
}

if (match_cond)
    x.removeThing(y);
x.swap(z);

// is equivalent to

if (match_cond) {
    x.removeThing(y);
}

x.swap(z);

@andrewrk andrewrk changed the title error for incorrectly indented code error for incorrectly indented code / misleading indentation Jul 8, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 20, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@kuon
Copy link
Contributor

kuon commented Jan 23, 2023

My argument agains this, basically "to not make zig space sensitive" is because it would make generating zig code much harder. I am currently generating some zig code from a web gui, and having to handle this would be very cumbersome.

What I propose is to always run zig fmt when compiling, and issuing a warning if the formatting step changed the code. This warning should be configurable into an error in build.zig and command line option.

Finally, my way of writing code is to nearly always uses incorrect indentation and then let the formatter fix it before I commit, if zig fmt cannot correct this anymore because it is a compile error would be a major productivity drawback (I am not sure if you mean that).

@silversquirl
Copy link
Contributor

@kuon as andrew points out earlier in the discussion, code generators can simply emit no indentation whatsoever and still abide by this rule.

Your point about zig fmt is important though, and I'd like to see that addressed. I also write code with wildly incorrect formatting, then simply save and let my "format on save" hook fix it, so if zig fmt is unable to fix indentation errors that could slow me down considerably.

@Flaminator
Copy link

Will this also trigger an error when you for example have comments that are indented differently from code?

I see some people suggesting to always have zig fmt run when compiling. This is something I would not like to see happening as zig fmt or any other formatter for that matter do a lot of things that I don't like to see happen to my own code.

@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jun 29, 2023
@andrewrk
Copy link
Member Author

Prerequisite:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet