Name argument if data conversion fails in std.getopt

[burner]

Fixes #9662

revival of #3491 as the suggested "better" solution never happened

[dlang-bot]

Thanks for your pull request, @burner!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10593"

[thewilsonator]

Enforce whitespace before opening parenthesis
grep -nrE "\<(for|foreach|foreach_reverse|if|while|switch|catch|version)\(" $(find etc std -name '*.d') ; test $? -eq 1
std/getopt.d:622:    catch(ConvException e)
std/getopt.d:2006:    catch(GetOptException e)
std/getopt.d:2028:    catch(GetOptException e)

[burner]

@thewilsonator thank you

[thewilsonator]

Looks like this breaks tsv-utils with the change of exception type. cc @jondegenhardt

[thewilsonator]

eBay/tsv-utils#360

[rikkimax]

During this months planning session it was decided that breaking user code that this PR does, as signalled by tsv-utils is not acceptable.

I won't close, as the exception can be changed back.

[burner]

During this months planning session it was decided that breaking user code that this PR does, as signalled by tsv-utils is not acceptable.

Well tsv-utils is somewhat abandoned, meaning it should maybe not be in the buildkit thing, also it only breaks a unit-test, and there is a PR to fix the very strong assumption in the unittest

[LightBender]

@rikkimax The right answer to this is to remove the abandoned package from BuildKite and merge this.

[jmdavis]

Looks like this breaks tsv-utils with the change of exception type. cc @jondegenhardt

Honestly, it would break some of my personal projects as well, since you need to catch ConvException specifically if you want to react to that particular type of failure. Changing the type of an exception to anything other than a subclass of the current exception type is a breaking change. So, if we're looking to avoid breaking changes, we can't do it.

[schveiguy]

I think this can be fixed to be a nicer message with the same exception type, right?

[burner]

I think this can be fixed to be a nicer message with the same exception type, right?

Yes, throwing a ConvException instead of an GetOptException is possible, please feels wrong to me conceptionally.

std.getopt throwing ConvException even though std.getopt has GetOptException. @LightBender @jmdavis maybe you guys should have a think about this in general.

[jmdavis]

I think this can be fixed to be a nicer message with the same exception type, right?

Yes, throwing a ConvException instead of an GetOptException is possible, please feels wrong to me conceptionally.

std.getopt throwing ConvException even though std.getopt has GetOptException. @LightBender @jmdavis maybe you guys should have a think about this in general.

If we were designing it from scratch, then maybe using GetOptException would make more sense (it might still be useful to be able to detect that the conversion failed specifically, but we could have a GetOptConvException or something that derived from GetOptException if we still want that). However, we're talking about code that's been used in the wild for almost as long as D2 has been around, and it's been throwing ConvException that whole time, so code exists which catches ConvException in order to handle that particular error condition appropriately.

So, even if using a different exception type than we currently do would be desirable, we can't do so without breaking existing code (and in this case, the kind of code that's going to break likely won't be on code.dlang.org, because getopt is most frequently used in some place like main in an application and not in a library; so, if anything, it's kind of surprising that the CI testing projects caught it).

If we don't care about breaking that code, then we can make the change, but the decision on that in general is that we don't want to break existing code, often even when it arguably would be desirable to do so.

Personally, I have code that would break with this change, and at least some of it actually gives more detailed information about what's required than can be given with an error message from Phobos, because it depends on what my program is doing, and for that to work, my program needs to know that the conversion failed, not just that getopt failed. So, switching to GetOptException would be worse for that code even if we didn't care about breaking changes (which is why I would then argue for an exception derived from GetOptException like GetOptConvException if we were to switch to GetOptException). Now, I can certainly change my code and live with the results, but if I've written code that catches ConvException from getopt in order to react to that particular error condition, then it's highly likely that there are other people who have done the same.

As a general rule, if we don't want to break existing code, we cannot change exception types unless we change to subclasses, because existing code can catch those specific exception types and act according to what that type was. In that respect, changing getopt to throw GetOptException instead of ConvException would be a lot like changing getopt to simply throw Exception. Any code catching that specific exception type will then no longer function properly. It's a silent change that generally won't be caught until bad input is given to one of the affected programs.

Changing the exception type that a function throws is a lot like changing the return type except that you're probably not going to get a compilation error when you change to a type that wasn't a subclass of what was being thrown before. Instead, you're going to get behavioral changes that can't be caught by the compiler. And the logic for when you can change the exception type without breaking code is roughly the same as with a return type. Subclasses are generally fine, because they can be used by code that expected the original type, but changing to an unrelated type (or to a super type) breaks code. Similarly, adding new, unrelated exception types to be thrown can break code, because code that was written to handle all of the specific exception types that a function threw would then be missing one. So, in practice, we need to be about as careful with which exception types a function throws as we are with what it returns, and arguably, in this case, part of the problem is that we weren't careful enough with getopt when it was introduced.

I have no problem with doing something different with Phobos v3's version of getopt, since then we have an opportunity to redo the design in whatever manner we think is appropriate, and IMHO, making it so that getopt always throws some form of GetOptException (potentially with some derived exception types for specific error conditions) would indeed be an improvement over throwing ConvException so that code that doesn't care about the difference doesn't have to care about why exactly getopt failed (whereas code that does care can catch the appropriate subclass). However, std.getopt's getopt has been throwing ConvException for a very long time, and the only way that it makes sense to change it is if we're okay with breaking existing code.

[schveiguy]

What about GetOptConvException which derives from ConvException? This won't break code, but give you leeway to improve the message.

[LightBender]

@burner will the solution that @schveiguy proposed work for you to resolve the bug?

[burner]

@burner will the solution that @schveiguy proposed work for you to resolve the bug?

Yes, just pushed the change

[burner]

@jmdavis @LightBender maybe have a look here https://youtu.be/7R204VUlzGc?t=1708 I think there are some ideas to copy.

[rikkimax]

I'm ok with the error messages from tsv-utils, they are exposing them directly to the user which is just bad form for a CLI tool.

[LightBender]

Do we need to remove tsv-utils from the BuildKite until they merge a fix? Because it looks like merging this will permanently break buildkite. @rikkimax @schveiguy

[schveiguy]

I'm ok with disabling it at this point. Expecting exact text for error messages is a bit over the top.

[schveiguy]

@LightBender you didn't disable the test? Merging it like this will break all other PRs.

[schveiguy]

buildkite pr: dlang/ci#491

[LightBender]

@schveiguy I thought I could do it after the merge and I was rummaging around for the build file when you sent your PR in.

[schveiguy]

I dislike merging PRs with failing required runs, because it leads to confusing breakage. In this case, it's probably ok, because we know the breakage and will fix it. But for other PRs, they may have this stale state where buildkite is broken, and they are confused as to why it's not working, since it has nothing to do with their code.