bug#56682: locked narrowing

[Stefan Monnier]

Gregory Heytings [2022-08-16 19:39:21] wrote:
>> [ Sorry, Gregory.  But see below for more concrete/constructive
>>   criticism. ]
> Perhaps I misuderstood or overlooked something in what you said
> earlier, but to me your objection to that part of the code until now
> was mainly/purely "philosophical".

My *main* objection is in the not-overridable part of the design, yes
(i.e. philosophical).  The rest is just a minor question of coding.

>> - The locking is global: any `widen` or `narrow-to-region` will fail when
>> a locked narrowing is in effect, even when it's applied to an unrelated
>> buffer.  Again, it looks like a plain bug, but...
> It's not global, given that it is controlled by symbol that is bound within
> a function called inside a specific buffer.

Which buffer is current when the let-binding is installed has no
influence on the behavior because the variable is not buffer-local
(i.e. it's global, which is why the effect is global).
The dynamic scope means that the effect is limited *in time* (and only
applies to the current thread, admittedly), but it's still global in the
sense that it affects all the code that is run during this time, no
matter where it is executed (or were it was written).

But, yes, dynamically scoped variables are often described as being
"locally bound", even though the effect of that binding is global in the
sense that it applies everywhere while active, so there's plenty of
opportunities for confusion with this terminology :-(

> But indeed, if inside that
> function we use narrow-to-region/widen in another buffer, that would fail.
> It seems easy to correct that bug, though: if
> (current_buffer->long_line_optimizations_p && ! NILP (Vrestrictions_locked))

If `current_buffer` is not the one that's currently being narrowed by
redisplay, then `->long_line_optimizations_p` doesn't say that
`current_buffer` has been narrowed by "us", so such a fix might work but
I think a better fix is to `Fmake_local_variable` before calling
`specbind` so that it really states clearly that this lock applies only
to the narrowing installed in this particular buffer.

>> - The doc for `restrictions-locked` says:
>>
>>  This happens when `narrow-to-region', which see, is called from Lisp
>>  with an optional argument LOCK non-nil.
>>
>>  but the `narrow-to-region` function doesn't have any such LOCK argument.
>
> That should have been removed, indeed.  In a previous iteration of the code
> a LOCK argument was added to that function, and later removed.

Adding such a LOCK argument might be a good idea, actually, since ELisp
packages may also want to impose restrictions on widening (see my recent
message to `emacs-devel` about ELisp support for locked narrowing).

>> - The `narrow_to_region_internal` function, when called with `lock=true`
>> oddly includes the functionality of save-restriction to restore the
>> restriction and point but not to restore the `restrictions-locked` state.
>
> This I do not understand.  The comment above that function explains that it
> should be followed by "specbind (Qrestrictions_locked, Qt)".  There is no
> need to restore the restrictions-locked state, or am
> I misunderstanding something?

Why leave the `specbind (Qrestrictions_locked, Qt)` out of the function
when you've already included the 3 calls to `record_unwind_protect`?
The 4 share the same requirements (because `specbind` pushes on the
`specpdl`, just like `record_unwind_protect`).  They naturally
belong together.

Also, a function that sometimes pushes to `specpdl` (and hence requires
the caller to do the `SPECPDL_INDEX + unbind_to` dance) and sometimes
not, depending on some value of its argument is a bit unnatural (it
complicates the reasoning when we try and convince ourselves that the
stack discipline of the specpdl is obeyed).  So I'd recommend you split
the function into two: a first one that only does the narrowing itself
and a second one which calls the first plus unconditionally does the
`specbind+record_unwind_protect`.

>> - In that same function the two branches of the `if (lock)` have a lot of
>> obvious redundancy.
> Not "a lot", but indeed there's some redundancy there that could have
> been avoided.

AFAICT it's more than half of the code of each branch.


        Stefan