[bug] Unable to detect if file flag was actually provided?

[rsheasby]

Hi There.

This has been my go-to library for argument parsing, and I recently tried to use this to parse file arguments. However, it seems impossible to determine if a flag was actually provided. For example:

configFile := parser.File("c", "config", etc)
parser.Parse()
if configFile != nil {
    // Do stuff with the file
}
// Do stuff if -c wasn't provided

This does not work, because the configFile is always non-nil, as the parser seems to work by overwriting the internal structures of os.File, and if the argument isn't provided, the configFile will still be non-nil. Furthermore, the standard library doesn't help in this regard, since all the regular functions assume that the file object is valid, so if I try something like configFile.Name(), it just panics.

Is there something I'm missing here, or is there no way to distinguish if the -c flag was provided, without reflection nonsense or panic handling?

[akamensky]

Hey @rsheasby, thanks for the report.

I've added parser.File in the very beginning, but against my own expectations ended up not using it as much. So it is entirely possible some uses cases were overlooked.

Just for the clarity, can you please provide a full example that is not working as you expected (as in entire contents of main package that reproduces this issue)? That would be super helpful to understand what exactly is going on here.

Is there something I'm missing here, or is there no way to distinguish if the -c flag was provided, without reflection nonsense or panic handling?

I am not sure, the full reproducible example would be helpful to understand the use case, however the library is intended to be used without any tricks with reflection, casting etc. The worse case scenario should be dereferencing pointer (which is somewhat unavoidable here).

[akamensky]

Please also clarify, which go version are you compiling/running it with, what OS, system arch (amd64 or something else).

I've just tried a few cases here, all worked as expected:

1. This fails with panic: open ./non-existent-file: no such file or directory

   
   package main

import ( "github.com/akamensky/argparse" "os" "fmt" )

func main() { p := argparse.NewParser("test", "test") fd := p.File("f", "flag1", os.O_RDONLY, 0666, &argparse.Options{Default: "./non-existent-file"}) err := p.Parse(os.Args) if err != nil { panic(err) } fmt.Println(fd) }

2. This fails as well, just as expected:

```go
package main

import (
        "github.com/akamensky/argparse"
        "os"
        "fmt"
)

func main() {
        p := argparse.NewParser("test", "test")
        fd := p.File("f", "flag1", os.O_RDWR, 0666, &argparse.Options{Default: "./non-existent-file"})
        err := p.Parse(os.Args)
        if err != nil {
                panic(err)
        }
        fmt.Println(fd)
}

3. This passes as expected and creates new file and prints fd pointer address:

   
   package main

import ( "github.com/akamensky/argparse" "os" "fmt" )

func main() { p := argparse.NewParser("test", "test") fd := p.File("f", "flag1", os.O_RDWR|os.O_CREATE, 0666, &argparse.Options{Default: "./non-existent-file"}) err := p.Parse(os.Args) if err != nil { panic(err) } fmt.Println(fd) }

All examples were ran with go 1.15, 1.16 and 1.17rc on amd64 Darwin and Linux

[rsheasby]

Basically, the library fails to cater for scenarios when the file flag is optional. Here's an example:

package main

import (
    "log"
    "os"

    "github.com/akamensky/argparse"
)

func main() {
    parser := argparse.NewParser("example", "example")

    // Note this is an optional file. There's no default, and it's not required
    configFile := parser.File("c", "config", os.O_RDONLY, os.FileMode(0), &argparse.Options{
        Help: "config file",
    })

    err := parser.Parse(os.Args)
    if err != nil {
        log.Fatal(err)
    }

    if configFile != nil {
        // Do something if the config file does exist
        log.Println(configFile.Name())
    } else {
        // Do something if the -c flag isn't provided
        log.Println("no config file")
    }
}

And here's what I see if I try run that:

ryanpersonal@Ryans-MacBook-Pro ~/D/playground> go run . -c invalidfile
2021/08/12 18:22:10 open invalidfile: no such file or directory
exit status 1
# This is expected, since the library checks if the file exists, and returns an error otherwise

ryanpersonal@Ryans-MacBook-Pro ~/D/playground [1]> go run . -c main.go
2021/08/12 18:22:14 main.go
# This is expected, since the library will open the file if it exists

ryanpersonal@Ryans-MacBook-Pro ~/D/playground> go run .
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x10b95a8]

goroutine 1 [running]:
os.(*File).Name(...)
    /usr/local/go/src/os/file.go:55
main.main()
    /Users/ryanpersonal/Documents/playground/main.go:25 +0x168
exit status 2
# This is unexpected, since there should be a way to gracefully handle a case where the user hasn't provided a file, since it's an optional argument.

I can understand that changing the nil-ness of the File pointer isn't possible after it's been returned, but it should then instead either return a double pointer (I don't recommend this since it would be a breaking change), or return a pointer to a second boolean value to state if the parameter was actually provided (this is still breaking, but it's much easier to retrofit code to work with this by adding a , _, and this way keeps consistency with the other parameter types)

[akamensky]

@rsheasby thanks, I can reproduce this behavior and I'd say it is not as intended. Let me see what I can do here.

[akamensky]

I looked into this issue and found that nothing much I can do here. os.File is not a builtin type in Golang, but a struct, which means that initializing it as var fd os.File will always provide a valid non-nil pointer, however initializing it as var fd *os.File will not allow this library to pass the file descriptor back to application code. Same is true for builtin types, however those are always initialized with default value.

The simplest solution I see that would not need rewriting half the library is to have a helper method that helps you to check whether the os.File struct is "empty" or not. See #89

[akamensky]

The change is merged and released as v1.3.1.

@rsheasby Please fetch the latest version and see whether that approach works for you.

You can use it as (adjusting the code you provided):

    if !argparse.IsNilFile(configFile) {
        // Do something if the config file does exist
        log.Println(configFile.Name())
    } else {
        // Do something if the -c flag isn't provided
        log.Println("no config file")
    }

There may be a better approach, but I think I would actually reconsider having this functionality in next major version as it is quite problematic to handle in Go, and it is not up to the library to handle file descriptors (it should do only argument parsing IMO).

[rsheasby]

Thanks, looks like that's a solution. Honestly though, I agree that the library probably shouldn't be handling file descriptors. I'd actually recommend just deprecating the function entirely, and possibly removing it from a future major version.