Re: scheme.el bug & fix

Re: scheme.el bug & fix

[Stefan Monnier]

> The problem is that `scheme-mode-syntax-table' is not initialized
> with the syntax-table built in the let form, but with a standard
> syntax table.

It turns out the problem is that

  (with-syntax-table st (modify-syntex-entry foo bar))

does not modify `st' because `with-syntax-table' does not use `st'
but a copy of it.  Actually it's even documented in the docstring.
This sounds silly.  Does anybody have an idea why it is defined that way ?
If not, any objection the patch below which should also improve
(very marginally) the performance of Emacs ?


	Stefan


--- subr.el	4 Feb 2003 12:06:43 -0000	1.340
+++ subr.el	16 Feb 2003 23:52:07 -0000
@@ -1709,7 +1743,7 @@
     parent))
 
 (defmacro with-syntax-table (table &rest body)
-  "Evaluate BODY with syntax table of current buffer set to a copy of TABLE.
+  "Evaluate BODY with syntax table of current buffer set to TABLE.
 The syntax table of the current buffer is saved, BODY is evaluated, and the
 saved table is restored, even in case of an abnormal exit.
 Value is what BODY returns."
@@ -1719,7 +1753,7 @@
 	   (,old-buffer (current-buffer)))
        (unwind-protect
 	   (progn
-	     (set-syntax-table (copy-syntax-table ,table))
+	     (set-syntax-table ,table)
 	     ,@body)
 	 (save-current-buffer
 	   (set-buffer ,old-buffer)

Re: scheme.el bug & fix

[Luc Teirlinck]

Stefan Monnier wrote:

   It turns out the problem is that

     (with-syntax-table st (modify-syntex-entry foo bar))

   does not modify `st' because `with-syntax-table' does not use `st'
   but a copy of it.  Actually it's even documented in the docstring.
   This sounds silly.  Does anybody have an idea why it is defined that way ?
   If not, any objection the patch below which should also improve
   (very marginally) the performance of Emacs ?

Are you sure that you are not going to break plenty of existing code
this way?  People could very legitimately have relied on the present
behavior, which is, as you state yourself, very explicitly mentioned
in the doc string.  The reason for the "silly" behavior seems obvious:
to avoid accidental modification of st.

Is there any reason why scheme.el can not possibly define its syntax
table in a more standard and more natural way?  I do not believe that 
with-syntax-table was meant to be used the way scheme .el uses it.
(I believe it is meant to very temporarily change the syntax table of
the current buffer, not to be used to actually define syntax tables.)


Sincerely,

Luc.

Re: scheme.el bug & fix

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Stefan Monnier wrote:
> 
>    It turns out the problem is that
> 
>      (with-syntax-table st (modify-syntex-entry foo bar))
> 
>    does not modify `st' because `with-syntax-table' does not use `st'
>    but a copy of it.  Actually it's even documented in the docstring.
>    This sounds silly.  Does anybody have an idea why it is defined that way ?
>    If not, any objection the patch below which should also improve
>    (very marginally) the performance of Emacs ?
> 
> Are you sure that you are not going to break plenty of existing code
> this way?

I just checked all 28 occurrences of with-syntax-table and only 
the following would break:

autoconf.el: autoconf-current-defun-function

All other occurrences don't modify the syntax table *), so the copy
operation is indeed wasteful in the normal case.  antlr-mode.el and
cc-defs.el even define their own versions of the macro, and they don't
copy the syntax table either.

*) except for scheme.el which expects to be able to modify the syntax
   table, so it would be fixed rather than broken by the change...

Maybe we should make a new macro with-copy-of-syntax-table which does
copy the table, and change the original macro not to copy.

> Is there any reason why scheme.el can not possibly define its syntax
> table in a more standard and more natural way?  I do not believe that 
> with-syntax-table was meant to be used the way scheme .el uses it.

But if you e.g. use with-current-buffer, you are working on that
buffer, not a copy of it...   So I find the "copy of" behaviour
quite surprising.

> (I believe it is meant to very temporarily change the syntax table of
> the current buffer, not to be used to actually define syntax tables.)

But it is primarily used to temporarily *use* an existing syntax table, 
not temporarily *modify* a syntax table.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: scheme.el bug & fix

[Miles Bader]

storm@cua.dk (Kim F. Storm) writes:
> Maybe we should make a new macro with-copy-of-syntax-table which does
> copy the table, and change the original macro not to copy.

I think we should change the original macro not to copy, and indeed I
think that's certainly what most programmers would expect it to do.

Places that _want_ a copy could just use something like

  (with-syntax-table (copy-syntax-table ...) ...)

Which makes what's happening nice and explicit.

-Miles
-- 
Ich bin ein Virus. Mach' mit und kopiere mich in Deine .signature.

Re: scheme.el bug & fix

[Richard Stallman]

    Maybe we should make a new macro with-copy-of-syntax-table which does
    copy the table, and change the original macro not to copy.

That macro is not useful enough to be worth adding.
If we change with-syntax-table, people who want a copy
can write explicit code to do the copy--it is easy enough.

In general we don't want to add a function that is a simple
combination of existing functions unless it would be used very often.

Re: scheme.el bug & fix

[Stefan Monnier]

>    It turns out the problem is that
> 
>      (with-syntax-table st (modify-syntex-entry foo bar))
> 
>    does not modify `st' because `with-syntax-table' does not use `st'
>    but a copy of it.  Actually it's even documented in the docstring.
>    This sounds silly.  Does anybody have an idea why it is defined that way ?
>    If not, any objection the patch below which should also improve
>    (very marginally) the performance of Emacs ?
> 
> Are you sure that you are not going to break plenty of existing code
> this way?

I grepped through lisp/**/*.el and couldn't find a single case where
the table needed to be copied.

> People could very legitimately have relied on the present
> behavior, which is, as you state yourself, very explicitly mentioned
> in the doc string.  The reason for the "silly" behavior seems obvious:
> to avoid accidental modification of st.

But it is extremely rare to use modify-syntax-entry with no
third argument and within a with-syntax-table thing, so the
risk of accidental modification is basically inexistent.
As a matter of fact, syntax-tables are very rarely modified at
all, other than when they're created.

> Is there any reason why scheme.el can not possibly define its syntax
> table in a more standard and more natural way?

Of course, it can.  That's a separate question.  Please someone install
the patch as soon as you can: it's obviously the right thing to do.

> (I believe it is meant to very temporarily change the syntax table of
> the current buffer, not to be used to actually define syntax tables.)

Indeed, and you don't want this temporary syntax-table-switch to make
a copy of the table and then drop it on the floor (turning it into
garbage) unless there's a very good reason for it.
For example with-category-table does not copy the table.


	Stefan

Re: scheme.el bug & fix

[Luc Teirlinck]

Stefan Monnier wrote:

   I grepped through lisp/**/*.el and couldn't find a single case where
   the table needed to be copied.

According to Kim the following would break (and hence would need to be
fixed if we make the change):
autoconf.el: autoconf-current-defun-function

   > (I believe it is meant to very temporarily change the syntax table of
   > the current buffer, not to be used to actually define syntax tables.)

   Indeed, and you don't want this temporary syntax-table-switch to make
   a copy of the table and then drop it on the floor (turning it into
   garbage) unless there's a very good reason for it.

I now agree that it would be (or at the very least would have been)
better not to copy.  However, the change still seems to involve some
risks, and if we change the macro, we will need to be careful and
alert to the potential problems.

   As a matter of fact, syntax-tables are very rarely modified at
   all, other than when they're created.

Yes, but the point is that, in its current form, with-syntax-table
uses a newly created (copied) syntax-table.  It is very customary to
create a new syntax-table by first copying an old one and then making
a few changes.  The current version of with-syntax-table (and its
documentation string) clearly seems to allow (in fact, nearly
encourage) such use.  One could argue that this may have been a
mistake, but it is still the current behavior.

The type of bugs we are talking about can be extremely nasty.  An
obscure, seldom-but-not-never called function modifies, say,
standard-syntax-table.  All of a sudden, one of the user's abbrevs
does not expand.  After carefully double checking his abbrev-tables,
the user decides to file a bug report.  For safety, first try 
emacs -q.  Bug gone.  Check .emacs.  Starting a new emacs makes the
problem disappear.  The user must have hit some "wrong key" or
something.  No bug report.  A week or so later another strange
completely unrelated problem appears.  Same story.  Hit some wrong key
again, no bug report.  These are the kind of bugs where it will be
even hard for the user to file anything close to a useful bug report.

If we make the change, we should realize that some strange
irreproducible bug reports we might get could be due to people using
specialized Elisp programs, not included with the Emacs distribution
(there actually are quite a few of those), that rely on the old
with-syntax-table behavior and hence, with the new behavior, modify,
say, standard-syntax-table.  One will also have to be careful with
packages written by people who themselves use a released version
instead of the latest CVS.  Gerd does not seem to remember the
details, but if the reason would have been compatibility with XEmacs,
one will also have to be careful with packages originally written for
XEmacs.  (I myself am not familiar with XEmacs, so I do not know what
the situation is.)

If one makes the change, it should be emphasized in the News (C-h n).
The new behavior and the danger involved should be clearly mentioned
in the documentation string and the Elisp manual.  If we are going to
make the change, it may actually be best to include it in a released
version sooner rather than later, to minimize the time during which
people get accustomed to and use the old version.  (Fortunately, it is
a relatively new macro.)

Sincerely,

Luc.

Re: scheme.el bug & fix

[Stefan Monnier]

> Yes, but the point is that, in its current form, with-syntax-table
> uses a newly created (copied) syntax-table.  It is very customary to
> create a new syntax-table by first copying an old one and then making
> a few changes.  The current version of with-syntax-table (and its
> documentation string) clearly seems to allow (in fact, nearly
> encourage) such use.  One could argue that this may have been a
> mistake, but it is still the current behavior.

Note that with-syntax-table was introduced in Emacs-21, so it's
rarely used in external packages (most packages try to maintain
compatibility with Emacs-20, and most package writers learn
new macros rather slowly).


	Stefan

Re: scheme.el bug & fix

[Richard Stallman]

    Note that with-syntax-table was introduced in Emacs-21, so it's
    rarely used in external packages (most packages try to maintain
    compatibility with Emacs-20, and most package writers learn
    new macros rather slowly).

Gerd said there may have been an XEmacs compatibility issue.
Perhaps people have written code for XEmacs which assumes
with-syntax-table copies the syntax table.

But I suspect that the amount of such code is small, and that this
change is ok.

Re: scheme.el bug & fix

[Stefan Monnier]

>     Note that with-syntax-table was introduced in Emacs-21, so it's
>     rarely used in external packages (most packages try to maintain
>     compatibility with Emacs-20, and most package writers learn
>     new macros rather slowly).
> 
> Gerd said there may have been an XEmacs compatibility issue.
> Perhaps people have written code for XEmacs which assumes
> with-syntax-table copies the syntax table.

XEmacs indeed does the copying, but its docstring does not document
that behavior.

> But I suspect that the amount of such code is small, and that this
> change is ok.

Indeed, within XEmacs-21.1 (an old version, admittedly), it's only used
in help.el and (in the unbundled XEmacs packages) in semantic.el.
In both cases, it does not need copying.


	Stefan

Re: scheme.el bug & fix

[Miles Bader]

On Wed, Feb 19, 2003 at 09:31:40AM -0500, Stefan Monnier wrote:
> XEmacs indeed does the copying, but its docstring does not document
> that behavior.
...
> Indeed, within XEmacs-21.1 (an old version, admittedly), it's only used
> in help.el and (in the unbundled XEmacs packages) in semantic.el.
> In both cases, it does not need copying.

If someone does make this change, perhaps they could send the xemacs
maintainers mail mentioning the change; they might want to do the same.

-Miles
-- 
`Cars give people wonderful freedom and increase their opportunities.
 But they also destroy the environment, to an extent so drastic that
 they kill all social life' (from _A Pattern Language_)

Re: scheme.el bug & fix

[Richard Stallman]

Ok, let's change with-syntax-table not to copy it.

    If someone does make this change, perhaps they could send the xemacs
    maintainers mail mentioning the change; they might want to do the same.

I agree with that.

Re: scheme.el bug & fix

[Stefan Monnier]

> Ok, let's change with-syntax-table not to copy it.

Done.

>     If someone does make this change, perhaps they could send the xemacs
>     maintainers mail mentioning the change; they might want to do the same.
> 
> I agree with that.

Done.


	Stefan

Re: scheme.el bug & fix

[Luc Teirlinck]

I forgot the CC to emacs-devel in my previous messsage:

Stefan Monnier wrote (months ago):

   > Ok, let's change with-syntax-table not to copy it.

   Done.

With "it" is of course meant the temporary syntax table.

There is (apparently) no mention of this change in NEWS yet.  Unless
it is mentioned, people are going to produce some very nasty
(extremely difficult to debug) bugs for which they would carry no
blame whatsoever.

Sincerely,

Luc.