Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it.

[João Távora]

Hello Alan,

Alan Mackenzie <address@hidden> writes:

> First of all, I will say that I take exception, great exception, to
> your means of expressing yourself, both in your post and (even more
> so) in the commit you made.  It verges on sarcasm, and is totally over
> the top and uncalled for.

The only sarcastic expression I can read in my post is "volkswagenesque
atrocity".  All the rest is dead serious.  And even the VW reference is
appropriate.

It's not over the top, and I hope to explain why further down.

>> > > with it.
>
>> > That would leave lots of failed tests in make check.  People have
>> > already remarked on those failures, implicitly asking me to fix them.
>
>> Yes. But the correct fix was to revert the commit that broke
>> the tests, not this volkswagenesque atrocity.
>
> The tests are now broken.  They make assumptions about CC Mode's workings
> which don't hold.  Why can't we cooperate to fix them?

Of course we can!  But let's first analyse what these assumptions are
instead of breaking the tests.

Perhaps you have the impression that tests are somehow non-real, because
they are just code.  But you broke actual things in my (and probably
other's people usage of C-mode, C++-mode, Awk-mode etc).  Things that
have been working since Emacs 24.4 and that suddently stop working.  If
I don't stand up now it'll be like the last time and the bug will stay.
And this personally affects my work and other's.  You must take that
into account.

>> > My understanding, from a previous encounter, was that having no failing
>> > unit tests was of a high priority.  I've only commented a little bit
>> > out, I haven't made any permanent, unreverseable changes.
>> Then you'll certainly be OK if I revert your two commits myself:
> I am not at all OK about this.

Why?  I'm using the exact same argument you made: It's not permanent or
"unreversable", since we have a VCS.

>> the commit that broke the tests and the commit that disables the test.
>> That is certainly not permanent or unreversable, too!
> No, but the amount of work it now requires is greater that immediately
> after my commit.

?

Have read commit be505726b68d407a44fdcd9c7ac1ef722398532d?

You do realize that your commit is still in place right?  As we speak
users that build Emacs masters or install "emacs-snapshot" builds from
Ubuntu are seeing the behaviour you intend.  (And they have had
electric-pair-mode's features blown away from them).  I'm not saying
we're going to see pouring bug reports, but the damage is real?

>> > With that test in, I got the error message: "No test named
>> > `electric-pair-whitespace-chomping-2-at-point-4-in-c++-mode-in-strings'",
>> > and no other tests were performed, leaving an electric-tests.log file 86
>> > bytes long.  That's why I commented it out.  This may be some glitch in
>> > the testing system.
>> I see, you disabled all c++ tests, *sigh*
> Yes.  I couldn't understand the (undocumented) structure of these tests
> well enough to make better targetted amendments.

Then you should have asked before.

Now, I will explain something. c++ exists in this line

          (modes '(quote (ruby-mode c++-mode)))

But it could very well be

          (modes '(quote (ruby-mode c-mode awk-mode js-mode c++-mode 
perl-mode)))

This runs 1250 tests instead of the usual 400

    Selector: t
    Passed:  1247
    Failed:  4 (3 unexpected)  ;; this is related to the thing you broke
                               ;; 6 months ago, they're all cc-mode-based
    Skipped: 0
    Total:   1251/1251

That is to say, every mode you plug there should behave exactly the same
regarding delimiter insertion and deletion.  This is
electric-pair-mode's contract.

c++-mode is there only as a "lab mouse", it stands for all other cc-mode
related modes.

>> > > C Mode doesn't use electric-layout mode, but a user can surely
>> > > decide he wants to use it in c-mode, can he not??
>
>> > No.  Certainly not at the moment.
>
>> No? Will you come to his house and tell him personally not to
>> M-x electric-layout-mode? What if that's what he _prefers_?
>
> That user is purely hypothetical.

No, he's not! I use electric-layout-mode from time to time!  And
Beatrix's problem could very well be solved by it.  In fact if you put
this in your .emacs today, you're very close to getting
c-toggle-auto-newline for braces, and still follow your style variables
and whatnot.  All without c-toggle-auto-newline.

(setq electric-layout-rules '(electric-layout-for-c-style-du-jour))

(setq c--disable-fix-of-bug-33794 t)

(defun electric-layout-for-c-style-du-jour (inserted)
  (when (and (derived-mode-p 'c-mode 'c++-mode)
             (memq inserted '(?{ ?})))
    (save-excursion
      (backward-char)
      (c-brace-newlines (c-point-syntax)))))


> And if there actually were such a user, I would urge him to use CC
> Mode's auto-newline mode, which is much better tailored to CC Mode
> than a generic function could be.

In what ways?  Alan, can you make an objective criticism of
electric-layout-mode?  I'm not saying it can't be improved, but since
you like to bash it so much, can we read a reason?  I could use that to
improve it.

> And if he insisted, then maybe we could look at adding
> electric-layout-mode support to CC Mode.  But it would be a lot of work
> for little return, given that auto-newline mode exists, works, and works
> well.

In that case I insist!

I've shown above that it isn't!  Perhaps it's not perfect yet, but
certainly doesn't look like a lot of work.

Please please read the new electric-layout-mode API.  Just M-x
describe-variable RET eletric-layout-rules.

>> > > These tests pass fine currently.
>
>> > When I ran them, there were lots of failures, because the tests assumed
>> > electric-layout-mode was active.
>
>> The tests should activate electric-layout-mode temporarily if needed and
>> the revert to the previous situation.  If they aren't doing this, that is 
>> what
>> needs to be fixed, but it works for me and I don't have electric-layout-mode,
>> too.
>
> electric-layout-mode doesn't, and can't work with CC Mode, in particular
> with the c-electric-.... functions, because these are incompatible with a
> non-nil post-self-insert-hook.

They aren't generally incompatible, it's just this corner that Beatrix
reported that kicked the hornets nest.  I've been working with c/c++ and
electric-pair-mode for 5+ years now, and it works perfectly if you don't
turn on c-auto-newline.

But if you want to improve the situation for those that do turn on
c-auto-newline, why not create a

(defun c-self-insert-command (arg)
    (let ((post-self-insert-hook
            (if (some-condition-that-joaot-and-I-have-agreed)
                nil
                post-self-insert-hook)))
         (self-insert-command arg)))

It's much easier.


>> > > Please revert this fix and lets discuss why you need to disable tests.
>
>> > It's not a fix, it's a temporary workaround.
>
>> With all due respect, I've heard that one before many many times.
>
>> Do you realize that you've broken many many things about
>> electric-pair-mode?
>
> No, I don't.  It would have been good if you could have supplied me with
> specific recipes to reproduce where it (electric-pair-mode) goes wrong,
> thus allowing me to fix them.

The tests themselves supply those recipes.  M-x ert-describe-test does
really good job, if you don't get hung up. 

>
>> For example Beatrix, and me and everyone else will now lose
>> the default "autobalancing" functionality of electric-pair-mode,
>> which is its best feature! Autobalance used to work identically in
>> most modes, but now you broke that. Is it worth breaking so
>> many things just for fixing a much smaller case?
>
>> Just one example of one of the many cases you broke.
>
> What is "autobalancing" (the term doesn't appear in elec-pair.el) and how
> is it broken?  Why can't you cite a recipe to reproduce the alleged
> breakage?

See the docstring of electric-pair-preserve-balance.  And read the
test's descriptions.

>> #[nil "\300\301!\207"
>>       [electric-pair-mode 1]
>>       2]
>
> Pardon?  Do you mean M-: (electric-pair-mode 1)?

Of course I do mean that. Come on, this doc is autogenerated and I was
in a hurry last time, I can do some form-walking later-on to provide a
perfectly pristine instruction without that glitch.

>> The buffer's contents should become:
>
>>   |  (()])  |
>
>> , and point should be at 5.
>
> Indeed, this is what I observe.

That's because the tests are setting c--disable-fix-of-bug-33794 to t!

IOW I made my own Volkswagenesque atrocity, but at least mine is easier
to identify while we analyse the problem.  And it gives me (and perhaps
other) the opportunity to develop in a master Emacs in C/C++/whatever
without the failures.

If you roll back to just after your fix (the version Michael Albinus was
using), you'll see it break.  You can also see it break if you remove
the line of c--disable-fix-of-bug-33794.


>> You turn this into (())]) so now I have two unbalanced parenthesis types
>> in my buffer.
>
> Are you sure?  Wherein then, lies the difference between your compiled
> binary above and M-: (electric-pair-mode 1)?  Could this be a bug in
> e-p-m, or the byte compiler?

No, you're just testing with the wrong version.

This one test was picked out of the 85 or so that were broken (just for
C++, of course this number is repeated in C/Awk/Pike, whatever by )

>> > Anyhow, surely the implementation of Emacs should not be constrained by
>> > its tests?  Rather the tests should test the chosen implementation.
>
>> Really Alan, you completely broke electric-pair-mode by trying to
>> reimplement it in cc-mode.el where it was working perfectly but for a small
>> glitch (which really isn't its fault).
>
> I think you are exaggerating somewhat.  And you'll find I didn't
> "reimplement" electric-pair-mode, even though I was talking about this at
> one stage.  CC Mode calls electric-pair-post-self-insert-function, but in
> a controlled fashion at suitable places.
>
> And I disagree about your "small glitch" characterisation.  Adding
> post-self-insert-hook to Emacs broke, fundamentally, c-electric-...  This
> was the breakage that Beatrix Klebe's bug uncovered.  My patch fixed
> it.

It broke the interaction of c-auto-newline with electric-pair-mode mode
as far as I know.  This is what was described in bug#33794 by Beatrix
Klebe.  Does it break more things?  Does it break more things related to
electric-pair-mode?

>> If you're going to reimplement it, reimplement *all* of it (of course you 
>> might
>> come to the conclusion that its best to use the existing implementation...)
> I have used the existing implementation.

The function you called is not meant to be called from that context.
It's not an "external" function.  Shall I rename it
electric----pair-post-self-insert-function :-)

>> > > If we come to the conclusion that some tests are asserting unreasonable
>> > > expectations about the functionality you develop, we can disable them on 
>> > > a
>> > > case by case basis!
>
>> > I would have done that, indeed tried to do that, but the lack of
>> > documentation of electric-pair-test-for, electric-pair-define-test-form
>> > and so on, many of them with 8, 9 or more parameters, made that too
>> > difficult, given the urgency I felt to produce a workaround.
>
>> If this is indeed urgent, you can add some conditional check to the
>> offending code using 'ert-running-test'.
>
> Maybe you could do this.  I'm not familiar enough with the code in
> electric-test.el, important pieces of which, I repeat myself, are wholly
> undocumented.

It's much easier to do this with a variable that I used: then ert, and
some users can set that variable.  And it is even easier if we introduce
c-self-insert-command and think about the problem.

>> > > If on the other hand, if you need to do some work "temporarily", then
>> > > the best way to do it without disturbing other people's developments
>> > > is to do it in a separate branch.
>
>> > I fixed bug #33794[*] on master, as I always do with CC Mode bugs (and most
>> > other sorts of bugs, too).  That fix is, in principle, sound.  I didn't
>> > discover the problems with the unit tests till later (though I admit I
>> > should have done).
>
>> It's *not* sound if it breaks tests. At least not without first arguing
>> that the tests are placing unreasonable expectations on your code.
>
> The tests do place such unfounded expectations on the code.  In
> particular, the contents of post-self-insert-hook.

The tests assume modes won't

  (let ((post-self-insert-hook)) (self-insert-command))

yes.  I think that's a reasonable assumption.

Besides tests I also assume that!  I want my post-self-insert-hook to
work.  I want to write modes based on it, and I want to order pizzas for
Donald Trump every for every character I type!

>> > [*] That is, Beatrix Klebe's bug about CC Mode's auto-newlines not
>> > working with electric-pair-mode.
>> Beatrix Klebe, can, as a workaround, use electric-layout-mode
>> perfectly well for her use case (even though you insist she can't)
>> Until you can sort out c++-mode.
> I've sorted out CC Mode.  Beatrix Klebe does not need any workarounds,
> given that the bug she reported has been properly fixed.  It is likely
> she is currently using that fix.

You opened 83 or so other bugs times just for C++-mode!  Why couldn't
you tell Beatrix: look don't use electric-pair-mode in cc-mode when
using c-auto-newline??  

>> It's much easier than creating this mess that really interferes
>> with other's peoples work.
>
> I would ask you to consider that the mess was created by you, last night,
> and it interferes greatly with my work.  This mess was an order of
> magnitude greater than the mess you assert I made in
> electric-tests.el.

How so?  Really, explain how!  I did *not* remove your fix.  I just made
it possible to disable it!

> I don't consider the temporary changes I made in electric-tests.el were
> unreasonable, and if you disagree you could have discussed it with me in
> a less disagreeable fashion than what transpired.
>
> And now, it seems your idea is to leave special purpose code in
> CC Mode, just to obviate the need to amend electric-tests.el.  As you can
> imagine, I'm not keen on that.

It is you who left a buggy, incomplete re-implementation of
electric-pair-mdoe in cc-mode.el in the first place.  It's not needed,
Alan.

1. CC-mode can be thinner after all this
2. Beatrix can be happy
3. You could be happy
4. I could be happy
5. Donald Trump would have to pay and eat a million pizzas

> That variable must go.

It can.  Let's use a c-self-insert-command function.  Or macro, if you
prefer.  Then let's check clearly the conditions it should delegate to
the previous or the new behaviour.

But let's do this is a branch yes?  I've just pushed

   git push origin scratch/resolve-cc-mode-and-e-p-m
   Total 0 (delta 0), reused 0 (delta 0)
   remote: Sending notification emails to: address@hidden
   To git.sv.gnu.org:/srv/git/emacs.git
    * [new branch]            scratch/resolve-cc-mode-and-e-p-m -> 
scratch/resolve-cc-mode-and-e-p-m

> I have another suggestion: you could amend electric-tests.el not to
> attempt to use CC Mode at all - or stick with plainer-c-mode, possibly
> writing a plainer-c++-mode.  After all, the purpose is to test the
> electric-... functions, not CC mode.  That could save a lot of argument.

I would have to write plainer-<everything-based-on-cc-mode>-for that.
And give that to people working with me.  And teach them to enable these
new version.  It's ridiculous.

But I guess it's an option if we make them default in auto-mode-alist
and such.  Quite a nuclear option, tho.

João