WIP: fix #552: use invoke_in_world

[simeonschaub]

Depends on JuliaLang/julia#37902 and still needs to be made backwards-compatible.

fixes #552 (at least on future Julia versions)

[simeonschaub]

Do we need to catch all errors here, or can we only catch certain ones? This bit me during debugging, when I tried to throw an error in the finalizer to get a stacktrace for debugging.

[timholy]

Certainly it's fine to comment out while debugging, or to catch e; isa(e, SomeKindOfError) && throw(e) if SomeKindOfError deserves special handling. What kind of errors did you have in mind?

Off-topic, but since we talked about backtrace() in our other discussion, just checking, do you know about catch_backtrace()? It's sometimes quite useful.

[simeonschaub]

I was more wondering, what kind of errors this was initially supposed to catch, because catching all possible errors seems seems a bit dangerous to me, since we possibly ignore errors the user would like to know about. So I was more thinking of something along the lines of e isa ErrorThatsDistributedsFault || rethrow(), i.e. that the default would be to rethrow errors. Perhaps that's just not possible in this case though.

Off-topic, but since we talked about backtrace() in our other discussion, just checking, do you know about catch_backtrace()?

I do now! 😆

[simeonschaub]

Or can we at least log those errors with something like @warn?

[timholy]

I see your point. This probably is the "lazy" approach, but doing better requires that we anticipate two things:

• it seems likely that many errors might have already been caught by the main process, and so a report from a worker would be redundant. However, I suspect this was more of a concern in previous versions of Revise where there was more try/catch stuff; now, it seems likely that it wouldn't even get here if it fails in the main process.
• workers may not all have the same code---for example, in some cases I've deliberately loaded Gtk only in the main process and had the workers be just computational engines. In such cases, some revise attempts will fail, but it would be wrong to send a warning to the user who would mentally think "I know, I did it on purpose."

As long as we can avoid pestering users incorrectly, I'm all in favor of pestering them when they deserve it 😄.

[simeonschaub]

Is there a way to check whether an error was thrown during regular "revising" or something else? If not, I would propose we just keep it as it is currently, but use either @debug or Revise's logging facilities to log the error, so people who want to know about these kinds of errors can still get them.

[timholy]

The pkgdata might be in revision_errors but the needed info doesn't propagate down here. I think logging it makes sense. EDIT: but it probably doesn't make it here if it triggers an error during regular revision, so you could just give it a whirl? As long as you can distinguish the second point above, perhaps via the exception type?

[timholy]

Doesn't it make more sense to only update the world age at the beginning of a revision? I worry that updating it during each package's deletion phase will just trigger the parent issue all over again. You might not see that in your tests, but add a second package with a deletion: then the update happens before Revise gets a chance to define the replacement, and I believe at that point you are in trouble.

[simeonschaub]

The problem is that macro expansion at least needs to be aware of macros that Revise itself creates when revising code. (Perhaps there should be a separate worldage for that?) This is where the error from @testmac here comes from. I honestly don't quite know how to fix that, because as far as I understand it, for every file Revise revises, it first does all the relevant method deletions and only then evaluates the new definitions. For macros, this means that we would need to update our worldage between those evaluations, because otherwise, later method definitions using those macros wouldn't get macro expanded correctly, or error because the macro wasn't defined in the current worldage yet. If we did update our worldage between each of these definitions though, we wouldn't be able to solve #552 anymore. Do you have any ideas on how this could still work?

[timholy]

Oh, interesting. Perhaps the easiest would be to change the order: handle each package (both deletion and evaluation in one package before moving to the next).

In the old days, doing the deletions first was basically a hack to allow moving a method from one file to another (within the same package) without accidentally deleting it after evaluating in the new location. But Revise is now much smarter about that: it sorts packages and files in dependency order, and it supports having multiple locations transiently. So it seems like it should be safe to switch the strategy now.