Improve error handling for registering callbacks

[vlasakm]

When we initially introduced support for luatexbase / ltluatex style callbacks (luatexbase.add_to_callback, etc.), we introduced also some checking and reported some errors with err, though we were not as thorough as the original LaTeX code, because we mainly aimed to provide compatibility with existing packages, which already were using the functions correctly, as they had to go through LaTeX's more extensive checking.

As time progressed, we in OpTeX added more uses of the callbacks, and OpTeX users themselves have been using the mechanism, since in OpTeX we don't allow direct registering of callbacks, it is always necessary to go through callback.add_to_callback. However, this mean that some uses could not obey the "rules", while we were expecting some invariants to hold true.

One such problem manifested for callbacks added without descriptions. Internally we expected that each callback function would be registered with a description, and that a description wouldn't be used more than once for a single callback name.

This was not an immediate problem when adding the callbacks, but could lead to problems when removing from callbacks, since that is where we assume some consistency in the registered functions, and descriptions. If the description was nil (i.e. not passed) the internal list of functions and descriptions could drift away, causing callbacks to be deregistered prematurely, as the condition for deregistration is that there are no descriptions left.

The solution implemented here is a more thorough checking of arguments and consistency.

As part of the consistency checking, I found that our use of "finish_pdffile" callback itself didn't conform to the invariants (had no description), further suggesting, that such mistakes are easy to make accidentally. That is also fixed here.

[olsak]

When I applied this path a new problem appeared. \load inside \load is impossible. Try \load[minim-mp]. The minim-mp.opm file includes another \load. We get:

error: for callback 'process_input_buffer' there already is '_mathsb' added

The \load macro should be corrected...

[olsak]

Maybe, there is better idea to do nothing if there is a callback with the same description registered already, print no error. It seems to be more simple than to check if the callback is registered already from macro level.

[vlasakm]

Hm, it seems I was too eager with the checks. LuaLaTeX seems to allow duplicate entries.

[olsak]

I tried to replace the line 240 (err(...)) by return command. It seems that this is what we want: do nothing.

[vlasakm]

I think we should allow duplicate entries like we did until now - LaTeX seems to do as well.

Though I would prefer the error myself - it seems that if there is an accidental clash, and different packages use the same description, then unexpected things can happen for removals.

If we allow multiple entires and removal is attempted, we will remove the first. LaTeX seems to do the same. So just removing the check seems fine to me.

Registering the \mathsb callback multiple times is probably something we should avoid in the "purity" sense, but it doesn't seem that important, as running the callback multiple times on the same input hasn't been a problem.

[olsak]

I m not sure that this is the same as my return command. If somebody register the same callback twice then it is run twice or not? Desired behavior is (IMHO) to simply ignore the second callback.add. The _mathsb can be run twice because it does not any changes of the data stream by the second instance. But there can be another callback function which does something in both instances. If a user want to run the function explicitly twice then he can register it with different description.

[vlasakm]

Yes, what I suggest is different from yours.

So in this pull request, we merged the approach of emitting an error on duplicates. Previously, we silently allowed them, and if removal was requested, we removed the first one registered (i.e. First In - First Out).

You suggest to silently discard duplicates. I suggest to stick to orignal behavior, because that's what we have been doing, and is also what LaTeX does.

Imagine:

% package A
add_to_callback(..., "fix")

% package B
add_to_callback(..., "fix")

% package B
remove_callback(..., "fix")

% package A
remove_callback(..., "fix")

Here package B removes the callback installed by package A, and vice versa. This is because they use a clashing name, and first inserted entry is always removed first. This is wrong, but that's what we have been doing and what LaTeX does.

Your suggestion of silently not registering duplicates would also require silently allowing more removals than additions (as here, only package A's callback would be installed, and removed by B, A would have nothing to remove).

I.e. my first idea was to loudly prevent any subtle issues. Now I suggest to go back to the previous bad behavior, just as a pragmatic choice. I don't think adding a different bad behavior makes a big change.

I don't like silently dropping duplicate entries. If the user doesn't see an error, they expect a success. The obvious success state (that the callback was registered) didn't happen, and the user doesn't have a chance to know that something went wrong.

If we allow duplicates, like previously, we succesfully add the callback - no silent ignoring. What will be problematic is removal, if any other order than FIFO is expected. But I am hoping that by doing what LaTeX does, we can at least stay somewhat compatible, and less surprising for users.

[olsak]

OK, I keep the Lua error when registering the same callback twice and do checking at macro level.