|
| 1 | +#!/usr/bin/env perl |
| 2 | + |
| 3 | +# Detect bare 'grep' used as a test assertion where 'test_grep' |
| 4 | +# should be used, and '! test_grep' where 'test_grep !' should |
| 5 | +# be used. |
| 6 | +# |
| 7 | +# The shared shell parser tokenizes test bodies so that 'grep' |
| 8 | +# inside heredocs, command substitutions like $(grep ...), and |
| 9 | +# quoted strings is collapsed into a single token and never seen |
| 10 | +# by our check. A line-oriented approach would need to track |
| 11 | +# heredoc delimiters, nested $() depth, and cross-line pipe |
| 12 | +# state to avoid false positives on patterns like: |
| 13 | +# |
| 14 | +# write_script foo.sh <<-\EOF |
| 15 | +# grep pattern file # data, not an assertion |
| 16 | +# EOF |
| 17 | +# |
| 18 | +# The Lexer already handles these. |
| 19 | + |
| 20 | +use warnings; |
| 21 | +use strict; |
| 22 | +use File::Basename; |
| 23 | +do(dirname($0) . "/lib-shell-parser.pl") |
| 24 | + or die "$0: failed to load lib-shell-parser.pl: $@$!\n"; |
| 25 | + |
| 26 | +my $exit_code = 0; |
| 27 | + |
| 28 | +# GrepLintParser inherits ScriptParser's ability to find |
| 29 | +# test_expect_success/failure blocks and call check_test() |
| 30 | +# on each body. We override check_test() to walk the token |
| 31 | +# stream looking for bare grep assertions. |
| 32 | +package GrepLintParser; |
| 33 | + |
| 34 | +our @ISA = ('ScriptParser'); |
| 35 | + |
| 36 | +# After these tokens, the next token is a command word. |
| 37 | +# For example, in 'echo foo && grep bar file', the 'grep' |
| 38 | +# after '&&' is at command position and should be flagged. |
| 39 | +my %cmd_start = map { $_ => 1 } qw(&& || ; ;; do then else elif), "\n", '{', '('; |
| 40 | + |
| 41 | +# Tokens indicating grep's output is piped or redirected. |
| 42 | +my %filter_op = map { $_ => 1 } qw(| > >> <); |
| 43 | + |
| 44 | +# A token is at "command word" position if the shell would |
| 45 | +# interpret it as a program name rather than an argument. |
| 46 | +# Only 'grep' at command position is an assertion we should |
| 47 | +# flag; 'grep' as an argument ('test_must_fail grep') or |
| 48 | +# value ('for cmd in grep sed') is not. |
| 49 | +sub is_command_word { |
| 50 | + my ($tokens, $pos) = @_; |
| 51 | + return 1 if $pos == 0; |
| 52 | + for (my $j = $pos - 1; $j >= 0; $j--) { |
| 53 | + my $t = $tokens->[$j]->[0]; |
| 54 | + # After a separator or pipe, a new command starts. |
| 55 | + return 1 if $cmd_start{$t} || $t eq '|'; |
| 56 | + # After '}' or ')', what follows is a separator or |
| 57 | + # redirect on the compound command, not a new command. |
| 58 | + return 0 if $t eq '}' || $t eq ')'; |
| 59 | + # '!' is a prefix that does not consume command |
| 60 | + # position; keep scanning to find what precedes it. |
| 61 | + next if $t eq '!'; |
| 62 | + # Any other word means we are past the command word. |
| 63 | + return 0; |
| 64 | + } |
| 65 | + return 1; |
| 66 | +} |
| 67 | + |
| 68 | +# lint_ok() reports whether a bare grep carries a trailing |
| 69 | +# '# lint-ok' comment telling this linter to skip it. |
| 70 | +# |
| 71 | +# In practice this is needed for just one case: a grep acting |
| 72 | +# as a data filter whose output is consumed by a redirect or |
| 73 | +# pipe on an enclosing compound command (such as a subshell or |
| 74 | +# brace group) rather than by grep's own pipeline, e.g. |
| 75 | +# |
| 76 | +# ( grep ... && # lint-ok |
| 77 | +# sed ... ) >out |
| 78 | +# |
| 79 | +# { grep ... || : # lint-ok |
| 80 | +# } >out |
| 81 | +# |
| 82 | +# is_filter() only scans grep's own pipeline: it stops at the |
| 83 | +# separator before the compound command closes and never sees |
| 84 | +# the outer redirect, so it would flag such a grep as an |
| 85 | +# assertion. A grep that really is an assertion is better |
| 86 | +# written as test_grep (or a guarded test_grep when the file's |
| 87 | +# presence is conditional) than annotated with lint-ok. |
| 88 | +sub lint_ok { |
| 89 | + my ($raw_lines, $ln) = @_; |
| 90 | + if ($ln < 1 || $ln > @$raw_lines) { |
| 91 | + warn "lint_ok: line number $ln out of range (1.." . |
| 92 | + scalar(@$raw_lines) . ")\n"; |
| 93 | + return 0; |
| 94 | + } |
| 95 | + return $raw_lines->[$ln - 1] =~ /lint-ok/; |
| 96 | +} |
| 97 | + |
| 98 | +# Grep is a filter (not an assertion) if it receives piped |
| 99 | +# input or sends its output to a pipe or redirect. Check |
| 100 | +# both directions from grep's position in the token stream. |
| 101 | +sub is_filter { |
| 102 | + my ($tokens, $pos) = @_; |
| 103 | + # Backward: is grep receiving piped input? |
| 104 | + # Newlines don't break pipes ('cmd |\n grep' is one |
| 105 | + # pipeline), so skip past them. |
| 106 | + for (my $j = $pos - 1; $j >= 0; $j--) { |
| 107 | + my $t = $tokens->[$j]->[0]; |
| 108 | + return 1 if $t eq '|'; |
| 109 | + next if $t eq "\n"; |
| 110 | + last if $cmd_start{$t} || $t eq '}' || $t eq ')'; |
| 111 | + } |
| 112 | + # Forward: is grep piping or redirecting output? |
| 113 | + # Unlike the backward scan, we do not skip newlines here: |
| 114 | + # a bare newline is a command boundary, and redirects or |
| 115 | + # pipes must appear on the same line as grep (or after a |
| 116 | + # line continuation, which the Lexer consumes). |
| 117 | + for (my $j = $pos + 1; $j < @$tokens; $j++) { |
| 118 | + my $t = $tokens->[$j]->[0]; |
| 119 | + return 0 if $cmd_start{$t}; |
| 120 | + return 1 if $filter_op{$t}; |
| 121 | + } |
| 122 | + return 0; |
| 123 | +} |
| 124 | + |
| 125 | +# Map a body-relative line number to a file line number. |
| 126 | +# For double-quoted bodies, backslash-continuation lines |
| 127 | +# (\<newline>) are consumed by the Lexer without appearing |
| 128 | +# in the body text, so the inner parser sees fewer lines |
| 129 | +# than the source file has. We walk the source lines to |
| 130 | +# count continuations and adjust accordingly. |
| 131 | +sub body_to_file_line { |
| 132 | + my ($body_lineno, $body_token, $raw_lines, $body_start) = @_; |
| 133 | + my $body_text = $body_token->[0]; |
| 134 | + my $body_end_line = $body_token->[4]; |
| 135 | + unless ($body_start && $body_start >= 1) { |
| 136 | + warn "body_start is not a positive integer\n"; |
| 137 | + return $body_lineno; |
| 138 | + } |
| 139 | + my $file_lineno = $body_lineno + $body_start - 1; |
| 140 | + # Only double-quoted bodies have line splices. |
| 141 | + return $file_lineno unless $body_text =~ /^"/; |
| 142 | + my $adj = 0; |
| 143 | + my $lines_seen = 0; |
| 144 | + unless ($body_end_line && $body_end_line >= $body_start) { |
| 145 | + warn "body_end_line is not set for double-quoted body\n"; |
| 146 | + return $file_lineno; |
| 147 | + } |
| 148 | + my $end = $body_end_line; |
| 149 | + if ($end > @$raw_lines) { |
| 150 | + warn "body_end_line ($end) exceeds file length (" . |
| 151 | + scalar(@$raw_lines) . ")\n"; |
| 152 | + return $file_lineno; |
| 153 | + } |
| 154 | + my $src_ln = $body_start; |
| 155 | + while ($src_ln <= $end && $lines_seen < $body_lineno) { |
| 156 | + my $line = $raw_lines->[$src_ln - 1]; |
| 157 | + # Odd trailing backslashes = continuation (\<nl>). |
| 158 | + # Even = escaped backslashes (\\), not a continuation. |
| 159 | + if ($line =~ /(\\*)$/ && length($1) % 2 == 1) { |
| 160 | + $adj++; |
| 161 | + } else { |
| 162 | + $lines_seen++; |
| 163 | + } |
| 164 | + $src_ln++; |
| 165 | + } |
| 166 | + if ($lines_seen < $body_lineno) { |
| 167 | + warn "body_lineno ($body_lineno) not found within body range " . |
| 168 | + "($body_start..$end)\n"; |
| 169 | + } |
| 170 | + return $file_lineno + $adj; |
| 171 | +} |
| 172 | + |
| 173 | +# ScriptParser calls this for each test body found in the script. |
| 174 | +sub check_test { |
| 175 | + my $self = shift @_; |
| 176 | + my $title = ScriptParser::unwrap(shift @_); |
| 177 | + my $body_token = shift @_; |
| 178 | + my $body_start = $body_token->[3]; |
| 179 | + my $body = ScriptParser::unwrap($body_token); |
| 180 | + # Handle heredoc-style test bodies: |
| 181 | + # test_expect_success 'title' - <<\EOF |
| 182 | + # grep pattern file |
| 183 | + # EOF |
| 184 | + # The '-' signals that the body follows as a heredoc. |
| 185 | + if ($body eq '-') { |
| 186 | + my $herebody = shift @_; |
| 187 | + if ($herebody) { |
| 188 | + $body = $herebody->{content}; |
| 189 | + $body_start = $herebody->{start_line}; |
| 190 | + } |
| 191 | + } |
| 192 | + return unless $body; |
| 193 | + |
| 194 | + my $raw_lines = $self->{raw_lines}; |
| 195 | + |
| 196 | + # The outer parser gives us the body as an opaque string. |
| 197 | + # Parse it to get individual tokens with command boundaries. |
| 198 | + my $parser = ShellParser->new(\$body); |
| 199 | + my @tokens = $parser->parse(); |
| 200 | + |
| 201 | + my $file = $self->{file}; |
| 202 | + |
| 203 | + for (my $i = 0; $i < @tokens; $i++) { |
| 204 | + my $text = $tokens[$i]->[0]; |
| 205 | + next unless is_command_word(\@tokens, $i); |
| 206 | + |
| 207 | + my $token_lineno = $tokens[$i]->[3]; |
| 208 | + unless (defined($token_lineno) && $token_lineno >= 1) { |
| 209 | + warn "token has no line number\n"; |
| 210 | + next; |
| 211 | + } |
| 212 | + my $file_lineno = body_to_file_line( |
| 213 | + $token_lineno, |
| 214 | + $body_token, $raw_lines, $body_start); |
| 215 | + |
| 216 | + # '!' negates the exit code without consuming command |
| 217 | + # position. '! test_grep' is an anti-pattern because |
| 218 | + # test_grep only prints diagnostics on grep failure, |
| 219 | + # and '!' inverts after that decision is already made. |
| 220 | + if ($text eq '!') { |
| 221 | + if ($i + 1 < @tokens && |
| 222 | + $tokens[$i + 1]->[0] eq 'test_grep' && |
| 223 | + !lint_ok($raw_lines, $file_lineno)) { |
| 224 | + print "$file:$file_lineno: error: ", |
| 225 | + 'use "test_grep !" instead of ', |
| 226 | + '"! test_grep"', "\n"; |
| 227 | + $exit_code = 1; |
| 228 | + } |
| 229 | + next; |
| 230 | + } |
| 231 | + |
| 232 | + # Bare grep as a command (not a filter) is a test |
| 233 | + # assertion that should use test_grep for better |
| 234 | + # failure diagnostics. |
| 235 | + if ($text eq 'grep' && |
| 236 | + !is_filter(\@tokens, $i) && |
| 237 | + !lint_ok($raw_lines, $file_lineno)) { |
| 238 | + print "$file:$file_lineno: error: ", |
| 239 | + "bare grep outside pipeline ", |
| 240 | + "(use test_grep)\n"; |
| 241 | + $exit_code = 1; |
| 242 | + } |
| 243 | + } |
| 244 | +} |
| 245 | + |
| 246 | +package main; |
| 247 | + |
| 248 | +for my $file (@ARGV) { |
| 249 | + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n"; |
| 250 | + my @raw_lines = <$fh>; |
| 251 | + close $fh; |
| 252 | + my $s = join('', @raw_lines); |
| 253 | + my $parser = GrepLintParser->new(\$s); |
| 254 | + $parser->{file} = $file; |
| 255 | + $parser->{raw_lines} = \@raw_lines; |
| 256 | + $parser->parse(); |
| 257 | +} |
| 258 | +exit $exit_code; |
0 commit comments