Home › Prolint

ifindent details

Mon, 2006-09-18 17:59 — jurjen

ifindent1: IF statement with indenting that could indicate a bug

ifindent2: IF statement with questionable indenting or couldn't check

These two rules share the same code base, but the warnings are separated into
major warnings (from ifindent1) and
minor warnings (from ifindent2).
This allows the more important warnings to have a higher severity.

If you want to run both rules "ifindent1" and "ifindent2", you might as well run rule ifindent instead to double the performance.

The rules output a variety of warnings for different situations. The major,
or 100-level, warnings will generally indicate either possible bugs or bad
indenting, and the minor, or 200-level, warnings generally do not indicate
bugs, but rather are IF statements which for various reasons cannot be checked
for potential bugs.

how these rules work

These rules have a somewhat complicated implementation. What they do is assess
your code in the same manner that the compiler will with regard to the grouping
of IF and ELSE blocks. What the compiler does may not be what you intended,
but at runtime, that is what will happen. Your indentation suggests how you
intended for your code to work, so we compare how the compiler will look at
it versus what your indentation implies, and report the differences.

This rule tests that all children nodes of an IF node have a greater than
or equal indent to the IF node, and also tests the next statement after
the IF to make sure it has an indent equal to the IF's indent. These two
checks ensure that the statements controlled by the IF block (as determined
by the compiler) are indented from it, and the statements outside and
following the IF block (as determined by the compiler) are not controlled
by the IF block. Note that this approach can occasionally result in more
than one warning being given for a particular line.

An example of the kinds of bugs found by this rule: 

 
IF ok = TRUE THEN               
  ASSIGN c1 = "correct". 
  IF other = TRUE THEN RETURN. 
ELSE                    /* Should be for "ok = TRUE" but actually is for "other = TRUE" */
  ASSIGN c1 = "oops". 
IF other THEN           
  ASSIGN v1 = "Hello".

In this example, Prolint will give three warnings:

  • "#101: Node IF has greater indent than IF on line 1. Expected to be 0, is 2."

    The IF at line 3 is not controlled by the IF ok = TRUE
    on line 1, but the code indenting implies that it is.

  • "#102: More indent expected for node ELSE. Node's indent should be at least 2, is 0."

    This refers to the ELSE on line 4. It occurs because the IF other
    on line 3 is the IF that the ELSE on line 4 is paired
    to, and given the IF other's current
    indent of 2, the rule says that the ELSE on line 4 for it should also
    have an indent of 2. This is where the code bug is - adding the missing
    DO... END. statements to the IF ok = TRUE on line 1 will result in
    the ELSE being correctly paired to the IF on line 1.

  • "#202: Node IF has less indent than IF on line 3. Expected to be 2, is 0."

    This warning follows on from the current situation - the IF on
    line 7 is not controlled by either of the previous IFs, so its' indent
    should be equal to the previous statement. Unfortunately, this previous
    statement is (incorrectly) the IF on line 3. This warning
    will be resolved simply by fixing the real problem, the missing DO.

the risc:

It is not really clear that the ELSE on line 4 is paired to the IF
on line 3, which causes a bug by changing the conditional execution of the code.

how to solve this:

If the programmer fixes the blocking by adding in the missing DO...END, all
of these messages will go away without any changes to the indenting. Alternately,
the programmer could try fixing the first indent to the recommended amount,
then rerunning the rule to see if this has removed the rest of the warnings.

The corrected code: 

IF ok = TRUE THEN DO:              
  ASSIGN c1 = "correct". 
  IF other = TRUE THEN RETURN. 
END.
ELSE                    /* Is now for "ok = TRUE" as intended */
  ASSIGN c1 = "oops". 
IF other THEN           
  ASSIGN v1 = "Hello".

After this correction, all warnings are prevented.

indent suggestions

These rules' suggested indents aren't enforcing a particular indenting
standard, but are simply the suggestions which would result in the indenting
being consistent with the blocking the compiler will use. It is assumed that
the program's indenting reflects the following model:

  • a block that follows an IF... THEN block will have the same indent as
    the IF. (as it is not controlled by the IF - it is a sibling of the IF and
    so should have the same indent level).

  • a statement contained within an IF block will be indented as much or
    more than the IF, as it is controlled by the IF.

    warning messages given

    • 101: important: indicates possible code bug. Next statement node has greater indent than the IF, falsely implying that it is controlled by the IF.
    • 102: important: indicates possible code bug. A subnode of the IF has less indent than the IF, implying that it is NOT controlled by the IF.
    • 201: informational. The next statement after the IF isn't in the same file as the IF,
      so we can't meaningfully compare indents.

    • 202: informational. The next statement node after the IF has less indent than the IF.
    • 203: informational. A subnode of the IF is in a different file, so no meaningful
      indent comparison can be made on this node. Only one of these warnings are
      output per IF statement, as an indicator.

    If it's hard to see why the rules are giving a certain warning, try changing
    the code's indent as it suggests which may cause the problem/possible
    bug to become obvious.

    Alternately, you can run the program through COMPILE...PREPROCESS
    then have a look at the code where the warning comes up - the problem
    may then become apparent.

    known problems


    Tabs:


    Prolint treats Tab characters as only one single character, although they should contribute to a larger indent. We can fix that if you want.

    false positives

    Preprocessing:

    As with all rules which use Proparse, this rule operates against code
    which is effectively preprocessed. Code which contains preprocessor
    directives will frequently cause false positives, due to the
    programmer's indenting in from the &IF etc. for readability. In the
    resulting preprocessed code, these just look like their indents are
    wrong. The number of false positives is reduced by omitting indent
    checks on AppBuilder-generated code, but some will still remain.
    Preprocessors can also cause false positives when a preprocessor
    is defined to contain more than one Progress statement. If the rule
    tries to calculate the indent of the second statement in the preprocessor,
    it will have some huge indent like 497, and this may cause warnings.

    For example:

    &scop PART1 IF TRUE THEN FIND FIRST order NO-LOCK NO-ERROR.
&scop PART2 IF AVAILABLE order THEN MESSAGE "avail order" VIEW-AS ALERT-BOX.
&scop ALLPARTS {&PART1} {&PART2} 
IF TRUE THEN DO: 
  MESSAGE "TRUE" VIEW-AS ALERT-BOX. 
END.
ELSE DO:
  {&ALLPARTS} 
END.

    The IF statement contained in &PART2 has an indent of about 50,
    because after the preprocessing has been done, this IF is on the
    same line as the IF from &PART1.

    Bad Indenting:


    These are probably when the developer didn't notice the indenting wasn't quite right.

    Tabs:


    (As above) Prolint treats Tab characters as only one single character, although they appear much wider to the user. We can fix that if you want.

    Include Files:


    Occasional false positives occur on nodes either at the beginning of,
    or immediately after, include files. This occurs because of the way
    the white space is handled when entering or leaving includes.

    more examples


    An example of a warning from the rules that doesn't quite tell you the correct problem,
    just that there is one here: 

     
    RUN myProg(var1,var2,OUTPUT retvar).
        IF retvar <> 0 THEN MESSAGE("Failed.") 
                       VIEW-AS ALERT-BOX.
    RUN yourProg (var3, OUTPUT retvar).

    The warning given for this code is that the indent is expected to be 8 but is 4,
    for node RUN on line 4. This is because we're only checking
    indents on IF nodes, their subnodes, and their next statement node after.
    We aren't checking that the IF has a sensible indent in the first place,
    but we're able to see that the second RUN (IF's
    next statement following) doesn't have a sensible indent when compared to
    the IF.

    what's not checked by the rule

    IF functions. This rule only checks IF statements.

    Indents of nodes in different files:


    It's pretty meaningless to try to check any indents of subnodes which are
    in a different file. If they're subnodes of the IF, they're in some
    include and could have any indenting scheme, and since they may
    not be stand-alone statements, there's nothing to compare indents
    to. Also, some programmers will write portions of a statement in one
    include and other portions in another include or the main procedure.

    Therefore, indents are not checked across files - if an IF is in
    one file and some of its' children statements in the block are
    in another file (an include), these children are ignored.

    In this example, we can check the indent of the ASSIGN node, but not of any of the nodes inside the {someinclude.i}. 

         IF ... THEN DO:
       ASSIGN var = "blue".
       {someinclude.i}
     END.

    How to suppress these warnings:

    You can put the directive {&_proparse_ prolint-nowarn(ifindent1)} or
    directly before {&_proparse_ prolint-nowarn(ifindent2)}
    the statement. See also: suppress warnings.


    ‹ idiskeywordupifindent ›

    • Comments

    Comment viewing options

    Select your preferred way to display the comments and click "Save settings" to activate your changes.
    Wed, 2013-01-02 10:29 — c_verbiest

    Good rule, but slow

    This rule has helped us to detect some hard to find bugs before the code got to the release.

    But during profiling on prolint to determine why it takes so long I found that it, both ifindent1 and ifindent2 active,
    took +/- 70% of the lint time for my test case.

    I disabled the rule in our profile for now, I'll try to look into the performance issue.
    The main performance issue was that it causes a huge amount of calls to parserGetNodeFileName, parserGetNodeText and parserQueryGetResult