[PATCH 0/3] Detect suspicious indentation after conditional

[PATCH 0/3] Detect suspicious indentation after conditional

[Julius Werner]

This patch series is adding functionality to checkpatch.pl to test for
incorrect code indentation after a conditional statement, like this:

 if (a)
   b;
   c;

(Indentation implies that `c;` was guarded by the conditional, but it
isn't.) The main part is re-sending a patch from Ivo Sieben that was
already proposed in 2014 [1]. I don't know why it was never merged --
it seems that there was no discussion on it. I hope that it was only
overlooked, because it works great, and I think this is a very important
class of common error to catch.

I have tested it extensively on the kernel tree and in the course of
that found a few more edge cases that get fixed by the other two
patches. With all these applied, the vast majority of hits I get from
this check on the kernel tree are actual indentation errors or other
code style violations (e.g. case label and statement on the same line).
The only significant remaining group of false positives I found are
cases of macros being defined within a function, which are overall very
rare. I think the benefit of adding this check would far outweigh the
remaining amount of noise.

[1]: https://lore.kernel.org/patchwork/patch/465116

Ivo Sieben (1):
  Suspicious indentation detection after conditional statement

Julius Werner (2):
  checkpatch: ctx_statement_block: Fix preprocessor guard tracking
  checkpatch: Ignore labels when checking indentation

 scripts/checkpatch.pl | 56 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 4 deletions(-)

-- 
2.29.2

[PATCH 1/3] checkpatch: ctx_statement_block: Fix preprocessor guard tracking

[Julius Werner]

The preprocessor guard tracking in ctx_statement_block() is (and seems
to have always been) subtly broken whenever tracking over an #else: the
code is supposed to restore state from the current top of the stack
(like and #endif just without removing it). However, it indexes the
stack at [$#stack - 1]. In Perl, $# does not give you the length of an
array, it gives you the highest valid index. Therefore, the correct
index should just be [$#stack].

The preprocessor guard tracking also gets confused when
ctx_statement_block() was called on a line that is already inside a
preprocessor guard, and steps out of it within the same statement. This
happens commonly with constructs like this:

 #if CONFIG_XXX
   for (a = first_a(); !a_finished(); a = next_a()) {
 #else
   for (b = first_b(); !b_finished(); b = next_b()) {
 #endif
     ... loop body ...

The best course of action in this case is to not try to restore any
previous state (which we don't have) at all, so we should just keep our
current state if $#stack is already 0.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index df8b23dc1eb0af..ffccbd2033e579 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1572,9 +1572,9 @@ sub ctx_statement_block {
 		# Handle nested #if/#else.
 		if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
 			push(@stack, [ $type, $level ]);
-		} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
-			($type, $level) = @{$stack[$#stack - 1]};
-		} elsif ($remainder =~ /^#\s*endif\b/) {
+		} elsif ($remainder =~ /^#\s*(?:else|elif)\b/ && $#stack > 0) {
+			($type, $level) = @{$stack[$#stack]};
+		} elsif ($remainder =~ /^#\s*endif\b/ && $#stack > 0) {
 			($type, $level) = @{pop(@stack)};
 		}
 
-- 
2.29.2

[PATCH 2/3] Suspicious indentation detection after conditional statement

[Julius Werner]

From: Ivo Sieben <meltedpianoman@gmail.com>

Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.

For example the following error is caught;

	if (foo)
		bar();
		return;

Which can be either intended by the programmer as:

	if (foo)
		bar();
	return;

or
	if (foo) {
		bar();
		return;
	}

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
 scripts/checkpatch.pl | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ffccbd2033e579..c1dfc0107be41d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4185,6 +4185,47 @@ sub process {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
+
+# Also check if the next statement after the previous condition has the same indent
+			my ($stat_next, undef, $line_nr_next_next) =
+				ctx_statement_block($line_nr_next, $remain_next, $off_next);
+			my $s_next = $stat_next;
+
+			# Remove line prefixes
+			$s_next =~ s/\n./\n/gs;
+
+			# Remove any comments
+			$s_next =~ s/$;//g;
+
+			# Skip this check for in case next statement starts with 'else'
+			if ($s_next !~ /\s*\belse\b/) {
+
+				# Remove while that belongs to a do {} while
+				if ($stat =~ /\bdo\b/) {
+					$s_next =~ s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+				}
+
+				# Remove blank lines
+				$s_next =~ s/\s*\\?\n//g;
+
+				# Get the real next lines
+				my $next_nof_lines = $line_nr_next_next - $line_nr_next;
+				my $stat_next_real = raw_line($line_nr_next, $next_nof_lines);
+				if (!defined($stat_next_real)) {
+					$stat_next_real = "";
+				} elsif ($next_nof_lines > 1) {
+					$stat_next_real = "[...]\n$stat_next_real";
+				}
+				my (undef, $nindent) = line_stats('+' . $s_next);
+
+				#print "stat_next<$stat_next> stat<$stat> indent<$indent> nindent<$nindent> s_next<$s_next> stat_next_real<$stat_next_real>\n";
+
+				if ($nindent > $indent) {
+					WARN("SUSPICIOUS_CODE_INDENT",
+					     "suspicious code indentation after conditional statements\n" .
+					     $herecurr . "$stat_real\n$stat_next_real\n");
+				}
+			}
 		}
 
 		# Track the 'values' across context and added lines.
-- 
2.29.2

[PATCH 3/3] checkpatch: Ignore labels when checking indentation

[Julius Werner]

Goto labels are commonly written in the leftmost column (sometimes with
one space in front), regardless of indentation level. Sometimes they're
on a line of their own, but sometimes the same line is shared with a
normal code statement that then starts at the expected indentation
level. When checking indentation, we should check where that normal
piece of code starts, not where the label starts (there's a separate
INDENTED_LABEL test to check the label itself). Therefore, the
line_stats() function that is used to get indentation level should treat
goto labels like whitespace. The SUSPICIOUS_CODE_INDENT test also needs
to explicitly ignore labels to make sure it doesn't get confused by
them.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 scripts/checkpatch.pl | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c1dfc0107be41d..d89367a59e7d37 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1396,8 +1396,12 @@ sub copy_spacing {
 sub line_stats {
 	my ($line) = @_;
 
-	# Drop the diff line leader and expand tabs
+	# Drop the diff line leader
 	$line =~ s/^.//;
+
+	# Treat labels like whitespace when counting indentation
+	$line =~ s/^( ?$Ident:)/" " x length($1)/e;
+
 	$line = expand_tabs($line);
 
 	# Pick the indent from the front of the line.
@@ -4197,6 +4201,9 @@ sub process {
 			# Remove any comments
 			$s_next =~ s/$;//g;
 
+			# Remove any leading labels
+			$s_next =~ s/\n( ?$Ident:)/"\n" . " " x length($1)/eg;
+
 			# Skip this check for in case next statement starts with 'else'
 			if ($s_next !~ /\s*\belse\b/) {
 
-- 
2.29.2

Re: [PATCH 0/3] Detect suspicious indentation after conditional

[Julius Werner]

*friendly ping*

Hi Andy, Joe,

Any comments on this patch series? Are you guys the right point of
contact for checkpatch changes?

On Thu, Mar 25, 2021 at 8:50 PM Julius Werner <jwerner@chromium.org> wrote:
>
> This patch series is adding functionality to checkpatch.pl to test for
> incorrect code indentation after a conditional statement, like this:
>
>  if (a)
>    b;
>    c;
>
> (Indentation implies that `c;` was guarded by the conditional, but it
> isn't.) The main part is re-sending a patch from Ivo Sieben that was
> already proposed in 2014 [1]. I don't know why it was never merged --
> it seems that there was no discussion on it. I hope that it was only
> overlooked, because it works great, and I think this is a very important
> class of common error to catch.
>
> I have tested it extensively on the kernel tree and in the course of
> that found a few more edge cases that get fixed by the other two
> patches. With all these applied, the vast majority of hits I get from
> this check on the kernel tree are actual indentation errors or other
> code style violations (e.g. case label and statement on the same line).
> The only significant remaining group of false positives I found are
> cases of macros being defined within a function, which are overall very
> rare. I think the benefit of adding this check would far outweigh the
> remaining amount of noise.
>
> [1]: https://lore.kernel.org/patchwork/patch/465116
>
> Ivo Sieben (1):
>   Suspicious indentation detection after conditional statement
>
> Julius Werner (2):
>   checkpatch: ctx_statement_block: Fix preprocessor guard tracking
>   checkpatch: Ignore labels when checking indentation
>
>  scripts/checkpatch.pl | 56 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
>
> --
> 2.29.2
>

Re: [PATCH 0/3] Detect suspicious indentation after conditional

[Joe Perches]

On Wed, 2021-04-14 at 14:18 -0700, Julius Werner wrote:
> *friendly ping*
> 
> Hi Andy, Joe,
> 
> Any comments on this patch series? Are you guys the right point of
> contact for checkpatch changes?

I don't have any issue with this patch set, but Andy is really
the person that should approve any changes to this block of code.

> On Thu, Mar 25, 2021 at 8:50 PM Julius Werner <jwerner@chromium.org> wrote:
> > 
> > This patch series is adding functionality to checkpatch.pl to test for
> > incorrect code indentation after a conditional statement, like this:
> > 
> >  if (a)
> >    b;
> >    c;
> > 
> > (Indentation implies that `c;` was guarded by the conditional, but it
> > isn't.) The main part is re-sending a patch from Ivo Sieben that was
> > already proposed in 2014 [1]. I don't know why it was never merged --
> > it seems that there was no discussion on it. I hope that it was only
> > overlooked, because it works great, and I think this is a very important
> > class of common error to catch.
> > 
> > I have tested it extensively on the kernel tree and in the course of
> > that found a few more edge cases that get fixed by the other two
> > patches. With all these applied, the vast majority of hits I get from
> > this check on the kernel tree are actual indentation errors or other
> > code style violations (e.g. case label and statement on the same line).
> > The only significant remaining group of false positives I found are
> > cases of macros being defined within a function, which are overall very
> > rare. I think the benefit of adding this check would far outweigh the
> > remaining amount of noise.
> > 
> > [1]: https://lore.kernel.org/patchwork/patch/465116
> > 
> > Ivo Sieben (1):
> >   Suspicious indentation detection after conditional statement
> > 
> > Julius Werner (2):
> >   checkpatch: ctx_statement_block: Fix preprocessor guard tracking
> >   checkpatch: Ignore labels when checking indentation
> > 
> >  scripts/checkpatch.pl | 56 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 4 deletions(-)
> > 
> > --
> > 2.29.2
> > 

Re: [PATCH 0/3] Detect suspicious indentation after conditional

[Julius Werner]

*another friendly ping*

Hi Andy, any comments?

Joe, if Andy doesn't have time to look at this anymore at the moment
(if I'm looking for this right I think the last mail from him I can
find on LKML was in 2019?), is there anything else I can do to
convince you to take this series?