Feedback-ID: id4a34324:Fastmail
Date: Mon, 18 Jul 2022 14:50:42 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-committers@lists.postgresql.org,
	Andrew Dunstan <andrew@dunslane.net>
Subject: Re: pgsql: ecpg: Output dir, source dir, stamp file argument for
 preproc/*.
Message-ID: <20220718215042.sl3hivoupdb7lkwv@awork3.anarazel.de>
References: <E1oDWqn-000ADD-Oh@gemulon.postgresql.org>
 <20220718211408.o2uyu5kvud56ihlj@awork3.anarazel.de>
 <822625.1658179641@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <822625.1658179641@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20220718215042.sl3hivoupdb7lkwv%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-07-18 17:27:21 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.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