[suggestion] Check that `.parse()` is called.

[h-2]

a small convenience feature: it is easy to forget calling .parse(). It would help if the destructor of the parser checks if the function was called and throws a logic_error("You forgot to call .parse()") if it was not the case.

Note, that you explicitly need to mark a destructor as noexcept(false) if you want to throw from it.

[smehringer]

Thanks for the input, sounds like a good idea!

[eseiler]

I played around a bit:

Details

diff --git a/include/sharg/parser.hpp b/include/sharg/parser.hpp
index b9ffe71..be4c54c 100644
--- a/include/sharg/parser.hpp
+++ b/include/sharg/parser.hpp
@@ -211,12 +211,17 @@ public:
         init(argc, argv);
     }
 
-    //!\brief The destructor.
-    ~parser()
+    /*!\brief The destructor.
+     * \throws sharg::design_error if sharg::parser::parse() was not called.
+     */
+    ~parser() noexcept(false)
     {
         // wait for another 3 seconds
         if (version_check_future.valid())
             version_check_future.wait_for(std::chrono::seconds(3));
+
+        if (!parse_was_called && std::uncaught_exceptions() == 0)
+            throw design_error{"The function parse() was not called for the app " + info.app_name + "!"};
     }
     //!\}
 
@@ -404,7 +409,7 @@ public:
     void parse()
     {
         if (parse_was_called)
-            throw design_error("The function parse() must only be called once!");
+            throw design_error{"The function parse() must only be called once for the app " + info.app_name + "!"};
 
         detail::version_checker app_version{info.app_name, info.version, info.url};
 

The problem is that stuff like

#include <ctime>

#include <sharg/parser.hpp>

int main()
{
    std::string option_value;

    char const * argv[] = {"./parser_test", "--version-check", "false", "-s", "option_string"};
    sharg::parser parser{"test_parser", 5, argv};
    parser.add_option(option_value, sharg::config{.short_id = 's'});

    std::srand(std::time(nullptr));
    int const random_variable = std::rand();

    try
    {
        if (random_variable % 3 != 0)
            throw std::invalid_argument{"moo"};
    }
    catch (std::exception const &)
    {
        return 1;
    }

    parser.parse();
    return 0;
}

wouldn't work anymore. The example is a bit weird, but the basic principle is that if calling parse() after you have constructed the parser is not ensured, you might get an exception:

$ ./example || echo $? # std::invalid_argument is thrown and caught
terminate called after throwing an instance of 'sharg::design_error'
  what():  The function parse() was not called for the app test_parser!
Aborted
134 # != 1
$ ./example || echo $? # std::invalid_argument is not thrown

You could, e.g., add another try/catch starting from sharg::parser parser until parser.parse() and kinda just return 1 again...

I can ensure with std::uncaught_exceptions() that I only throw in the destructor if there are no uncaught exceptions, but as soon as something is handled via try/catch, it's not uncaught anymore.
The other question is, of course, whether such a scenario would happen in the wild. Still, I wanted to point it out. Something like this might happen when you have multiple sub-commands, and basically have a main try/catch in your main that just prints error messages.

I am not sure if this can be implemented without breaking any use cases, and also how much we should care about maybe-esoteric use cases.

P.S.: For the tests, we can just set parser.parse_was_called = true via the test_accessor where we do not care about parse() being called (which is often).

[h-2]

The example is a bit weird

Yeah, I don't understand in which case you need an exit-path in the application between parser construction and calling .parse(). In my opinion, all you should do inbetween is add your options. But maybe I am missing something?

the basic principle is that if calling parse() after you have constructed the parser is not ensured, you might get an exception:

Yes, that's the point :)

I can ensure with std::uncaught_exceptions() that I only throw in the destructor if there are no uncaught exceptions,

Yep, that's something you always need to do.

Something like this might happen when you have multiple sub-commands, and basically have a main try/catch in your main that just prints error messages.

I do this, but I am not seeing the problem. To invoke the subcommand parser, I need to have called the main parser's .parse(), otherwise I wouldn't know which subcommand was issued. So from my POV, the invocations are clearly sequenced and there is no chance to have the subparser throw before the main-parser has been parsed.

I am not sure if this can be implemented without breaking any use cases, and also how much we should care about maybe-esoteric use cases.

I think there is a clear intention of how the parser can and should be used, and implementing this check is in line with that. If it breaks user code, they were doing something funky before (or likely have a bug in their code). And they can easily fix it!

[smehringer]

Hmm interesting edge case. I'd also consider this unlikely, since parsing is usually done before anything else.

I can ensure with std::uncaught_exceptions() that I only throw in the destructor if there are no uncaught exceptions,

Would that "fix" your example? ("fix" as in "well there is a parse() somewhere in the code" so I don't throw a sharg exception in neither case), no right because the exception is caught?

I'm not sure if we want/can to enforce that "parse() is called before program exits" or "parse is called somewhere in the program". The former being a semantic API break.

[h-2]

I think a few things have been conflated here. It is not relevant if an exception is thrown before iff that exception is caught.

The two actual problems are:

1. An exception is thrown that is not caught. This problem can be fixed in the library by checking std::uncaught_exceptions().
2. A return happens before .parse() is called. This cannot be fixed, because this is what we are trying to detect 🙂

I'm not sure if we want/can to enforce that "parse() is called before program exits" [...] being a semantic API break.

Like I said, I think the API is

1. create parser
2. add options
3. parse

If you don't call parse, you have a bug. Or what would be the use-case for creating a parser that is not parsed? Is there anything else you can do with it?

That having been said, this is just a minor convenience feature, and we shouldn't spend more time discussing it. If you are not convinced, just feel free to close this issue!

[eseiler]

I'll discuss it with @smehringer soon. I think we can add it. Maybe after changing the ctor to take a config, so we don't have to adjust the tests twice. Adjusting the tests for this feature is a bit finicky, because we have to use the test accessor to allow not calling parse where we don't want to call parse.
Because the config PR will also change how the app name is set, these tests (at least) will call parse, and we don't need to use the accessor there.

[eseiler]

After #238, this is probably a lot more straightforward. Only parse() should throw (before, also the constructor or certain member functions threw).

For the tests, we could have the test accessor set parse_was_called where necessary.