Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl-Sensitive Sunglasses
 
PerlMonks  

Propose addition to use warnings?

by perldigious (Priest)
on Nov 28, 2016 at 14:30 UTC ( [id://1176706]=perlmeditation: print w/replies, xml ) Need Help??

I'm not sure what the official mechanism for making such a suggestion would be, but I recently had to go on a bug hunt in my code that left me surprised use warnings didn't give me some sort of, "are you SURE you aren't doing something silly here at line x?" message. Maybe it isn't practical to expect use warnings to save me from myself in this case, but I figured I'd throw it out here as a meditation for discussion and to get opinions from Monks far wiser than I am.

I'm of course simplifying and paraphrasing, but essentially I had some code that did the following:

open(my $first_fh, "<", "file1.txt") or die "Cannot open \"file1.txt\" +: $!."; open(my $second_fh, "<", "file2.txt") or die "Cannot open \"file2.txt\ +": $!."; my @file1_lines = <$first_fh>; close $first_fh; while (my $line = <$second_fh>) { my @data = my_sub($first_fh, $line); # copy/paste error, should ha +ve passed $second_fh here # and of course a bunch of non-relevant stuff here }

So I accidentally passed the $first_fh that I had previously closed to my_sub, but as it turns out use warnings didn't make a peep about this. I did of course get warnings from inside my_sub when it attempted to make use of the filehandle, but the kicker was that any such use was wrapped inside of a conditional that actually made it very rare it would ever get used (maybe once every 100,000 lines or so of typical data).

DISCLAIMER: The fact that I didn't have something in my standard test data that ensured the conditional that used the filehandle was exercised is entirely on me (forgive me, for I have sinned). I accept full responsibility for that mistake and the initial copy/paste laziness error that forced me to engage in my bug hunt. I'm only pointing out that usually use strict, use warnings, or perl itself is pretty good about preemptively saving me from myself, and in this case it didn't.

Any thoughts from the Monastery?

Just another Perl hooker - will code for food

Replies are listed 'Best First'.
Re: Propose addition to use warnings?
by choroba (Cardinal) on Nov 28, 2016 at 14:47 UTC
    I don't understand your question. Do you mean Perl should warn when you pass a closed filehandle to a subroutine?

    There's nothing inherently wrong in passing a filehandle:

    #!/usr/bin/perl use warnings; use strict; main(@ARGV); sub main { my ($file1, $file2) = @_; open my $fh, '<', $file1 or die $!; print "first: $_" while <$fh>; close $fh; again($fh, $file2); print "again: $_" while <$fh>; } sub again { my ($fh, $name) = @_; open $fh, '<', $name or die $!; }

    ($q=q:Sq=~/;[c](.)(.)/;chr(-||-|5+lengthSq)`"S|oS2"`map{chr |+ord }map{substrSq`S_+|`|}3E|-|`7**2-3:)=~y+S|`+$1,++print+eval$q,q,a,

      Do you mean Perl should warn when you pass a closed filehandle to a subroutine?

      Yes, that's what I was asking about, but reading davido's reply makes me think that may not be a reasonable expectation. Perl just usually spoils me by telling me when I'm trying to do something that's probably stupid, and this is a case where I was and it didn't. :-)

      Just another Perl hooker - will code for food

        Well to perl's credit, it *did*, but just later on, and inside of an edge-case conditional statement ;)

Re: Propose addition to use warnings?
by haukex (Archbishop) on Nov 28, 2016 at 15:20 UTC

    Hi perldigious,

    To answer the other part of your question: If you wanted to propose a new warning, you'd do so to the Perl 5 Porters, perhaps as a new "wishlist" priority bug report. However, from being a lurker on P5P for quite some time, I don't think a new warning would be added without there being a lot of usefulness to it, and the warning, its implications, implementation etc. need to be well thought-out. Also, a lot of things can be done outside core Perl, for example, obvious re-uses of a filehandle, at least in the same lexical scope, could be detected with a Perl::Critic policy, perhaps based on a modified Perl::Critic::Policy::InputOutput::RequireBriefOpen. Of course, another way to prevent your bug would be to be more careful with the lexical scoping of filehandle variables ;-)

    Hope this helps,
    -- Hauke D

      Thanks haukex, good to know what the mechanism is, though I sort of figured it would be presumptuous of me to suggest it officially without asking the Monastery first, which is why I did.

      Of course, another way to prevent your bug would be to be more careful with the lexical scoping of filehandle variables

      Yes, that is most certainly the best solution. I have this habit of declaring all my filehandles in the global scope and opening them all right away as a first step in my programs just to force the user to make sure all the the input files are accounted for before my program goes off and spends a bunch of time processing some of them only to find later one or more is missing and it has to die anyway. It's probably an amateurish thing to do.

      Just another Perl hooker - will code for food
Re: Propose addition to use warnings?
by Corion (Patriarch) on Nov 28, 2016 at 14:41 UTC

    You must have done something weird in my_sub because for me, warnings already tell me that I'm using a closed filehandle:

    #!/usr/bin/perl use strict; use warnings; open my $fh, '<', $0 or die "I can't read myself"; open my $fh2, '<', $0 or die "I can't read myself (2)"; my @lines = <$fh>; close $fh; sub my_sub { my( $this_fh, $line ) = @_; warn sprintf "At offset %d: %s", tell $this_fh, $line; }; while (my $line = <$fh2>) { my @data = my_sub($fh, $line); # copy/paste error, should have pas +sed $second_fh here # and of course a bunch of non-relevant stuff here } __END__ tell() on closed filehandle $fh at tmp.pl line 15. At offset -1: #!/usr/bin/perl tell() on closed filehandle $fh at tmp.pl line 15. At offset -1: ...

    Maybe you can show us what you were doing in my_sub with the filehandle and where Perl didn't warn there?

      Hi Corion,

      The way I understood perldigious' post is that the usage of the filehandle in my_sub is very rare, so more like warn sprintf "At offset %d: %s", tell $this_fh, $line if rand()<0.0001;, which makes the issue much harder to detect.

      Regards,
      -- Hauke D

        Yes, thanks for clarifying for me haukex. Corion is correct, there is a warning provided when the filehandle is attempted to be used by the subroutine, but that is a very rare occurrence due to an if it happens to be wrapped in, and to my shame, my test data didn't properly exercise that part of my code to make sure it was working when I wrote it (an oversight on my part).

        Just another Perl hooker - will code for food
Re: Propose addition to use warnings?
by haukex (Archbishop) on Nov 28, 2016 at 14:41 UTC

    Hi perldigious,

    So as I understand it you would like the warning to be triggered when you use $first_fh as a parameter to my_sub? Note that's it's perfectly fine to re-use $first_fh after it was closed, so the warning can't just be "filehandle used after closing" or something similar, the trigger needs to be something more specific. But how Perl should know that the first parameter to my_sub needs to be an opened filehandle? This sounds like a case for parameter validation to me. At the top of my_sub: my $fh = shift; croak "argument must be an opened filehandle" unless ref $fh eq 'GLOB' && $fh->opened;. Update: Probably better: At the top of your script use Scalar::Util qw/openhandle/;, and at the top of my_sub: croak "argument must be an open filehandle" unless openhandle($fh); (Update 2: But see also Best way to check if something is a file handle?, identifying every kind filehandle-like thing appears to be nontrivial.)

    Regards,
    -- Hauke D

      Indeed haukex, I agree that validation for a filehandle would be good if it was a trivial thing to do. I couldn't have told you whether it was or wasn't off the top of my head, so thanks for the link.

      The possible re-use of the scalar $first_fh did occur to me, but that's why I initially figured a warning might have been appropriate since someone could explicitly ignore that warning if they decided to re-use the variable name. I sort of leaned toward the, "well wouldn't that be kind of sloppy to re-use it anyway?" argument when it occurred to me, but I wouldn't be surprised if there were cases where it made good sense to do so.

      Again, I did just really want the "filehandle used after closing" warning, though I can easily accept the answer that it's impractical.

      Just another Perl hooker - will code for food

        Hi perldigious,

        ... I wouldn't be surprised if there were cases where it made good sense to do so.

        I think choroba showed an excellent example of code where it's perfectly ok to pass a closed filehandle to a sub. I think usually it's the job of modules like Perl::Critic to enforce a certain coding style, not the job of warnings.

        I agree that validation for a filehandle would be good if it was a trivial thing to do.

        The code I showed with Scalar::Util will work in many cases, but not every single one. So it really depends on whether you're coding a module which you intend to release to many users, where the chances are good that someone might try to pass in a valid object that fails the test, or whether you're coding "just" a script where you've got a good overview of the values that are being passed to the sub. If it's the latter, then my approach would probably be to just use the code I showed, and if someday I decide I want to pass in something that fails the check, I can always reevaluate then.

        Regarding your other reply about opening the files early in your scripts, note you could use file tests to check on the files at the start of the script so you don't have to open them right away. There is a small chance they might get deleted in between the file test and the open, but there's plenty of situations where one can safely assume that won't happen.

        Regards,
        -- Hauke D

Re: Propose addition to use warnings?
by davido (Cardinal) on Nov 28, 2016 at 16:55 UTC

    This isn't likely to go anywhere. Here's why:

    Unless a prototype disallows it, it's always legal to pass an in-scope lexical scalar variable to a subroutine call. Perl doesn't really care at compiletime what the subroutine actually does with parameters passed into it. So the only safeguard Perl can reasonably do is complain if you make some system call that requires a filehandle, and pass in a closed one. That's a runtime check though, because the compiler doesn't grok the logic enough to know that a given variable is in some particular state.

    But Perl already does complain when you attempt to do some file operation on a closed filehandle -- that already generates a warning. In your case, the situation comes up infrequently, so the warning is latent, and not likely to appear right away. But I do think Perl is doing the best it can in that regard already. At what line in the code you posted should Perl kknow you're doing something nutty?

    my_sub($first_fh,...

    ...that's not a reasonable place, because Perl doesn't really know the purpose of my_sub, and doesn't know at this point what you are planning to do with $first_fh.


    Dave

      Thanks davido, I figured I may be asking for too much, but I wasn't sure so I asked the Monastery.

      ...that's not a reasonable place, because Perl doesn't really know the purpose of my_sub, and doesn't know at this point what you are planning to do with $first_fh.

      That is where I would have requested the warning to be given, basically a "you already closed this filehandle, are you sure you still want to be using it for anything at all?" sort of warning. If it's not reasonable it's not reasonable. I suppose I'm just being the guy trying to blame his tools when it's actually his own ineptitude at using them that's the problem. :-)

      Just another Perl hooker - will code for food

        I didn't mean for "not reasonable" to sound like your request is unreasonable. Just that setting the warning there isn't ideal because my_sub could be something like:

        sub my_sub { my $closed_handle = shift; die "For some reason our handle isn't closed yet." if $closed_handle->opened; }

        ...or

        close $fh or my_sub($fh); sub my_sub { my $handle = shift; my $error = $handle->error; die "Got this error code: $error\n" if $error; }

        ...or even...

        # Open a handle only if it's not opened. sub my_sub { open $_[0], '<', $_[1] if !$_[0]->opened; }

        Dave

Re: Propose addition to use warnings?
by kcott (Archbishop) on Nov 29, 2016 at 00:15 UTC

    G'day perldigious,

    I looked at this from the perspective of using a lexical filehandle (which is good) but then giving that filehandle global scope (which is less good). Consider this version of your code:

    #!/usr/bin/env perl use strict; use warnings; use autodie; { open(my $first_fh, "<", "file1.txt"); my @file1_lines = <$first_fh>; } { open(my $second_fh, "<", "file2.txt"); while (my $line = <$second_fh>) { my @data = my_sub($first_fh, $line); } } sub my_sub { 1 }

    All the I/O involving $first_fh is performed in the first anonymous block. On exiting that block, $first_fh goes out of scope, Perl automatically closes it for you, and strict will warn you about it being an argument to &my_sub.

    Running that code gives a compile-time error:

    Global symbol "$first_fh" requires explicit package name ... line 16. Execution ... aborted due to compilation errors.

    Now's the time for that Oops! moment you were hoping for but didn't get. :-)

    After s/first/second/ the code compiles cleanly, as evidenced by the runtime error I get:

    Can't open 'file1.txt' for reading: 'No such file or directory' ... li +ne 8

    As a general rule, use lexical variables in the smallest scope possible.

    — Ken

      Thanks kcott, I believe the added scoping you show is one of the things haukex had previously alluded to (at least that's how I interpreted his suggestion). As I had previously mentioned, I have a not so great habit of opening all my required input files immediately near the top of all my programs (essentially making all filehandles global) just to make sure they exist. I do this so users can figure out immediately if they forgot a certain input file and don't wait for my program to grind away processing the other ones before the program has to die. As haukex also pointed out, a simple file test would be a better way to go about that, presumably a series of simple -e tests replacing all the opens at the top of my program, and then the more constrained scoping as I open and process each file.

      Just another Perl hooker - will code for food

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlmeditation [id://1176706]
Approved by Corion
Front-paged by kcott
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others sharing their wisdom with the Monastery: (5)
As of 2024-03-29 12:15 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found