[PATCH] checkpatch: Avoid missing typo suggestions

[PATCH] checkpatch: Avoid missing typo suggestions

[Kees Cook]

My codespell dictionary has a lot of capitalized words. For example:

MSDOS->MS-DOS

Since checkpatch uses case-insensitive matching, I get an undefined
variable warning and then empty suggestions for things like this:

Use of uninitialized value $typo_fix in concatenation (.) or string at ./scripts/checkpatch.pl line 2958.

WARNING: 'msdos' may be misspelled - perhaps ''?
+       struct msdos_dir_entry *de;

This fixes the matcher to avoid the warning, but it's still a rather
silly suggestion:

WARNING: 'msdos' may be misspelled - perhaps 'MS-DOS'?
+       struct msdos_dir_entry *de;

So I'm not really sure what to do with this ... filter out bad
suggestions instead?

Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9f8146600d0..1aaf3317b6ad 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -727,7 +727,7 @@ if ($codespell) {
 
 			my ($suspect, $fix) = split(/->/, $line);
 
-			$spelling_fix{$suspect} = $fix;
+			$spelling_fix{lc($suspect)} = $fix;
 		}
 		close($spelling);
 	} else {
-- 
2.25.1


-- 
Kees Cook

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Joe Perches]

On Wed, 2020-06-03 at 16:19 -0700, Kees Cook wrote:
> My codespell dictionary has a lot of capitalized words. For example:
> 
> MSDOS->MS-DOS
> 
> Since checkpatch uses case-insensitive matching, I get an undefined
> variable warning and then empty suggestions for things like this:
> 
> Use of uninitialized value $typo_fix in concatenation (.) or string at ./scripts/checkpatch.pl line 2958.
> 
> WARNING: 'msdos' may be misspelled - perhaps ''?
> +       struct msdos_dir_entry *de;
> 
> This fixes the matcher to avoid the warning, but it's still a rather
> silly suggestion:
> 
> WARNING: 'msdos' may be misspelled - perhaps 'MS-DOS'?
> +       struct msdos_dir_entry *de;
> 
> So I'm not really sure what to do with this ... filter out bad
> suggestions instead?

Hey Kees.

Maybe this?

btw: My codespell dictionary file moved to
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt

and I had to use --codespell --codespellfile=(above) so
maybe there should be multiple lookups for this file
like the array below.

Are there other standard codespell dictionary locations?
---
 scripts/checkpatch.pl | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5f00df2c3f59..52aa0dd53d80 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -59,7 +59,7 @@ my $minimum_perl_version = 5.10.0;
 my $min_conf_desc_length = 4;
 my $spelling_file = "$D/spelling.txt";
 my $codespell = 0;
-my $codespellfile = "/usr/share/codespell/dictionary.txt";
+my $codespellfile;
 my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
@@ -716,7 +716,20 @@ if (open(my $spelling, '<', $spelling_file)) {
 }
 
 if ($codespell) {
-	if (open(my $spelling, '<', $codespellfile)) {
+	if (!defined($codespellfile)) {
+		my @csfiles = ("/usr/share/codespell/dictionary.txt",
+			       "/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt");
+		foreach my $csfile (@csfiles) {
+			if (-f $csfile) {
+				$codespellfile = $csfile;
+				last;
+			}
+		}
+	}
+
+	if (!defined($codespellfile)) {
+		warn "No codespell typos will be found - codespell dictionary not found\n";
+	} elsif (open(my $spelling, '<', $codespellfile)) {
 		while (<$spelling>) {
 			my $line = $_;
 
@@ -2963,13 +2976,21 @@ sub process {
 			while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
 				my $typo = $1;
 				my $typo_fix = $spelling_fix{lc($typo)};
-				$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
-				$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
+				$typo_fix = $spelling_fix{$typo} if (!defined($typo_fix));
+				$typo_fix = $spelling_fix{uc($typo)} if (!defined($typo_fix));
+				$typo_fix = 'unknown typo fix' if (!defined($typo_fix));
+				if ($typo =~ /^[A-Z]+$/) {
+					$typo_fix = uc($typo_fix);
+				} elsif ($typo =~ /^[A-Z]/) {
+					$typo_fix = ucfirst($typo_fix);
+				}
+
 				my $msg_level = \&WARN;
 				$msg_level = \&CHK if ($file);
 				if (&{$msg_level}("TYPO_SPELLING",
 						  "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
-				    $fix) {
+				    $fix &&
+				    $typo_fix ne 'unknown typo fix') {
 					$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
 				}
 			}

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Maxim Uvarov]

On Thu, 4 Jun 2020 at 03:39, Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-06-03 at 16:19 -0700, Kees Cook wrote:
> > My codespell dictionary has a lot of capitalized words. For example:
> >
> > MSDOS->MS-DOS
> >
> > Since checkpatch uses case-insensitive matching, I get an undefined
> > variable warning and then empty suggestions for things like this:
> >
> > Use of uninitialized value $typo_fix in concatenation (.) or string at ./scripts/checkpatch.pl line 2958.
> >
> > WARNING: 'msdos' may be misspelled - perhaps ''?
> > +       struct msdos_dir_entry *de;
> >
> > This fixes the matcher to avoid the warning, but it's still a rather
> > silly suggestion:
> >
> > WARNING: 'msdos' may be misspelled - perhaps 'MS-DOS'?
> > +       struct msdos_dir_entry *de;
> >
> > So I'm not really sure what to do with this ... filter out bad
> > suggestions instead?
>
> Hey Kees.
>
> Maybe this?
>
> btw: My codespell dictionary file moved to
> /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
>
> and I had to use --codespell --codespellfile=(above) so
> maybe there should be multiple lookups for this file
> like the array below.
>
> Are there other standard codespell dictionary locations?

It might be better to support standard and non standard locations. I
think it's better to request from codespell where his dictionary is.
I created ticket for this:
https://github.com/codespell-project/codespell/issues/1540

However this patch is good as a temporary solution.

Regards,
Maxim.

> ---
>  scripts/checkpatch.pl | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5f00df2c3f59..52aa0dd53d80 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -59,7 +59,7 @@ my $minimum_perl_version = 5.10.0;
>  my $min_conf_desc_length = 4;
>  my $spelling_file = "$D/spelling.txt";
>  my $codespell = 0;
> -my $codespellfile = "/usr/share/codespell/dictionary.txt";
> +my $codespellfile;
>  my $conststructsfile = "$D/const_structs.checkpatch";
>  my $typedefsfile = "";
>  my $color = "auto";
> @@ -716,7 +716,20 @@ if (open(my $spelling, '<', $spelling_file)) {
>  }
>
>  if ($codespell) {
> -       if (open(my $spelling, '<', $codespellfile)) {
> +       if (!defined($codespellfile)) {
> +               my @csfiles = ("/usr/share/codespell/dictionary.txt",
> +                              "/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt");
> +               foreach my $csfile (@csfiles) {
> +                       if (-f $csfile) {
> +                               $codespellfile = $csfile;
> +                               last;
> +                       }
> +               }
> +       }
> +
> +       if (!defined($codespellfile)) {
> +               warn "No codespell typos will be found - codespell dictionary not found\n";
> +       } elsif (open(my $spelling, '<', $codespellfile)) {
>                 while (<$spelling>) {
>                         my $line = $_;
>
> @@ -2963,13 +2976,21 @@ sub process {
>                         while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
>                                 my $typo = $1;
>                                 my $typo_fix = $spelling_fix{lc($typo)};
> -                               $typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
> -                               $typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
> +                               $typo_fix = $spelling_fix{$typo} if (!defined($typo_fix));
> +                               $typo_fix = $spelling_fix{uc($typo)} if (!defined($typo_fix));
> +                               $typo_fix = 'unknown typo fix' if (!defined($typo_fix));
> +                               if ($typo =~ /^[A-Z]+$/) {
> +                                       $typo_fix = uc($typo_fix);
> +                               } elsif ($typo =~ /^[A-Z]/) {
> +                                       $typo_fix = ucfirst($typo_fix);
> +                               }
> +
>                                 my $msg_level = \&WARN;
>                                 $msg_level = \&CHK if ($file);
>                                 if (&{$msg_level}("TYPO_SPELLING",
>                                                   "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
> -                                   $fix) {
> +                                   $fix &&
> +                                   $typo_fix ne 'unknown typo fix') {
>                                         $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
>                                 }
>                         }
>

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Joe Perches]

On Thu, 2020-06-04 at 09:55 +0300, Maxim Uvarov wrote:
> On Thu, 4 Jun 2020 at 03:39, Joe Perches <joe@perches.com> wrote:

Hi Maxim.

> > btw: My codespell dictionary file moved to
> > /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
> > 
> > and I had to use --codespell --codespellfile=(above) so
> > maybe there should be multiple lookups for this file
> > like the array below.
> > 
> > Are there other standard codespell dictionary locations?
> 
> It might be better to support standard and non standard locations.

It already does with the --codespellfile=<location> opti.

> I think it's better to request from codespell where his dictionary is.

Maybe a good idea, but looking at the codespell git, for
versions 1.17 on there are several standard dictionaries.
https://github.com/codespell-project/codespell/tree/v1.17.1/codespell_lib/data

> I created ticket for this:
> https://github.com/codespell-project/codespell/issues/1540

Even if codespell is updated, the script would have to deal
with older versions that don't support requesting that option.

cheers, Joe

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Maxim Uvarov]

On Thu, 4 Jun 2020 at 10:29, Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-06-04 at 09:55 +0300, Maxim Uvarov wrote:
> > On Thu, 4 Jun 2020 at 03:39, Joe Perches <joe@perches.com> wrote:
>
> Hi Maxim.
>
> > > btw: My codespell dictionary file moved to
> > > /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
> > >
> > > and I had to use --codespell --codespellfile=(above) so
> > > maybe there should be multiple lookups for this file
> > > like the array below.
> > >
> > > Are there other standard codespell dictionary locations?
> >
> > It might be better to support standard and non standard locations.
>
> It already does with the --codespellfile=<location> opti.
>

I mean if codespess is in your PATH then it will be good to query
dictinary.txt. Even if you install codespell to your $HOME it still is
possible to use it without manually specifying where is the file.


> > I think it's better to request from codespell where his dictionary is.
>
> Maybe a good idea, but looking at the codespell git, for
> versions 1.17 on there are several standard dictionaries.
> https://github.com/codespell-project/codespell/tree/v1.17.1/codespell_lib/data
>
> > I created ticket for this:
> > https://github.com/codespell-project/codespell/issues/1540
>
> Even if codespell is updated, the script would have to deal
> with older versions that don't support requesting that option.
>

Yes, we have to support old versions.

> cheers, Joe
>
>

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Kees Cook]

On Wed, Jun 03, 2020 at 05:39:47PM -0700, Joe Perches wrote:
> On Wed, 2020-06-03 at 16:19 -0700, Kees Cook wrote:
> > My codespell dictionary has a lot of capitalized words. For example:
> > 
> > MSDOS->MS-DOS
> > 
> > Since checkpatch uses case-insensitive matching, I get an undefined
> > variable warning and then empty suggestions for things like this:
> > 
> > Use of uninitialized value $typo_fix in concatenation (.) or string at ./scripts/checkpatch.pl line 2958.
> > 
> > WARNING: 'msdos' may be misspelled - perhaps ''?
> > +       struct msdos_dir_entry *de;
> > 
> > This fixes the matcher to avoid the warning, but it's still a rather
> > silly suggestion:
> > 
> > WARNING: 'msdos' may be misspelled - perhaps 'MS-DOS'?
> > +       struct msdos_dir_entry *de;
> > 
> > So I'm not really sure what to do with this ... filter out bad
> > suggestions instead?
> 
> Hey Kees.
> 
> Maybe this?
> 
> btw: My codespell dictionary file moved to
> /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt

Yeah, mine too. I think I may have added a symlink to my filesystem to
work around this.

> and I had to use --codespell --codespellfile=(above) so
> maybe there should be multiple lookups for this file
> like the array below.

That seems like a good idea.

> 
> Are there other standard codespell dictionary locations?
> ---
>  scripts/checkpatch.pl | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5f00df2c3f59..52aa0dd53d80 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -59,7 +59,7 @@ my $minimum_perl_version = 5.10.0;
>  my $min_conf_desc_length = 4;
>  my $spelling_file = "$D/spelling.txt";
>  my $codespell = 0;
> -my $codespellfile = "/usr/share/codespell/dictionary.txt";
> +my $codespellfile;
>  my $conststructsfile = "$D/const_structs.checkpatch";
>  my $typedefsfile = "";
>  my $color = "auto";
> @@ -716,7 +716,20 @@ if (open(my $spelling, '<', $spelling_file)) {
>  }
>  
>  if ($codespell) {
> -	if (open(my $spelling, '<', $codespellfile)) {
> +	if (!defined($codespellfile)) {
> +		my @csfiles = ("/usr/share/codespell/dictionary.txt",
> +			       "/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt");
> +		foreach my $csfile (@csfiles) {
> +			if (-f $csfile) {
> +				$codespellfile = $csfile;
> +				last;
> +			}
> +		}
> +	}
> +
> +	if (!defined($codespellfile)) {
> +		warn "No codespell typos will be found - codespell dictionary not found\n";
> +	} elsif (open(my $spelling, '<', $codespellfile)) {
>  		while (<$spelling>) {
>  			my $line = $_;
>  
> @@ -2963,13 +2976,21 @@ sub process {
>  			while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
>  				my $typo = $1;
>  				my $typo_fix = $spelling_fix{lc($typo)};
> -				$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
> -				$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
> +				$typo_fix = $spelling_fix{$typo} if (!defined($typo_fix));
> +				$typo_fix = $spelling_fix{uc($typo)} if (!defined($typo_fix));

This won't catch stuff like:

Cambrige->Cambridge

because neither "cambrige" nor "CAMBRIGE" is in %spelling_fix. And the
original text is lost due to the //i. :( I'm really not sure what to do
with these things in codespell. Lower case everything? Throw away
anything not all lower case?

-- 
Kees Cook

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Joe Perches]

On Thu, 2020-06-04 at 15:08 -0700, Kees Cook wrote:
> On Wed, Jun 03, 2020 at 05:39:47PM -0700, Joe Perches wrote:
> > On Wed, 2020-06-03 at 16:19 -0700, Kees Cook wrote:
> > > My codespell dictionary has a lot of capitalized words. For example:
> > > 
> > > MSDOS->MS-DOS
> > > 
> > > Since checkpatch uses case-insensitive matching, I get an undefined
> > > variable warning and then empty suggestions for things like this:
> > > 
> > > Use of uninitialized value $typo_fix in concatenation (.) or string at ./scripts/checkpatch.pl line 2958.
> > > 
> > > WARNING: 'msdos' may be misspelled - perhaps ''?
> > > +       struct msdos_dir_entry *de;
> > > 
> > > This fixes the matcher to avoid the warning, but it's still a rather
> > > silly suggestion:
> > > 
> > > WARNING: 'msdos' may be misspelled - perhaps 'MS-DOS'?
> > > +       struct msdos_dir_entry *de;
> > > 
> > > So I'm not really sure what to do with this ... filter out bad
> > > suggestions instead?
> > 
> > Hey Kees.
> > 
> > Maybe this?
> > 
> > btw: My codespell dictionary file moved to
> > /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
> 
> Yeah, mine too. I think I may have added a symlink to my filesystem to
> work around this.
> 
> > and I had to use --codespell --codespellfile=(above) so
> > maybe there should be multiple lookups for this file
> > like the array below.
> 
> That seems like a good idea.
> 
> > Are there other standard codespell dictionary locations?
> > ---
> >  scripts/checkpatch.pl | 31 ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 5f00df2c3f59..52aa0dd53d80 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -59,7 +59,7 @@ my $minimum_perl_version = 5.10.0;
> >  my $min_conf_desc_length = 4;
> >  my $spelling_file = "$D/spelling.txt";
> >  my $codespell = 0;
> > -my $codespellfile = "/usr/share/codespell/dictionary.txt";
> > +my $codespellfile;
> >  my $conststructsfile = "$D/const_structs.checkpatch";
> >  my $typedefsfile = "";
> >  my $color = "auto";
> > @@ -716,7 +716,20 @@ if (open(my $spelling, '<', $spelling_file)) {
> >  }
> >  
> >  if ($codespell) {
> > -	if (open(my $spelling, '<', $codespellfile)) {
> > +	if (!defined($codespellfile)) {
> > +		my @csfiles = ("/usr/share/codespell/dictionary.txt",
> > +			       "/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt");
> > +		foreach my $csfile (@csfiles) {
> > +			if (-f $csfile) {
> > +				$codespellfile = $csfile;
> > +				last;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (!defined($codespellfile)) {
> > +		warn "No codespell typos will be found - codespell dictionary not found\n";
> > +	} elsif (open(my $spelling, '<', $codespellfile)) {
> >  		while (<$spelling>) {
> >  			my $line = $_;
> >  
> > @@ -2963,13 +2976,21 @@ sub process {
> >  			while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
> >  				my $typo = $1;
> >  				my $typo_fix = $spelling_fix{lc($typo)};
> > -				$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
> > -				$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
> > +				$typo_fix = $spelling_fix{$typo} if (!defined($typo_fix));
> > +				$typo_fix = $spelling_fix{uc($typo)} if (!defined($typo_fix));
> 
> This won't catch stuff like:
> 
> Cambrige->Cambridge

Huh?  Did you test this?

$ git diff scripts/spelling.txt
diff --git a/scripts/spelling.txt b/scripts/spelling.txt
index d9cd24cf0d40..43c3332935fe 100644
--- a/scripts/spelling.txt
+++ b/scripts/spelling.txt
@@ -251,6 +251,7 @@ callibration||calibration
 callled||called
 calucate||calculate
 calulate||calculate
+Cambridg||Cambridge
 cancelation||cancellation
 cancle||cancel
 capabilites||capabilities

$ cat test_spell.c 
// SPDX-License-Identifier: GPL-2.0-only

/* This is a test.
 * Cambridg
 */

$ ./scripts/checkpatch.pl --strict -f test_spell.c 
CHECK: 'Cambridg' may be misspelled - perhaps 'Cambridge'?
#4: FILE: test_spell.c:4:
+ * Cambridg

total: 0 errors, 0 warnings, 1 checks, 5 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

test_spell.c has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

> because neither "cambrige" nor "CAMBRIGE" is in %spelling_fix. And the
> original text is lost due to the //i. :( I'm really not sure what to do
> with these things in codespell. Lower case everything? Throw away
> anything not all lower case?

Re: [PATCH] checkpatch: Avoid missing typo suggestions

[Kees Cook]

On Thu, Jun 04, 2020 at 06:02:05PM -0700, Joe Perches wrote:
> Huh?  Did you test this?

I didn't, no. I was going off my earlier discoveries about how the
"msdos" thing got parsed weird. My apologies!

> $ ./scripts/checkpatch.pl --strict -f test_spell.c 
> CHECK: 'Cambridg' may be misspelled - perhaps 'Cambridge'?
> #4: FILE: test_spell.c:4:
> + * Cambridg

Thanks for sorting this (and me) out! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook