Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <776550b7fac3870e65e903f0d8cb917dec58f7c2.camel@perches.com>
Subject: Re: [RFC PATCH v2] checkpatch: rewrite Kconfig parsing
From:   Joe Perches <joe@perches.com>
To:     Nicolai Fischer <nicolai.fischer@fau.de>,
        linux-kernel@vger.kernel.org
Cc:     apw@canonical.com, johannes.czekay@fau.de,
        linux-kernel@i4.cs.fau.de
Date:   Tue, 08 Dec 2020 10:58:10 -0800
In-Reply-To: <97fb4ba0-68b3-8faf-1d98-e4990b2e63e3@fau.de>
References: <a07d684c-37ea-b589-c4b1-489e88b5caa5@fau.de>
         <97fb4ba0-68b3-8faf-1d98-e4990b2e63e3@fau.de>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2020-12-08 at 18:18 +0100, Nicolai Fischer wrote:
> Checkpatch currently only warns if the help text is too short.
> To determine this the diff gets parsed for keywords starting
> a new entry, but several kinds of false positives can occur with
> the current implementation, especially when the config
> is not well formatted.
> 
> This patch makes the parsing more robust and includes
> new warnings if:
> 1) the help attribute is not specified last
> 2) there is no blank line or endif before the next keyword
> 3) the help text is not indented 2 spaces more than
> ???the attribute itself.
> 
> Signed-off-by: Nicolai Fischer <nicolai.fischer@fau.de>
> Co-developed-by: Johannes Czekay <johannes.czekay@fau.de>
> Signed-off-by: Johannes Czekay <johannes.czekay@fau.de>
> ---
> 
> This patch rewrites most of the Kconfig parsing to address
> the issues mentioned in the first RFC:
> 
> 1) search for 'help' instead of '---help---'
> > I believe all the '---help---' lines have been converted to just 'help'
> > so the '(?:---)?' bits here could be removed
> 
> 2) create new warnings:
> > Perhaps it'd be better to create a new warning when the help text
> > block is not the last block of the config section.  Maybe warn when
> > a blank line or endif is not the separator to the next keyword.
> > Maybe warn when the next line after help is not indented 2 more
> > spaces than the help line.
> 
> 3) fix handling of blank lines and rely on keywords for end of help text
> > This doesn't allow blank lines for multi-paragraph help text either.
> > 
> > I think keyword parsing is necessary and some false positives are
> > inevitable as the parsing logic in a line-by-line analyzer will
> > always be incomplete.
> 
> 
> It has occurred to us, that kconfig-language.rst does not explicitly
> specify that the help text should be the last attribute, although
> this is the defacto convention. Now that checkpatch actively checks
> for this, we should probably update the documentation accordingly.

Generally process is either to update documentation along with
with a checkpatch change or to update documentation first.

Also checkpatch isn't necessarily the best tool for this.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> -# check for Kconfig help text having a real description
> +# Check if Kconfig is well formatted. Warn if help text:
> +# 1) is shorter than $min_conf_desc_length lines
> +# 2) is not specified last
> +# 3) and next keyword are not separated by a blank line or endif
> +# 4) is not indented correctly
> ?# Only applies when adding the entry originally, after that we do not have
> ?# sufficient context to determine whether it is indeed long enough.
> ?		if ($realfile =~ /Kconfig/ &&

[]

> +				my $l = $line;
> +				$l =~ s/^$help_indent//;
> +				if ($l =~ /^(?:bool|tristate|string|hex|int|prompt|default|
> +					depends\ on|select|imply|visible\ if|range|option)\b/x) {

I think this is overly fragile.
These keywords are not required to be at the same indent as help.

Also as specified in scripts/kconfig/lexer.h, the kconfig specification
has more keywords than the list above.

In general, checkpatch does not have to be the tool of choice for
verifying everything.

For instance, checkpatch has a trivial check for MAINTAINERS entry
ordering, but there is a complete tool called parse-maintainers.pl
that verifies alphabetic section ordering.

I think most of what you seem to be attempting should be in a new
tool that completely understands Kconfig parsing.

I suggest instead of updating checkpatch, the scripts/kconfig/
content be updated to do these things.