Soft deprecation of LoadError in runtime

[c42f]

Remove use of LoadError from the runtime now that backtraces are more reliable. It's left in boot.jl for now for 1.x compatibility. This is one approach to fixing #31830 (I'm not sure how this interacts with the 1.2 release schedule).

It also partly addresses #31257 as suggested in #31257 (comment)

I'd like to point out that there's several instances of special code for unwrapping LoadError which are removed here, and my feeling is that this is general improvement which will allow people to delete similar code.

I've pulled out the first patch of this change as #31881 because I think that is independently useful and non-breaking. Hopefully that will be resolved quickly in which case I'll rebase this.

Compatibility

I expect that removing this may be difficult from a compatibility point of view and I'm not sure whether this can be considered a minor change. My impression is that packages don't rely on LoadError much in code, but do rely on it in tests. In an informal survey of locally installed packages, I've seen 8/350 with test constructs looking like

    @test_throws LoadError macroexpand(:(@ some_pkg_macro))

and at least one which explicitly unpacks the LoadError.

To make this into as minor a change as possible (avoid breaking such tests) I've grandfather special rules for LoadError into stdlib/Test. However, this won't completely make this into a non-breaking change and the question is whether the remaining breakage is acceptable.

TODO

TODOs which have arisen from the discussion:

• Ensure that errors in macro use get the macro use location in the backtrace.
• NEWS entry
• Consider adding SyntaxError (or existing ParseError?) to replace LoadError(ErrorException("syntax: ... while we're changing these things.

[c42f]

To get a better feel for remaining breakage not mitigated in this PR, I have run the tests of all 22/350 packages which I have installed locally and which mention LoadError somewhere in their text. The results:

• 15 packages passed tests (ArgParse DICOM EponymTuples FastClosures IJulia ImageCore Interpolations JuMP Parameters PlotlyJS PyCall SimpleTraits SoftGlobalScope Unitful UnitfulUS). I believe most of these would have failed without the mitigation in Test.
• 1 package broke due to the changes here (Revise), though it was also broken by the previous round of LoadError changes.
• 5 packages failed for superficially unrelated reasons (GDAL Blink Plots Rebugger StaticArrays)

This is surprisingly encouraging, though it's by no means a systematic survey of all packages. I'm not sure I have the resources to cast the net wider though.

[c42f]

[Edit: I think the following questions have been answered sufficiently by the discussion below]

Questions for triage

I've attached the triage label because I'd like some feedback in regard to the 1.2 release:

• This is technically breaking. Do people feel this is "minor change" enough to be 1.x material?
• If so, should we do this in 1.2? (If not, we have at least two other solutions, as mentioned in Regression in stacktrace collection with macros #31830 (comment))

Apologies if this comes late in the release cycle, I don't know where we are in preparing the RC.

[timholy]

If the only breakages are in internal guts of packages like Revise and the debugger stack, it's not worth stressing over: these packages make heavy usage of Julia's internal representations of code, so are fair game for breakage as far as I am concerned. (I don't enjoy the maintenance burden, but "freezing" the development of Julia's internals would be far worse.) OTOH, if they are just flagging things that look like the kind of code patterns that might appear in more enduser-oriented packages, there's more to think about. What's the specific breakage error message in Revise?

[c42f]

The failure in the Revise tests is quite mundane. It's simply because LoadError is no longer thrown, so explicitly testing for it for example in the following lines doesn't work:

https://github.com/timholy/Revise.jl/blob/4d4cdff9c07b8fe689d9273a2e62caaa3e586f55/test/runtests.jl#L1869-L1870

This is definitely a pattern which could occur elsewhere and probably does, though much less than I originally feared. Also much more likely to occur as a spurious test failure than an actual failure in package code.

[c42f]

Message needs changing if this was to go into 1.2

[ararslan]

I'd say this is probably a fine candidate to go into a 1.x release, but it's too late for 1.2.

[JeffBezanson]

I recall discussing somewhere that the type of a thrown exception is allowed to change, and I believe there have already been 1.x changes of that nature.

[c42f]

the type of a thrown exception is allowed to change

If that's the case, it occurs to me that a logical step related to this PR might be to replace a lot of what are ErrorException("syntax: ...") with a SyntaxError type.

[timholy]

I'd say this is probably a fine candidate to go into a 1.x release, but it's too late for 1.2.

To clarify, something has to go into 1.2, even if it's just a NEWS item announcing a breaking change. See #31830.

[stevengj]

• Packages that have @test_throws LoadError: ArgParse, BioSequences.jl, Blobs.jl, CUDAdrv.jl, Classes.jl, DICOM.jl, Debugger.jl, DeferredFutures.jl, DistributedArrays.jl, EponymTuples.jl, Jute.jl, MLStyle.jl, Mimi.jl, NBInclude.jl, POMDPs.jl, Parameters.jl, Rematch.jl, StaticArrays.jl, StrLiterals.jl, Unitful.jl, UnitfulUS.jl

• Packages that explicitly detect and unwrap LoadError: Atom.jl, ExpressionPatterns.jl, FastClosures.jl, FortranFiles.jl, IJulia.jl, Jive.jl, Kwonly.jl, Reconstructables.jl, YAJL.jl

• Packages that throw LoadError: Espresso.jl, Revise.jl, SoftGlobalScope.jl, Sugar.jl

• Other usage: DataKnots.jl, Hecke.jl, Instruments.jl, Jewel.jl, PyCall.jl, RequestsCache.jl

It seems likely that this change will break at least some of the above? Would be good to look more closely, at least.

[JeffBezanson]

My view of #31830 is that it doesn't really make sense to throw LoadError for an error during macro expansion --- it's not necessarily loading a file, and if we are loading a file the LoadError will be added later by include. However it looks to me like we're doing that so you can see the location of the macro call that triggered the error. Do you see the location of the macro call after this change?

So for 1.2, I think we should just revert the jl_rethrow to jl_throw change to fix the regression, and deal with removing LoadError later.

[c42f]

However it looks to me like we're doing that so you can see the location of the macro call that triggered the error. Do you see the location of the macro call after this change?

Oh, good catch. I thought this was fixed by #31881, but that only fixes a subset of these kind of problems, unfortunately not including macro invocation. So either we need an equivalent of LoadError to convey that information, or ideally figure out how to get a frame which is visible to julia users into the backtrace. A thought strikes me that both jl_parse_eval_all and jl_toplevel_eval_flex are "interpreter-like" so perhaps with some work they could piggyback on the interpreter backtrace machinery.

[JeffBezanson]

We could add a MacroExpandError and wrap with that instead. Putting the macro call location in the backtrace is not essential, since that code is not actually running. In any case I think wrapping and re-throwing seems like the right thing here, since this isn't really a nested exception.

[c42f]

A MacroExpandError is simple to implement and could communicate additional state (eg, the partially expanded expression being processed). Having said that, I still think it would be useful and intuitive if backtraces always had a frame corresponding to the top level code being processed, including during macro expansion.

[StefanKarpinski]

Status? Seems like this isn't a 1.5 blocker.

[c42f]

Correct, not a 1.5 blocker in any way. There's some design decisions and certainly some implementation to finish here.