Re: pgsql: ecpg: Output dir, source dir, stamp file argument for preproc/*.

Hi,

On 2022-07-18 17:27:21 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Jul 18 16:48:25 ./src/interfaces/ecpg/preproc/parse.pl: Bareword file handle opened at line 30, column 1. See pages 202,204 of PBP. ([InputOutput::ProhibitBarewordFileHandles] Severity: 5)
> > Jul 18 16:48:25 ./src/interfaces/ecpg/preproc/parse.pl: Bareword file handle opened at line 31, column 1. See pages 202,204 of PBP. ([InputOutput::ProhibitBarewordFileHandles] Severity: 5)
>
> > afaict that's bogus. It's unnecessary that the code uses "our" instead of
> > "my", but there's no bareword there and replacing our with my fixes that
> > complaint.
>
> Ah, I think I've got it. Per "man perlfunc":
>
> "our" has the same scoping rules as "my" or "state", meaning that
> it is only valid within a lexical scope. Unlike "my" and "state",
> which both declare new (lexical) variables, "our" only creates an
> alias to an existing variable: a package variable of the same name.

> Since there's not actually any such package variable, the net effect is
> that the first argument of open() is an undeclared name. I can more or
> less see why perl might treat that the same as a bareword, though I
> definitely agree that this error message is more confusing than helpful.

That'd make some sense - but it doesn't look like perlcritic is digging that
deep. Even if I make sure that the our references an existing variable in
package scope, perlcritic's complaint is the the same. I suspect perlcritic
simply has an exception for 'my' somewhere...

[digs a little]

Yep:

/usr/share/perl5/Perl/Critic/Policy/InputOutput/ProhibitBarewordFileHandles.pm
if ( $first_token->isa('PPI::Token::Word') ) {
if ( ($first_token ne 'my') && ($first_token !~ m/^STD(?:IN|OUT|ERR)$/xms ) ) {
return $self->violation( $DESC, $EXPL, $elem );
}

I'll push the obvious fix in a bit.

I find the references to some book, with an abbreviation that I certainly
didn't know and which seems ambiguous even in perl context, not helpful.
--verbose 10 makes the above warning instead be:

Bareword file handle opened at line 31, column 1.
InputOutput::ProhibitBarewordFileHandles (Severity: 5)
Using bareword symbols to refer to file handles is particularly evil
because they are global, and you have no idea if that symbol already
points to some other file handle. You can mitigate some of that risk by
`local'izing the symbol first, but that's pretty ugly. Since Perl 5.6,
you can use an undefined scalar variable as a lexical reference to an
anonymous filehandle. Alternatively, see the IO::Handle or IO::File or
FileHandle modules for an object-oriented approach.

open FH, '<', $some_file; #not ok
open my $fh, '<', $some_file; #ok
my $fh = IO::File->new($some_file); #ok

There are three exceptions: STDIN, STDOUT and STDERR. These three
standard filehandles are always package variables.

Perhaps we should tweak the default format in our perlcriticrc?

Greetings,

Andres Freund