Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: Warn that thrown exceptions cannot be relied on #50669

Closed
wants to merge 1 commit into from

Conversation

jakobnissen
Copy link
Contributor

The specific exception type thrown by Base and the standard libraries is an internal implementation detail, and should not be relied on by users. Document this in the docstring of Test.@test_throws, and mention it in the manual section on try/catch.

The specific exception type thrown by Base and the standard libraries is
an internal implementation detail, and should not be relied on by users.
Document this in the docstring of `Test.@test_throws`, and mention it in the
manual section on try/catch.
@Seelengrab
Copy link
Contributor

Seelengrab commented Jul 25, 2023

This very much feels like another thing thrown on the pile of "you can't rely on anything not explicitly written out somewhere". This is the kind of stuff that makes working in/with julia frustrating and needlessly difficult for newcomers and existing devs alike.

@jakobnissen
Copy link
Contributor Author

I agree! However, se #50633 . Given that we currently can't actually rely on exceptions, I think it's better for users to know it when they write their code or tests than for them to be surprised when it breaks.

Unless of course you think that writing it out in the official sige would lead to even more breakage, and make it harder to decide that exceptions must not change, since it's now officially sanctioned that they can?

@timholy
Copy link
Member

timholy commented Jul 26, 2023

While I sympathize with the pain from the standpoint of a package developer (I've had to update lots of tests, too), I think this would be a step backwards. I'd recommend @test "appears in error message" foo(args...); still not sure-fire from the standpoint of "will I ever have to change this test again?", but it is possible to change the type without modifying the message. And it focuses on the important thing, the information conveyed to the user. Some discussion appears in #41888.

I'd also remind people that for all the pain, being able to improve our error messages is probably one of the more important things we need to do in the ecosystem to make Julia more user friendly. Should JuliaSyntax not be generating oh-so-much-prettier error messages for parsing errors? That would be an enormous loss. This "breakage" isn't true breakage: the operation failed before, and it still fails, we're just doing a better job of "teaching" the user what went wrong. It's a top-tier priority for the ecosystem and an inconvenience only to those of us with the skills and experience to cope.

@jakobnissen
Copy link
Contributor Author

jakobnissen commented Jul 26, 2023

I agree somewhat. The problem of course is that:

  1. "the operation failed before, and it still fails" is precisely not true for the @test_throws, or for try/catch. This worked, and therefore users can, and do, write tests and/or code relying on stuff like catch e; if e isa EOFError.

  2. It's not always true that what matters is the message displayed to the user. Specifically, this does not matter when the exception is handled programmatically with try/catch, such that the message is never displayed.

However, I agree that this PR is not a great solution either. The problems fundamentally are that:

  1. If the take home message is that no, we can't rely on exception types (or messages) being stable, then we don't really have support for error handling in Julia. Which I think is a problem in a programming language, given that Julia uses exceptions as the main way to signal errors, and

  2. It's highly misleading to present @test_throws and the pattern cactch e; if e isa ArgumentError as recommended example code for our users to emulate and then to break it. I think we have a cultural problem with underspecifying the behaviour of Julia code, and then casually breaking people's code, and I would like to see us improve in that area. Ideally by not actually breaking code, but at the very least by making it discoverable and learnable what can be relied on.

I would also argue against your point that:

[Improved error messages is] an inconvenience only to those of us with the skills and experience to cope.

I wrote a Julia codebase in my previous job, and then when I quit my job, I handed my coworkers a set of programs backed by a large-ish legacy Julia codebase that they do not necessarily understand. I think it's a problem for the language that I don't feel confident at all that the code I wrote now two years ago still works despite my best efforts and tests (not just due to this particular issue, but many just like it). These are one of the factors that would seriously reduce my enthusiasm for me recommending Julia in cases where you don't have an expert Julian who can do ongoing maintenance.

So for me, this low level of breakage is not primarily a matter of an increased maintenance burden for packages I maintain (though that certainly is annoying as well), it's a big problem for the ability to write programs that you don't maintain, for example because you quit your job, or in the case of the several Julians who contribute to our open source community and then disappear from the community, after which their package spontaneously break.

@Seelengrab
Copy link
Contributor

Without talking about whether the exception type/message should be allowed to change at all, I think the vast majority of cases where this kind of breakage comes up is where the existing error/failure cases aren't documented at all, hence allowing the kind of wild-west, cowboy style "anything goes" breakage in the first place (it wasn't documented after all..). It's trivial to find examples of this - this kind of non-documentation of failure modes is pervasive throughout both Base and the ecosystem, and if we'd document them we'd at the very least have an incentive to think about the exception type (for example).

I consider raw error("foo") to be a horrific antipattern, and explicitly documenting that no exception type anywhere can be relied upon seems to me to be the worst of all worlds.

@jakobnissen
Copy link
Contributor Author

Okay that's fair. Given that the change in PR could contribute to the status quo (of breakage) being more cemented, it's not clear to me that this PR is an overall improvement. So, I'm closing this.

@timholy
Copy link
Member

timholy commented Jul 31, 2023

"the operation failed before, and it still fails" is precisely not true for the @test_throws, or for try/catch. This worked, and therefore users can, and do, write tests and/or code relying on stuff like catch e; if e isa EOFError.

I guess I meant "fails only in tests" which are in a sense not user code.

I agree with you that this is painful and has downsides. And perhaps in this case the message isn't any better. In which case perhaps we could revert that aspect of the change? One could try submitting a PR and seeing if it flies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants