Discussion on indentation rules around '#if' during linting

[JonatanWaern]

Lint gives incorrect warning for statements inside #if in case.

switch (foo) {
case 0:
    #if (true) {
        bar();
    }
}

Will warn on the 'bar();' line

[jvsqzj]

After discussion with the others in the team, we don't see this as an actual issue.
We've debated on whether or not the #if or #else intiated compound blocks should apply the indentation increment or not.

In the example above, the switch indentation is supposed to see all case X: declarations in alignment with the switch keyword, and all the statements for a corresponding case, are implemented under the SwitchCase enum.

We decided the SwitchHashIf statments don't increment indentation, as these are compile time declarations and it can be argued that this is an exception to the regular indentation rules.

The real issue here in my opinion, is that currently the regular HashIfContent struct does increment the indentation level for its content, and this is not intentional. This is a side-effect of implementing the rule IN3 - indent_code_block which increments indentation depth for every CompoundContent node.

We have explored adding an exception for indent_code_block but the check does not have any knowledge of whether the CompoundContent was preceded by a #if / #else or just a regular code node that starts a CompoundContent body.

In summary, this is our view of how the example above is formatted correctly:

switch (foo) {
case 0:
#if (true) {
    bar();
}
}

And currently, the linting rule for it implements this behavior.
The indentation of the #if is not really tracked, and it can be placed in any column as we are excluding it from the checks by the indent_switch_case rule.

---

An example of the case that actually has an issue in my view, is this:

#if (SOME_COMPILE_TIME_PARAMETER_OR_CONSTANT) {
import "codebase/platform/generic-transaction.dml";
}

The inside of this CompoundContent is currently marked as a warning, but this is the regular format I would expect from a similar example of C code that uses preprocessing or macros. Hence, we are considering how to implement an exception for this rule for the HashIfContent

[jvsqzj]

A very specific question to address having context of where a CompoundContent is opened would be:

@JonatanWaern Can you recommend any mechanism to check the parent node of a CompoundContent in the AST ?

[JonatanWaern]

After discussion with the others in the team, we don't see this as an actual issue. We've debated on whether or not the #if or #else intiated compound blocks should apply the indentation increment or not.

In the example above, the switch indentation is supposed to see all case X: declarations in alignment with the switch keyword, and all the statements for a corresponding case, are implemented under the SwitchCase enum.

We decided the SwitchHashIf statments don't increment indentation, as these are compile time declarations and it can be argued that this is an exception to the regular indentation rules.

The real issue here in my opinion, is that currently the regular HashIfContent struct does increment the indentation level for its content, and this is not intentional. This is a side-effect of implementing the rule IN3 - indent_code_block which increments indentation depth for every CompoundContent node.

We have explored adding an exception for indent_code_block but the check does not have any knowledge of whether the CompoundContent was preceded by a #if / #else or just a regular code node that starts a CompoundContent body.

In summary, this is our view of how the example above is formatted correctly:

switch (foo) {
case 0:
#if (true) {
    bar();
}
}

And currently, the linting rule for it implements this behavior. The indentation of the #if is not really tracked, and it can be placed in any column as we are excluding it from the checks by the indent_switch_case rule.

I don't have a particular vested interest in either style here (though my intuition would be #if s indenting as a normal one would). I will add the others from the DML team and we can gather some opinions on this.

A very specific question to address having context of where a CompoundContent is opened would be:

@JonatanWaern Can you recommend any mechanism to check the parent node of a CompoundContent in the AST ?

ATM I dont think so. The AST does not contain backrefs to parents, adding it is probably a little bit of work but not trivial. (AstObject contents are already boxed, but you'd need to add an enum over all possible types that the parent could be in order to disambiguate).
A work-around hack that I suspect would work is to in the hashif case peek at the contained statement, and if it is a compound statement you can preemptively deincrement the indentation level with the expectation that it will be reincremented later. Thus resulting in the 'same' indentation level.

[JonatanWaern]

I don't have a particular vested interest in either style here (though my intuition would be #if s indenting as a normal one would). I will add the others from the DML team and we can gather some opinions on this.

Internal discussions of this in the DML team all lead to us liking #if statments being indented similarly to regular 'if's, the exception being for top-level compatibility-quards, e.g.

#if (dml_1_2) {
template some_template { ... }
} #else {
template some_template { ... }
}

There may be some relevant discussion around how to indent #if's around switch cases (not statements within cases), e.g.

switch (foo) {
case 0:
    ...
    break;
#if (condition) {
case 1:
    ...
}
}

Regardless, I expect there to be more discussion on this here. For now I am removing the 'bug' label, and tagging this as a question instead, I guess?

[JonatanWaern]

Mirroring some further internal discussions of #if cases and their usages:

Main difference seems to be whether to treat them like C preprocessor directives (which are not usually indented) or as regular if statements.

From what I've seen, the following three scenarios all seem like 'reasonable' indentation levels for #if statements in some sense

Top-level flag-style of objects with long declarations

#if (some_flag) {
bank whatever {
    ... long
}
}

This makes sense since it'd be cumbersome to fully indent the long declaration, and this is similar to what is done in C. Personally I dislike the double '}' at the end, which you would not have in C.

Flag-style conditional deeply within a structure somewhere:

...
           group g {
#if (some_flag) {
               ... statements indented normally
}
           }
...

Here the statements are indented as if the #if didn't exist, since it is at top-level. This is conceptually similar to the case above.

#if as regular if:

...
           method m {
               #if (some_flag) {
                   ... statements 
               }
           }
...

Just regular 'if' statement indentation