Why does perl warn that open my $fh, $file is missing parentheses?

Question

It is my first day to perl, and I find this warning very confusing.

Parentheses missing around "my" list at ./grep.pl line 10.

It seems

open FILE, $file;

works fine.

What is wrong with

open my $fh, $file;

Thanks!

#!/usr/bin/perl

use strict;
use warnings;

sub grep_all {
        my $pattern = shift;

        while (my $file = shift) {
                open my $fh, $file;
                while (my $line = <$fh>) {
                        if ($line =~ m/$pattern/) {
                                print $line;
                        }   
                }   
        }   
}

grep_all @ARGV;

Answer 1

I've been hacking Perl for more than 15 years, and I admit this warning caused me to scratch my head for a minute because almost every example call to open in the standard Perl documentation and nearly every Perl tutorial in existence contains open with no parentheses, just like you wrote it.

You wrote this question on your first day with Perl, but you're already enabling the strict and warnings pragmata! This is an excellent start.

False starts

An easy but dumb way to “fix” the warning is to disable all warnings. This would be a terrible move! Warnings are meant to help you.

Naïve ways to squelch the warning are abandoning the lexical filehandle in favor of the bad old way with a bareword

open FH, $file;

using explicit parentheses with open

open(my $fh, $file);

making my's parentheses explicit

open my($fh), $file;

using circumscribed parentheses

(open my $fh, $file);

or using 3-argument open.

open my $fh, "<", $file;

I recommend against using any of these by themselves because they all have a severe omission in common.

The best approach

In general, the best way to silence this warning about missing parentheses involves adding no parentheses!

Always check whether open succeeds, e.g.,

open my $fh, $file or die "$0: open $file: $!";

To disable Perl's magic open and treat $file as the literal name of a file—important, for example, when dealing with untrusted user input—use

open my $fh, "<", $file or die "$0: open $file: $!";

Yes, both shut up the warning, but the much more important benefit is your program handles inevitable errors rather than ignoring them and charging ahead anyway.

Read on to understand why you got the warning, helpful hints about your next Perl program, a bit of Perl philosophy, and recommended improvements to your code. Finally, you'll see that your program doesn't require an explicit call to open!

Write helpful error messages

Notice the important components of the error message passed to die:

1. the program that complained ($0)
2. what it tried to do ("open $file")
3. why it failed ($!)

These special variables are documented in perlvar. Develop the habit now of including these important bits in every error message that you'll see—although not necessarily those that users will see. Having all of this important information will save debugging time in the future.

Always check whether open succeeds!

Once again, always check whether open and other system calls succeed! Otherwise, you end up with strange errors:

$ ./mygrep pattern no-such-file
Parentheses missing around "my" list at ./mygrep line 10.
readline() on closed filehandle $fh at ./mygrep line 11.

Explanation of Perl's warnings

Perl's warnings have further explanation in the perldiag documentation, and enabling the diagnostics pragma will look up explanations of any warning that perl emits. With your code, the output is

$ perl -Mdiagnostics ./mygrep pattern no-such-file
Parentheses missing around "my" list at ./mygrep line 10 (#1)
(W parenthesis) You said something like

my $foo, $bar = @_;

when you meant

my ($foo, $bar) = @_;

Remember that my, our, local and state bind tighter than comma.

readline() on closed filehandle $fh at ./mygrep line 11 (#2)
(W closed) The filehandle you're reading from got itself closed sometime before now. Check your control flow.

The -Mdiagnostics command-line option is equivalent to use diagnostics; in your code, but running it as above temporarily enables diagnostic explanations without having to modify your code itself.

Warning #2 is because no-such-file does not exist, but your code unconditionally reads from $fh.

It's puzzling that you see warning #1 at all! This is the first time I recall ever seeing it in association with a call to open. The 5.10.1 documentation has 52 example uses of open involving lexical filehandles, but only two of them have parentheses with my.

It gets curiouser and curiouser:

$ perl -we 'open my $fh, $file'
Name "main::file" used only once: possible typo at -e line 1.
Use of uninitialized value $file in open at -e line 1.

Parentheses are missing, so where's the warning?!

Adding one little semicolon, however, does warn about missing parentheses:

$ perl -we 'open my $fh, $file;'
Parentheses missing around "my" list at -e line 1.
Name "main::file" used only once: possible typo at -e line 1.
Use of uninitialized value $file in open at -e line 1.

Let's look in perl's source to see where the warning comes from.

$ grep -rl 'Parentheses missing' .
./t/lib/warnings/op
./op.c
./pod/perl561delta.pod
./pod/perldiag.pod
./pod/perl56delta.pod

Perl_localize in op.c—which handles my, our, state, and local—contains the following snippet:

/* some heuristics to detect a potential error */
while (*s && (strchr(", \t\n", *s)))
  s++;

while (1) {
  if (*s && strchr("@$%*", *s) && *++s
       && (isALNUM(*s) || UTF8_IS_CONTINUED(*s))) {
    s++;
    sigil = TRUE;
    while (*s && (isALNUM(*s) || UTF8_IS_CONTINUED(*s)))
      s++;
    while (*s && (strchr(", \t\n", *s)))
      s++;
  }
  else
    break;
}
if (sigil && (*s == ';' || *s == '=')) {
  Perl_warner(aTHX_ packWARN(WARN_PARENTHESIS),
    "Parentheses missing around \"%s\" list",
    lex
      ? (PL_parser->in_my == KEY_our
        ? "our"
        : PL_parser->in_my == KEY_state
          ? "state"
          : "my")
      : "local");
}

Notice the comment on the first line. In My Life With Spam, Mark Dominus wrote, “Of course, this is a heuristic, which is a fancy way of saying that it doesn't work.” The heuristic in this case doesn't work either and produces a confusing warning.

The conditional

if (sigil && (*s == ';' || *s == '=')) {

explains why perl -we 'open my $fh, $file' doesn't warn but does with a trailing semicolon. Watch what happens for similar but nonsensical code:

$ perl -we 'open my $fh, $file ='
Parentheses missing around "my" list at -e line 1.
syntax error at -e line 1, at EOF
Execution of -e aborted due to compilation errors.

We get the warning! The 3-argument open case doesn't warn because "<" prevents sigil from becoming true, and the or die ... modifier passes muster, in obtuse terms, because the or token begins with a character other than ; or =.

The intent of the warning appears to be providing a helpful hint for how to fix code that will otherwise produce surprising results, e.g.,

$ perl -lwe 'my $foo, $bar = qw/ baz quux /; print $foo, $bar'
Parentheses missing around "my" list at -e line 1.
Useless use of a constant in void context at -e line 1.
Use of uninitialized value $foo in print at -e line 1.
quux

Here, the warning does make sense, but the case you found is a leak in the heuristic.

Less is more

Perl has syntactic sugar that makes writing Unix-style filters easy, as explained in the perlop documentation.

The null filehandle <> is special: it can be used to emulate the behavior of sed and awk. Input from <> comes either from standard input, or from each file listed on the command line. Here's how it works: the first time <> is evaluated, the @ARGV array is checked, and if it is empty, $ARGV[0] is set to "-", which when opened gives you standard input. The @ARGV array is then processed as a list of filenames. The loop

while (<>) {
  ... # code for each line
}

is equivalent to the following Perl-like pseudo code:

unshift(@ARGV, '-') unless @ARGV;
while ($ARGV = shift) {
  open(ARGV, $ARGV);
  while (<ARGV>) {
    ... # code for each line
  }
}

Using the null filehandle (also known as the diamond operator) makes your code behave like the Unix grep utility.

• filter each line of each file named on the command line, or
• filter each line of the standard input when given only a pattern

The diamond operator also handles at least one corner case that your code doesn't. Note below that bar is present in the input but doesn't appear in the output.

$ cat 0
foo
bar
baz
$ ./mygrep bar 0
Parentheses missing around "my" list at ./mygrep line 10.

Keep reading to see how the diamond operator improves readability, economy of expression, and correctness!

Recommended improvements to your code

#! /usr/bin/env perl

use strict;
use warnings;

die "Usage: $0 pattern [file ..]\n" unless @ARGV >= 1;

my $pattern = shift;

my $compiled = eval { qr/$pattern/ };
die "$0: bad pattern ($pattern):\n$@" unless $compiled;

while (<>) {
  print if /$compiled/;
}

Rather than hardcoding the path to perl, use env to respect the user's PATH.

Rather than blindly assuming the user provided at least a pattern on the command line, check that it's present or give a helpful usage guide otherwise.

Because your pattern lives in a variable, it might change. This is hardly profound, but that means the pattern may need to be recompiled each time your code evaluates /$pattern/, i.e., for each line of input. Using qr// avoids this waste and also provides an opportunity to check that the pattern the user supplied on the command line is a valid regex.

$ ./mygrep ?foo
./mygrep: bad pattern (?foo):
Quantifier follows nothing in regex; marked by <-- HERE in
m/? <-- HERE foo/ at ./mygrep line 10.

The main loop is both idiomatic and compact. The $_ special variable is the default argument for many of Perl's operators, and judicious use helps to emphasize the what rather than the how of the implementation's machinery.

I hope these suggestions help!

Answer 2

my is for declaring a variable or a list of them. It is a common mistake in Perl to write

my $var1, $var2, $var3;

to declare all of them. The warning should advice you to use the correct form:

my ($var1, $var2, $var3);

In your example the code does exactly what you want (you didn't get any errors or wrong results, did you?), but to make it absolutely clear you can write

open my ($fh), $file;

Although one could argue that putting my in the middle of line is like hiding it. Maybe more readable:

my $fh;
open $fh, $file;