bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

I think the following should be harmless. Why does it result in broken
indentation?

1. Open in c++-mode buffer with following contents:

int main(int argc, char** argv) {
  fgets(0, 1, 2);
}

2. Add an empty line after the semicolon, move cursor there, and
evaluate this:

(with-syntax-table (make-char-table 'syntax-table nil)
  (modify-syntax-entry ?\( "(")
  (modify-syntax-entry ?\) ")")
  (modify-syntax-entry ?< "(")
  (modify-syntax-entry ?> ")")
  (insert "fgets(0, 1, 2)"))

3. Press `;', see the indentation of the current line change, and
include (erroneously) two extra spaces:

int main(int argc, char** argv) {
  fgets(0, 1, 2);
    fgets(0, 1, 2);
}

Originally: https://github.com/company-mode/company-mode/issues/212

In GNU Emacs 24.3.94.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2014-10-06 on axl
Repository revision: 117555 sdl.web@gmail.com-20141005005838-oyl694hqhu2d3632
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04.1 LTS

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Alan Mackenzie]

Hello, Dmitry.

In article <mailman.12024.1414247237.1147.bug-gnu-emacs@gnu.org> you wrote:
> I think the following should be harmless. Why does it result in broken
> indentation?

Why do you think it should be harmless?

> 1. Open in c++-mode buffer with following contents:

> int main(int argc, char** argv) {
>  fgets(0, 1, 2);
> }

> 2. Add an empty line after the semicolon, move cursor there, and
> evaluate this:

> (with-syntax-table (make-char-table 'syntax-table nil)
>  (modify-syntax-entry ?\( "(")
>  (modify-syntax-entry ?\) ")")
>  (modify-syntax-entry ?< "(")
>  (modify-syntax-entry ?> ")")
>  (insert "fgets(0, 1, 2)"))

> 3. Press `;', see the indentation of the current line change, and
> include (erroneously) two extra spaces:

> int main(int argc, char** argv) {
>  fgets(0, 1, 2);
>    fgets(0, 1, 2);
> }

If you go to the semicolon at the end of line 2 and do C-u C-x = you'll
see that that character has these text-properties:
  c-in-sws             t
  c-is-sws             t
  fontified            t
.  The c-in-sws and c-is-sws indicate that the semicolon has been
recognised and marked as syntactic whitespace.  The second line thus gets
parsed as "statement-cont", i.e. a continued statement, so it gets
indented an extra level.

If you cripple C++ Mode by substituting a wrong syntax table, you
shouldn't be too surprised when things go "wrong".  This seems like one of
these "well, don't do that, then" bugs.

Question: why do you want to play around with the syntax table in this
manner?  What are you trying to achieve?

> Originally: https://github.com/company-mode/company-mode/issues/212

> In GNU Emacs 24.3.94.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
> of 2014-10-06 on axl
> Repository revision: 117555 sdl.web@gmail.com-20141005005838-oyl694hqhu2d3632
> Windowing system distributor `The X.Org Foundation', version 11.0.11501000
> System Description:     Ubuntu 14.04.1 LTS

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/26/2014 02:34 AM, Alan Mackenzie wrote:

> Why do you think it should be harmless?

Because the syntax table change is temporary and its effect should be 
limited to my code?

> .  The c-in-sws and c-is-sws indicate that the semicolon has been
> recognised and marked as syntactic whitespace.  The second line thus gets
> parsed as "statement-cont", i.e. a continued statement, so it gets
> indented an extra level.

Yes, I see that. But how does this happen?

> If you cripple C++ Mode by substituting a wrong syntax table, you
> shouldn't be too surprised when things go "wrong".  This seems like one of
> these "well, don't do that, then" bugs.
>
> Question: why do you want to play around with the syntax table in this
> manner?  What are you trying to achieve?

I'm using a different syntax table for sexp movement, where it's 
necessary for both parens and angle brackets to have paren syntax class. 
In the actual code I modify the text after it's inserted, and 
`backward-sexp' is used two times to find necessary search bounds.

https://github.com/company-mode/company-mode/blob/42012730da15ffaef7c61722475040babed15332/company-template.el#L155-L173

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Alan Mackenzie]

Hello, Dmitry.

In article <mailman.12048.1414277182.1147.bug-gnu-emacs@gnu.org> you wrote:
> On 10/26/2014 02:34 AM, Alan Mackenzie wrote:

>> Why do you think it should be harmless?

> Because the syntax table change is temporary and its effect should be 
> limited to my code?

It's no so limited.  The before-change-functions and after-change-functions
hooks will be run with that syntax table active.  This is not good.

>> .  The c-in-sws and c-is-sws indicate that the semicolon has been
>> recognised and marked as syntactic whitespace.  The second line thus gets
>> parsed as "statement-cont", i.e. a continued statement, so it gets
>> indented an extra level.

> Yes, I see that. But how does this happen?

Somewhere in a before- or after-change-functions, c-backward-sws is being
called, to go backward over syntactic whitespace.  Somehow it gets
horribly confused, because the syntax table isn't correct.  I don't think
the exact details matter too much here.

>> If you cripple C++ Mode by substituting a wrong syntax table, you
>> shouldn't be too surprised when things go "wrong".  This seems like one of
>> these "well, don't do that, then" bugs.

>> Question: why do you want to play around with the syntax table in this
>> manner?  What are you trying to achieve?

> I'm using a different syntax table for sexp movement, where it's 
> necessary for both parens and angle brackets to have paren syntax class. 
> In the actual code I modify the text after it's inserted, and 
> `backward-sexp' is used two times to find necessary search bounds.

OK.  Can I suggest an alternative?  In C++ (and Java) Modes, the template
(generic) delimiters are marked with syntax-table text properties.
Unfortunately, at the moment this is done as part of font-locking, so
isn't done until you display.  However, if you put "(sit-for 0)" into
your code after inserting "< ... >", this will cause a redisplay,
allowing subsequent code to use the text properties, and a backward-sexp
will then work.

In the medium future (several weeks away), I'm hoping to fix CC Mode so
that the text properties are applied to < ... > on an after-change
function rather than at redisplay.

> https://github.com/company-mode/company-mode/blob/42012730da15ffaef7c61722475040babed15332/company-template.el#L155-L173

-- Alan Mackenzie (Nuremberg, Germany).

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Stefan Monnier]

> (with-syntax-table (make-char-table 'syntax-table nil)
>   (modify-syntax-entry ?\( "(")
>   (modify-syntax-entry ?\) ")")
>   (modify-syntax-entry ?< "(")
>   (modify-syntax-entry ?> ")")
>   (insert "fgets(0, 1, 2)"))

Why do that?


        Stefan

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/26/2014 06:25 AM, Stefan Monnier wrote:
>> (with-syntax-table (make-char-table 'syntax-table nil)
>>    (modify-syntax-entry ?\( "(")
>>    (modify-syntax-entry ?\) ")")
>>    (modify-syntax-entry ?< "(")
>>    (modify-syntax-entry ?> ")")
>>    (insert "fgets(0, 1, 2)"))
>
> Why do that?

To include angle brackets in paren syntax class. Basically, we only need 
to consider these and the straight parens when converting a method 
definition into a method call with placeholders, so using an 
otherwise-empty syntax table seemed appropriate when I wrote the above code.

It seems that I should only change the current syntax table only around 
the `backward-sexp' and `parse-partial-sexp' calls there, but not when 
doing any text modification.

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/26/2014 06:25 AM, Stefan Monnier wrote:
>> (with-syntax-table (make-char-table 'syntax-table nil)
>>    (modify-syntax-entry ?\( "(")
>>    (modify-syntax-entry ?\) ")")
>>    (modify-syntax-entry ?< "(")
>>    (modify-syntax-entry ?> ")")
>>    (insert "fgets(0, 1, 2)"))
>
> Why do that?

To include angle brackets in paren syntax class. Basically, we only need 
to consider these and the straight parens when converting a method 
definition into a method call with placeholders, so using an 
otherwise-empty syntax table seemed appropriate when I wrote the above code.

It seems that I should only change the current syntax table only around 
the `backward-sexp' and `parse-partial-sexp' calls there, but not when 
doing any text modification.

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/26/2014 06:24 AM, Alan Mackenzie wrote:

>> Because the syntax table change is temporary and its effect should be
>> limited to my code?
>
> It's no so limited.  The before-change-functions and after-change-functions
> hooks will be run with that syntax table active.  This is not good.

I see, thanks. Somehow, I figured that those hooks would run before or 
after the current command, whereas they're triggered right near any code 
that makes changes to the buffer. Question answered, closing.

On the other hand, you're doing some pretty unusual things with this. If 
`c-in-sws' and `c-is-sws' were added in syntax-propertize-function 
(which is called on-demand), this conflict wouldn't have manifested.

> OK.  Can I suggest an alternative?  In C++ (and Java) Modes, the template
> (generic) delimiters are marked with syntax-table text properties.
> Unfortunately, at the moment this is done as part of font-locking, so
> isn't done until you display.  However, if you put "(sit-for 0)" into
> your code after inserting "< ... >", this will cause a redisplay,
> allowing subsequent code to use the text properties, and a backward-sexp
> will then work.

Sounds like it should be performed during `syntax-propertize' instead. 
Even if you don't like this idea now, I suspect it'll happen eventually 
anyway, if only for consistency with other major modes.

In that sense, the `sit-for' suggestion is not future-proof. So I'll try 
only changing the syntax table around specific functions that don't 
modify the buffer text, but just move point, since that was the actual goal.

> In the medium future (several weeks away), I'm hoping to fix CC Mode so
> that the text properties are applied to < ... > on an after-change
> function rather than at redisplay.

Hmm. My current workaround is to use a temporary syntax table that 
inherits from c++-mode-syntax-table (but has angle brackets in the 
'paren' class). Sounds like it might also create a conflict, if maybe a 
more subtle one.

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Stefan Monnier]

>>> (with-syntax-table (make-char-table 'syntax-table nil)
>>> (modify-syntax-entry ?\( "(")
>>> (modify-syntax-entry ?\) ")")
>>> (modify-syntax-entry ?< "(")
>>> (modify-syntax-entry ?> ")")
>>> (insert "fgets(0, 1, 2)"))
>> 
>> Why do that?

> To include angle brackets in paren syntax class.

But `insert' doesn't use that temporary syntax-table (aside from
indirect use in before/after-change-functions, obviously).  So why use
`with-syntax-table' around a call to `insert'?

> It seems that I should only change the current syntax table only
> around the `backward-sexp' and `parse-partial-sexp' calls there, but
> not when doing any text modification.

Yes.  And even then, this should not be necessary since the major mode
already does it (or should do it) for you.


        Stefan

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Stefan Monnier]

> On the other hand, you're doing some pretty unusual things with this.
> If `c-in-sws' and `c-is-sws' were added in syntax-propertize-function
> (which is called on-demand), this conflict wouldn't have manifested.

;-)

> In that sense, the `sit-for' suggestion is not future-proof.  So I'll try
> only changing the syntax table around specific functions that don't modify
> the buffer text, but just move point, since that was the actual goal.

Rather than using a hack like `sit-for', and while waiting for cc-mode
to use syntax-propertize for that, you can use `font-lock-ensure' to
make sure that the corresponding part of the code has been font-locked.


        Stefan

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Alan Mackenzie]

Hi, Dmitry.

In article <mailman.12080.1414336227.1147.bug-gnu-emacs@gnu.org> you wrote:
> On 10/26/2014 06:24 AM, Alan Mackenzie wrote:

>>> Because the syntax table change is temporary and its effect should be
>>> limited to my code?

>> It's no so limited.  The before-change-functions and after-change-functions
>> hooks will be run with that syntax table active.  This is not good.

> I see, thanks. Somehow, I figured that those hooks would run before or 
> after the current command, whereas they're triggered right near any code 
> that makes changes to the buffer. Question answered, closing.

> On the other hand, you're doing some pretty unusual things with this. If 
> `c-in-sws' and `c-is-sws' were added in syntax-propertize-function 
> (which is called on-demand), this conflict wouldn't have manifested.

They're not syntax-table text properties, so the setting of them doesn't
belong in syntax-propertize.  It wouldn't make sense to do these in
syntax-propertize for a variety of other reasons, such as efficiency.

>> OK.  Can I suggest an alternative?  In C++ (and Java) Modes, the template
>> (generic) delimiters are marked with syntax-table text properties.
>> Unfortunately, at the moment this is done as part of font-locking, so
>> isn't done until you display.  However, if you put "(sit-for 0)" into
>> your code after inserting "< ... >", this will cause a redisplay,
>> allowing subsequent code to use the text properties, and a backward-sexp
>> will then work.

> Sounds like it should be performed during `syntax-propertize' instead. 
> Even if you don't like this idea now, I suspect it'll happen eventually 
> anyway, if only for consistency with other major modes.

Maybe it will, maybe it won't.  `syntax-propertize' has the disadvantage
of inefficiency; at any buffer change, all syntax-table text properties
after the position of the change are effectively wiped out, even where
(as is usual) this isn't necessary.  Also, restoring them means doing so
over a potentially large region of the buffer, rather than just locally
around point.

But, anyway ....

> In that sense, the `sit-for' suggestion is not future-proof. So I'll try 
> only changing the syntax table around specific functions that don't 
> modify the buffer text, but just move point, since that was the actual goal.

Might it be possible that you could get mixed up with less-than and
greater-than (or even shift-right) operators?  If so, be careful!

>> In the medium future (several weeks away), I'm hoping to fix CC Mode so
>> that the text properties are applied to < ... > on an after-change
>> function rather than at redisplay.

...., a change to use syntax-propertize needs the above change to happen
first.

> Hmm. My current workaround is to use a temporary syntax table that 
> inherits from c++-mode-syntax-table (but has angle brackets in the 
> 'paren' class). Sounds like it might also create a conflict, if maybe a 
> more subtle one.

Any reason you don't use the current syntax table as the base to add the
descriptors for < and > to rather than creating a new, blank one?  You
could use `copy-syntax-table'.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/27/2014 12:38 AM, Alan Mackenzie wrote:

> They're not syntax-table text properties, so the setting of them doesn't
> belong in syntax-propertize.

Well, they are used to set syntax information, so syntax-propertize is a 
much better place for them, conceptually, than font-lock.

> Maybe it will, maybe it won't.  `syntax-propertize' has the disadvantage
> of inefficiency; at any buffer change, all syntax-table text properties
> after the position of the change are effectively wiped out, even where
> (as is usual) this isn't necessary.

If it's a actually a problem, maybe you need to be able to use a custom 
invalidation strategy. Aside from the strategy itself, this shouldn't be 
too hard to implement.

>> In that sense, the `sit-for' suggestion is not future-proof. So I'll try
>> only changing the syntax table around specific functions that don't
>> modify the buffer text, but just move point, since that was the actual goal.
>
> Might it be possible that you could get mixed up with less-than and
> greater-than (or even shift-right) operators?  If so, be careful!

Thanks, but there are no "less-than" in a function signature.

>>> In the medium future (several weeks away), I'm hoping to fix CC Mode so
>>> that the text properties are applied to < ... > on an after-change
>>> function rather than at redisplay.
>
> ...., a change to use syntax-propertize needs the above change to happen
> first.

Not sure why, but ok.

>> Hmm. My current workaround is to use a temporary syntax table that
>> inherits from c++-mode-syntax-table (but has angle brackets in the
>> 'paren' class). Sounds like it might also create a conflict, if maybe a
>> more subtle one.
>
> Any reason you don't use the current syntax table as the base to add the
> descriptors for < and > to rather than creating a new, blank one?  You
> could use `copy-syntax-table'.

Is that much different from what I described above? I do

   (with-syntax-table (make-syntax-table (syntax-table)
     ...)

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Alan Mackenzie]

Hello, Dmitry.

On Mon, Oct 27, 2014 at 08:06:46AM +0700, Dmitry Gutov wrote:
> On 10/27/2014 12:38 AM, Alan Mackenzie wrote:

> > They're not syntax-table text properties, so the setting of them doesn't
> > belong in syntax-propertize.

> Well, they are used to set syntax information, so syntax-propertize is a 
> much better place for them, conceptually, than font-lock.

They (that is, the c-is-sws and c-in-sws properties) mark syntactic
whitespace.  They're set during calls to c-forward-syntactic-ws and
c-backward-syntactic-ws, which are used all over the place, just as much
in the indentation engine, and the commands, as in font lock.  They're
intended to mark CPP structures and, especially, massive comments, such
as are frequently found at the beginning of source files.  They're
intended to speed up the skipping of WS.

> > Maybe it will, maybe it won't.  `syntax-propertize' has the disadvantage
> > of inefficiency; at any buffer change, all syntax-table text properties
> > after the position of the change are effectively wiped out, even where
> > (as is usual) this isn't necessary.

> If it's a actually a problem, maybe you need to be able to use a custom 
> invalidation strategy. Aside from the strategy itself, this shouldn't be 
> too hard to implement.

It's already implemented and running.  :-)  The syntax-table properties
in the vicinity of a buffer change are removed and re-applied with tender
loving care.

[ ... ]

> >> Hmm. My current workaround is to use a temporary syntax table that
> >> inherits from c++-mode-syntax-table (but has angle brackets in the
> >> 'paren' class). Sounds like it might also create a conflict, if maybe a
> >> more subtle one.

> > Any reason you don't use the current syntax table as the base to add the
> > descriptors for < and > to rather than creating a new, blank one?  You
> > could use `copy-syntax-table'.

> Is that much different from what I described above? I do

>    (with-syntax-table (make-syntax-table (syntax-table)
>      ...)

No it's not.  Apologies.  I hadn't read your email properly.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#18826: 24.3.94; c++-mode bad indentation after programmatic insert with locally changed syntax table

[Dmitry Gutov]

On 10/27/2014 03:53 PM, Alan Mackenzie wrote:

> They (that is, the c-is-sws and c-in-sws properties) mark syntactic
> whitespace.  They're set during calls to c-forward-syntactic-ws and
> c-backward-syntactic-ws, which are used all over the place, just as much
> in the indentation engine, and the commands, as in font lock.  They're
> intended to mark CPP structures and, especially, massive comments, such
> as are frequently found at the beginning of source files.  They're
> intended to speed up the skipping of WS.

It sounds like it can be implemented in syntax-propertize-function 
without much trouble. Adding such props in movement commands sounds 
counter-intuitive, although I can see how this can be useful from the 
performance perspective.

> It's already implemented and running.  :-)  The syntax-table properties
> in the vicinity of a buffer change are removed and re-applied with tender
> loving care.

Cool. It'll require a patch to syntax.el in the trunk, right?

> No it's not.  Apologies.  I hadn't read your email properly.

That's good. I'll take that to mean that < and > having belonging to 
paren class won't stop them from being propertized in the 
before/after-change hooks like you described. Then I'll keep the code 
as-is, thanks!