bug#53260: char-syntax differs in interpreter and bytecode

bug#53260: char-syntax differs in interpreter and bytecode

[Mattias Engdegård]

Fchar_syntax and the bytecode Bchar_syntax differ:

Fchar_syntax calls SETUP_BUFFER_SYNTAX_TABLE. Bchar_syntax does not.
Bchar_syntax converts arguments to multibyte. Fchar_syntax does not.

The last property can be used to get different behaviour:

(let ((cs (byte-compile (lambda (x) (char-syntax x)))))
  (with-temp-buffer
    (let ((st (make-syntax-table)))
      (set-buffer-multibyte nil)
      (modify-syntax-entry 128 "_" st)
      (set-syntax-table st)
      (list (funcall cs 128) (char-syntax 128)))))
-> (119 95)

Not sure how to expose the presence or absence of SETUP_BUFFER_SYNTAX_TABLE. Suggestions?

And, most importantly, what would be the correct code?

(I suppose char-syntax is rare enough that we could call Fchar_syntax from Bchar_syntax and thus avoid any future divergence.)

bug#53260: char-syntax differs in interpreter and bytecode

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Fchar_syntax and the bytecode Bchar_syntax differ:
>
> Fchar_syntax calls SETUP_BUFFER_SYNTAX_TABLE. Bchar_syntax does not.
> Bchar_syntax converts arguments to multibyte. Fchar_syntax does not.

[...]

> And, most importantly, what would be the correct code?

Hm.  Perhaps Stefan has an opinion; added to the CCs.

> (I suppose char-syntax is rare enough that we could call Fchar_syntax
> from Bchar_syntax and thus avoid any future divergence.)

Used 172 times in-core, which isn't that rare...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53260: char-syntax differs in interpreter and bytecode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2022-01-15 09:36:16] wrote:
> Mattias Engdegård <mattiase@acm.org> writes:
>> Fchar_syntax and the bytecode Bchar_syntax differ:
>> Fchar_syntax calls SETUP_BUFFER_SYNTAX_TABLE. Bchar_syntax does not.
>> Bchar_syntax converts arguments to multibyte.  Fchar_syntax does not.
> [...]
>> And, most importantly, what would be the correct code?
> Hm.  Perhaps Stefan has an opinion; added to the CCs.

My past opinion is in its docstring:

    If you’re trying to determine the syntax of characters in the buffer,
    this is probably the wrong function to use, because it can’t take
    ‘syntax-table’ text properties into account.  Consider using
    ‘syntax-after’ instead.

The "can't" is because `char-syntax` doesn't know where the char comes from.

>> (I suppose char-syntax is rare enough that we could call Fchar_syntax
>> from Bchar_syntax and thus avoid any future divergence.)
> Used 172 times in-core, which isn't that rare...

I think he meant "rare" w.r.t dynamic count rather than static count.


        Stefan

bug#53260: char-syntax differs in interpreter and bytecode

[Mattias Engdegård]

15 jan. 2022 kl. 15.46 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>    If you’re trying to determine the syntax of characters in the buffer,
>    this is probably the wrong function to use, because it can’t take
>    ‘syntax-table’ text properties into account.  Consider using
>    ‘syntax-after’ instead.
> 
> The "can't" is because `char-syntax` doesn't know where the char comes from.

This is true and it leaves a narrower use for `char-syntax` in mode-specific code -- ie, when syntax-table text properties do not need to be taken into account.

I propose we do the following:

1. Remove SETUP_BUFFER_SYNTAX_TABLE() from Fchar_syntax because as far as I can tell it has no effect at all.

2. Remove make_char_multibyte(c) from Bchar_syntax because it seems to be the wrong thing to do: in a unibyte buffer, wouldn't the syntax table be indexed by byte value (so that char 255 in the buffer corresponds to entry 255 in the syntax table rather than entry 0x3fffff)?

3. Now both implementations are identical. Replace the one in the byte-code interpreter with a call to Fchar_syntax.

> I think he meant "rare" w.r.t dynamic count rather than static count.

Yes, that's right.

bug#53260: char-syntax differs in interpreter and bytecode

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 15 Jan 2022 18:29:41 +0100
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 53260@debbugs.gnu.org
> 
> 2. Remove make_char_multibyte(c) from Bchar_syntax because it seems to be the wrong thing to do: in a unibyte buffer, wouldn't the syntax table be indexed by byte value (so that char 255 in the buffer corresponds to entry 255 in the syntax table rather than entry 0x3fffff)?

I don't think we want to support unibyte buffers which have some text
that is syntactically significant.  A unibyte buffer is just a stream
of raw bytes, they are not characters.

bug#53260: char-syntax differs in interpreter and bytecode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 1. Remove SETUP_BUFFER_SYNTAX_TABLE() from Fchar_syntax because as far as
> I can tell it has no effect at all.

Sounds good.

> 2. Remove make_char_multibyte(c) from Bchar_syntax because it seems to be
> the wrong thing to do: in a unibyte buffer, wouldn't the syntax table be
> indexed by byte value (so that char 255 in the buffer corresponds to entry
> 255 in the syntax table rather than entry 0x3fffff)?

Doesn't sound right: char tables are indexed by chars (i.e. Unicode code
points) not by bytes, so we need to convert the byte into a char
before indexing.

> 3. Now both implementations are identical. Replace the one in the byte-code
> interpreter with a call to Fchar_syntax.

Sounds good.


        Stefan

bug#53260: char-syntax differs in interpreter and bytecode [PATCH]

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

15 jan. 2022 kl. 23.51 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Doesn't sound right: char tables are indexed by chars (i.e. Unicode code
> points) not by bytes, so we need to convert the byte into a char
> before indexing.

Sure, I'm happy to do it either way. Chars retrieved from unibyte buffers or strings really should be converted to multibyte before used with char-syntax; unibyte buffers are not very common but strings slightly more so.


[-- Attachment #2: 0001-Fix-Fchar_syntax-for-non-ASCII-in-unibyte-buffers.patch --]
[-- Type: application/octet-stream, Size: 3163 bytes --]

From 2adb5c862232abf126f73d0aa514f5ae8b6babba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 16 Jan 2022 11:58:00 +0100
Subject: [PATCH] Fix Fchar_syntax for non-ASCII in unibyte buffers

Fchar_syntax did not convert unibyte characters to multibyte when the
current buffer was unibyte, in contrast to `char-syntax` in
byte-compiled code (bug#53260).

* src/bytecode.c (exec_byte_code): Call out to Fchar_syntax;
the dynamic frequency is too low to justify inlining here, and it
did lead to implementations diverging.
* src/syntax.c (Fchar_syntax): Convert non-ASCII unibyte values to
multibyte.  Remove useless SETUP_BUFFER_SYNTAX_TABLE which has no
effect here.
* test/src/syntax-tests.el (syntax-char-syntax): New test.
---
 src/bytecode.c           |  8 +-------
 src/syntax.c             |  6 +++---
 test/src/syntax-tests.el | 15 +++++++++++++++
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/bytecode.c b/src/bytecode.c
index 472992be18..b7e65d05ae 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -1167,13 +1167,7 @@ #define DEFINE(name, value) LABEL (name) ,
 	  NEXT;
 
 	CASE (Bchar_syntax):
-	  {
-	    CHECK_CHARACTER (TOP);
-	    int c = XFIXNAT (TOP);
-	    if (NILP (BVAR (current_buffer, enable_multibyte_characters)))
-	      c = make_char_multibyte (c);
-	    XSETFASTINT (TOP, syntax_code_spec[SYNTAX (c)]);
-	  }
+	  TOP = Fchar_syntax (TOP);
 	  NEXT;
 
 	CASE (Bbuffer_substring):
diff --git a/src/syntax.c b/src/syntax.c
index 9df878b8ed..c1e81dfa47 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -1101,10 +1101,10 @@ DEFUN ("char-syntax", Fchar_syntax, Schar_syntax, 1, 1, 0,
 `syntax-after' instead.  */)
   (Lisp_Object character)
 {
-  int char_int;
   CHECK_CHARACTER (character);
-  char_int = XFIXNUM (character);
-  SETUP_BUFFER_SYNTAX_TABLE ();
+  int char_int = XFIXNAT (character);
+  if (NILP (BVAR (current_buffer, enable_multibyte_characters)))
+    char_int = make_char_multibyte (char_int);
   return make_fixnum (syntax_code_spec[SYNTAX (char_int)]);
 }
 
diff --git a/test/src/syntax-tests.el b/test/src/syntax-tests.el
index 3b9f21cde3..501b5e067f 100644
--- a/test/src/syntax-tests.el
+++ b/test/src/syntax-tests.el
@@ -506,4 +506,19 @@ test-from-to-parse-partial-sexp
     (should (parse-partial-sexp 1 1))
     (should-error (parse-partial-sexp 2 1))))
 
+(ert-deftest syntax-char-syntax ()
+  ;; Verify that char-syntax behaves identically in interpreted and
+  ;; byte-compiled code (bug#53260).
+  (let ((cs (byte-compile (lambda (x) (char-syntax x)))))
+    ;; Use a unibyte buffer with a syntax table using symbol syntax
+    ;; for raw byte 128.
+    (with-temp-buffer
+      (set-buffer-multibyte nil)
+      (let ((st (make-syntax-table)))
+        (modify-syntax-entry (unibyte-char-to-multibyte 128) "_" st)
+        (set-syntax-table st)
+        (should (equal (char-syntax 128) ?_))
+        (should (equal (funcall cs 128) ?_))))
+    (list (char-syntax 128) (funcall cs 128))))
+
 ;;; syntax-tests.el ends here
-- 
2.32.0 (Apple Git-132)

bug#53260: char-syntax differs in interpreter and bytecode [PATCH]

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Sure, I'm happy to do it either way. Chars retrieved from unibyte
> buffers or strings really should be converted to multibyte before used
> with char-syntax; unibyte buffers are not very common but strings
> slightly more so.

Makes sense to me.  Unless Stefan has any further comments, please go
ahead and push.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53260: char-syntax differs in interpreter and bytecode [PATCH]

[Mattias Engdegård]

20 jan. 2022 kl. 10.30 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Makes sense to me.  Unless Stefan has any further comments, please go
> ahead and push.

Thank you, pushed with a necessary modification: SETUP_BUFFER_SYNTAX_TABLE() is indeed necessary in Fchar_syntax because syntax.c has its own local #define SYNTAX() and doesn't use the one in syntax.h. Lovely.