Re: [Emacs-diffs] master cd06d17: Fix bug with face-id after restoring from pdump

Re: [Emacs-diffs] master cd06d17: Fix bug with face-id after restoring from pdump

[Daniel Colascione]

On 1/28/19 7:25 AM, Eli Zaretskii wrote:
> branch: master
> commit cd06d173a602bf0aa8a227ff1626dc70013fe480
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
> 
>      Fix bug with face-id after restoring from pdump

I was going to fix this problem by just turning lface_id_to_name into a 
normal Lisp vector and using its size as the next face ID. Why use this 
more complicated approach?

Re: master cd06d17: Fix bug with face-id after restoring from pdump

[Glenn Morris]

Eli Zaretskii wrote:

> branch: master
> commit cd06d173a602bf0aa8a227ff1626dc70013fe480

>     Fix bug with face-id after restoring from pdump

This fails to bootstrap on hydra.nixos.org, ref
https://hydra.nixos.org/build/88033376

It also fails on RHEL 7.6.

Re: [Emacs-diffs] master cd06d17: Fix bug with face-id after restoring from pdump

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Date: Mon, 28 Jan 2019 08:05:13 -0800
> 
> On 1/28/19 7:25 AM, Eli Zaretskii wrote:
> > branch: master
> > commit cd06d173a602bf0aa8a227ff1626dc70013fe480
> > Author: Eli Zaretskii <eliz@gnu.org>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> > 
> >      Fix bug with face-id after restoring from pdump
> 
> I was going to fix this problem by just turning lface_id_to_name into a 
> normal Lisp vector and using its size as the next face ID. Why use this 
> more complicated approach?

Based on previous discussion regarding frames and faces, I was under
the impression that you prefer moving stuff to the startup of emacs.
I'm not wed to my solution, feel free to change if you think it's
better.

Re: [Emacs-diffs] master cd06d17: Fix bug with face-id after restoring from pdump

[Daniel Colascione]

>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Mon, 28 Jan 2019 08:05:13 -0800
>>
>> On 1/28/19 7:25 AM, Eli Zaretskii wrote:
>> > branch: master
>> > commit cd06d173a602bf0aa8a227ff1626dc70013fe480
>> > Author: Eli Zaretskii <eliz@gnu.org>
>> > Commit: Eli Zaretskii <eliz@gnu.org>
>> >
>> >      Fix bug with face-id after restoring from pdump
>>
>> I was going to fix this problem by just turning lface_id_to_name into a
>> normal Lisp vector and using its size as the next face ID. Why use this
>> more complicated approach?
>
> Based on previous discussion regarding frames and faces, I was under
> the impression that you prefer moving stuff to the startup of emacs.
> I'm not wed to my solution, feel free to change if you think it's
> better.

The default approach to pretty much anything should be to use the first
entry in this implementation strategy that will work for a particular
problem.

1) Write it in Lisp
2) Write it in C using Lisp data structures
3) Use custom data structures in C, but allocated from the Lisp heap
4) malloc some random stuff for a pure-C approach

Right now, the face stuff is #3, but I think we could move it to #2 using
the approach in my previous message (just using an ordinary staticprod
Lisp_Object instead of a Lisp_Object* with a size field). Approaches #3
and #4 need special code for pdumper to work, but #1 and #2 Just Work, and
to the greatest extent possible, we should use #1 or #2 for new feature
work too. It'll reduce the total amount of code we need.

Re: master cd06d17: Fix bug with face-id after restoring from pdump

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 28 Jan 2019 12:58:43 -0500
> 
> Eli Zaretskii wrote:
> 
> > branch: master
> > commit cd06d173a602bf0aa8a227ff1626dc70013fe480
> 
> >     Fix bug with face-id after restoring from pdump
> 
> This fails to bootstrap on hydra.nixos.org, ref
> https://hydra.nixos.org/build/88033376
> 
> It also fails on RHEL 7.6.

Sorry about that, I hope I fixed that now.  But there was a real
reason for the failure...

Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

João, while debugging the problem uncovered by Hydra, I found out that
Flymake puts its own values of the 'face' property on its faces.  For
example:

  (put 'flymake-error 'face 'flymake-error)

This causes the following trouble:

  (face-id 'flymake-error)
    => flymake-error

This is unexpected, since face-id is supposed to return a numeric ID
of a face, otherwise make-glyph-code will break, for example.

IOW, the 'face' property of a face symbol is reserved and shouldn't be
used by packages in any other way except as explained above.  Can
Flymake please behave in this regard?

Thanks.

Re: [Emacs-diffs] master cd06d17: Fix bug with face-id after restoring from pdump

[Eli Zaretskii]

> Date: Mon, 28 Jan 2019 11:37:07 -0800
> From: "Daniel Colascione" <dancol@dancol.org>
> Cc: "Daniel Colascione" <dancol@dancol.org>,
>  emacs-devel@gnu.org
> 
> > Based on previous discussion regarding frames and faces, I was under
> > the impression that you prefer moving stuff to the startup of emacs.
> > I'm not wed to my solution, feel free to change if you think it's
> > better.
> 
> The default approach to pretty much anything should be to use the first
> entry in this implementation strategy that will work for a particular
> problem.
> 
> 1) Write it in Lisp
> 2) Write it in C using Lisp data structures
> 3) Use custom data structures in C, but allocated from the Lisp heap
> 4) malloc some random stuff for a pure-C approach
> 
> Right now, the face stuff is #3, but I think we could move it to #2 using
> the approach in my previous message (just using an ordinary staticprod
> Lisp_Object instead of a Lisp_Object* with a size field). Approaches #3
> and #4 need special code for pdumper to work, but #1 and #2 Just Work, and
> to the greatest extent possible, we should use #1 or #2 for new feature
> work too. It'll reduce the total amount of code we need.

I wasn't talking about the implementation language and other details,
I was talking about whether some initialization should be recorded in
the pdump file or re-done anew in the emacs session.  When something
doesn't "just work" after restoring from pdump, the question is
whether you add code to make it work as in unexec, or you just
recompute the necessary variables in emacs.

Re: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

On Mon, Jan 28, 2019 at 8:12 PM Eli Zaretskii <eliz@gnu.org> wrote:

> IOW, the 'face' property of a face symbol is reserved and shouldn't be
> used by packages in any other way except as explained above.  Can
> Flymake please behave in this regard?

Sure, I guess it's a question of using the property flymake-face.
I had no idea 'face' was reserved (how could/can I I know this)?

Wouldn't another (perhaps uglier, but easier) fix amount to renaming the
face 'flymake-error-face'? I think both fixes would be backward-
incompatible, but not "much".

For any of those, I'm away from my Emacs development machine right now,
so it'll have to wait a couple of days, or you someone can beat me to it.


João

Re: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 28 Jan 2019 20:39:41 +0000
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> On Mon, Jan 28, 2019 at 8:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > IOW, the 'face' property of a face symbol is reserved and shouldn't be
> > used by packages in any other way except as explained above.  Can
> > Flymake please behave in this regard?
> 
> Sure, I guess it's a question of using the property flymake-face.

Yes, that would be good.

> I had no idea 'face' was reserved (how could/can I I know this)?

No need to feel bad, I learned that very recently myself.

> Wouldn't another (perhaps uglier, but easier) fix amount to renaming the
> face 'flymake-error-face'?

Not sure how that would help in this matter.

> For any of those, I'm away from my Emacs development machine right now,
> so it'll have to wait a couple of days, or you someone can beat me to it.

There's no rush.

Re: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

On Mon, Jan 28, 2019 at 8:57 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > I had no idea 'face' was reserved (how could/can I I know this)?
> No need to feel bad, I learned that very recently myself.

Heh, I'm not feeling that bad :-)

But really, is there a list of reserved symbols somewhere? And if this is
reserved for presumably "internal" purposes, couldn't that symbol be
renamed to 'internal--face-id' or something like that?  Or are there too
many references?

> > Wouldn't another (perhaps uglier, but easier) fix amount to renaming the
> > face 'flymake-error-face'?
> Not sure how that would help in this matter.

As far as I can understand, this is because flymake-error, the flymake error
type, is conflated with flymake-error, the face. So if the face was
flymake-error-face  I think it wouldn't happen.  Have to check though, of
course.

> > For any of those, I'm away from my Emacs development machine right now,
> > so it'll have to wait a couple of days, or you someone can beat me to it.
> There's no rush.

OK.

-- 
João Távora

Re: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 28 Jan 2019 21:38:29 +0000
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> But really, is there a list of reserved symbols somewhere?

Not that I'm aware of.  But I added to the manual the description of
the role of the 'face' property of the face synbol.

> And if this is reserved for presumably "internal" purposes, couldn't
> that symbol be renamed to 'internal--face-id' or something like
> that?  Or are there too many references?

It's too late for that, I think.  Instead, packages should IMO try to
keep the global namespace clean in the property domain as well, thus
defining properties whose names have the prefix of the package name.

> > > Wouldn't another (perhaps uglier, but easier) fix amount to renaming the
> > > face 'flymake-error-face'?
> > Not sure how that would help in this matter.
> 
> As far as I can understand, this is because flymake-error, the flymake error
> type, is conflated with flymake-error, the face.

No, the problem is that each face has its numeric face ID stored as
the value of the face symbol's 'face' property.  So, no matter what is
the face symbol, its 'face' property should not be touched.

Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

[Sorry for the double email Eli, I forgot to CC the list in the first one]

On Tue, Jan 29, 2019 at 4:55 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > As far as I can understand, this is because flymake-error, the flymake error
> > type, is conflated with flymake-error, the face.
> No, the problem is that each face has its numeric face ID stored as
> the value of the face symbol's 'face' property.  So, no matter what is
> the face symbol, its 'face' property should not be touched.

Undoubtedly, but if the face changes the name to 'flymake-error-face' and
the diagnostic type retains the name 'flymake-error', then the 'face' property
will not be applied to a face, but to an arbitrary, non-face related symbol,
designating no more than a flymake diagnostic type (and not a face).
There is no reason why you would ask for the face ID of _that_ symbol,
is there?

> It's too late for that, I think.  Instead, packages should IMO try to
> keep the global namespace clean in the property domain as well, thus
> defining properties whose names have the prefix of the package name.

That is certainly true for properties whose semantics are valid only
within a package.  But flymake.el here is merely managing existing
properties of overlays designated by "external" symbols, such as
'face' ,'priority' ,'display', 'help-echo' ,etc... Now, flymake.el happened
to be unlucky enough to store these properties in the plist of a symbol
which, by doubling as a face symbol, already had some
implementation-specific a meaning for some of those properties.

So I'd first study the possibility of avoiding this "doubling as a
face symbol", which was merely aesthetic and even slightly confusing.

As a backward-compatible alternative to that, if it is not an immense
amount of work, we could look at the Emacs facility that treats 'face'
as an implementation detail (i.e. doesn't give it public meaning, contrary
to what text- and overlay properties do), and change it to use another
name for that implementation detail, such as <facility-name>--face-id
or something.

I think this second alternative is consistent with your views on the
global namespace.  It might be more work though.

--
João Távora

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 29 Jan 2019 17:26:50 +0000
> 
> > No, the problem is that each face has its numeric face ID stored as
> > the value of the face symbol's 'face' property.  So, no matter what is
> > the face symbol, its 'face' property should not be touched.
> 
> Undoubtedly, but if the face changes the name to 'flymake-error-face' and
> the diagnostic type retains the name 'flymake-error', then the 'face' property
> will not be applied to a face, but to an arbitrary, non-face related symbol,

Ah, okay.  Yes, this will solve the problem with the face.

> > It's too late for that, I think.  Instead, packages should IMO try to
> > keep the global namespace clean in the property domain as well, thus
> > defining properties whose names have the prefix of the package name.
> 
> That is certainly true for properties whose semantics are valid only
> within a package.  But flymake.el here is merely managing existing
> properties of overlays designated by "external" symbols, such as
> 'face' ,'priority' ,'display', 'help-echo' ,etc...

That is okay as long as the properties are used as documented.

> Now, flymake.el happened to be unlucky enough to store these
> properties in the plist of a symbol which, by doubling as a face
> symbol, already had some implementation-specific a meaning for some
> of those properties.

A 'face' property is documented for general use only for text, not for
symbol plists.

> As a backward-compatible alternative to that, if it is not an immense
> amount of work, we could look at the Emacs facility that treats 'face'
> as an implementation detail (i.e. doesn't give it public meaning, contrary
> to what text- and overlay properties do), and change it to use another
> name for that implementation detail, such as <facility-name>--face-id
> or something.
> 
> I think this second alternative is consistent with your views on the
> global namespace.  It might be more work though.

More importantly, I see no reason for such backward-incompatible
change.  It is easier to change one package, flymake, than to
potentially impose incompatible changes on external packages and user
customizations, even though this is an internal usage.  It just is too
veteran to change it for this reason.

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Daniel Colascione]

> [Sorry for the double email Eli, I forgot to CC the list in the first one]
>
> On Tue, Jan 29, 2019 at 4:55 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > As far as I can understand, this is because flymake-error, the flymake
>> error
>> > type, is conflated with flymake-error, the face.
>> No, the problem is that each face has its numeric face ID stored as
>> the value of the face symbol's 'face' property.  So, no matter what is
>> the face symbol, its 'face' property should not be touched.
>
> Undoubtedly, but if the face changes the name to 'flymake-error-face' and
> the diagnostic type retains the name 'flymake-error', then the 'face'
> property
> will not be applied to a face, but to an arbitrary, non-face related
> symbol,
> designating no more than a flymake diagnostic type (and not a face).
> There is no reason why you would ask for the face ID of _that_ symbol,
> is there?
>
>> It's too late for that, I think.  Instead, packages should IMO try to
>> keep the global namespace clean in the property domain as well, thus
>> defining properties whose names have the prefix of the package name.
>
> That is certainly true for properties whose semantics are valid only
> within a package.  But flymake.el here is merely managing existing
> properties of overlays designated by "external" symbols, such as
> 'face' ,'priority' ,'display', 'help-echo' ,etc... Now, flymake.el
> happened
> to be unlucky enough to store these properties in the plist of a symbol
> which, by doubling as a face symbol, already had some
> implementation-specific a meaning for some of those properties.
>
> So I'd first study the possibility of avoiding this "doubling as a
> face symbol", which was merely aesthetic and even slightly confusing.
>
> As a backward-compatible alternative to that, if it is not an immense
> amount of work, we could look at the Emacs facility that treats 'face'
> as an implementation detail (i.e. doesn't give it public meaning, contrary
> to what text- and overlay properties do), and change it to use another
> name for that implementation detail, such as <facility-name>--face-id
> or something.
>
> I think this second alternative is consistent with your views on the
> global namespace.  It might be more work though.
>

We could also just use an uninterned symbol.

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Daniel Colascione]

>> From: João Távora <joaotavora@gmail.com>
>> Date: Tue, 29 Jan 2019 17:26:50 +0000
>>
>> > No, the problem is that each face has its numeric face ID stored as
>> > the value of the face symbol's 'face' property.  So, no matter what is
>> > the face symbol, its 'face' property should not be touched.
>>
>> Undoubtedly, but if the face changes the name to 'flymake-error-face'
>> and
>> the diagnostic type retains the name 'flymake-error', then the 'face'
>> property
>> will not be applied to a face, but to an arbitrary, non-face related
>> symbol,
>
> Ah, okay.  Yes, this will solve the problem with the face.
>
>> > It's too late for that, I think.  Instead, packages should IMO try to
>> > keep the global namespace clean in the property domain as well, thus
>> > defining properties whose names have the prefix of the package name.
>>
>> That is certainly true for properties whose semantics are valid only
>> within a package.  But flymake.el here is merely managing existing
>> properties of overlays designated by "external" symbols, such as
>> 'face' ,'priority' ,'display', 'help-echo' ,etc...
>
> That is okay as long as the properties are used as documented.
>
>> Now, flymake.el happened to be unlucky enough to store these
>> properties in the plist of a symbol which, by doubling as a face
>> symbol, already had some implementation-specific a meaning for some
>> of those properties.
>
> A 'face' property is documented for general use only for text, not for
> symbol plists.
>
>> As a backward-compatible alternative to that, if it is not an immense
>> amount of work, we could look at the Emacs facility that treats 'face'
>> as an implementation detail (i.e. doesn't give it public meaning,
>> contrary
>> to what text- and overlay properties do), and change it to use another
>> name for that implementation detail, such as <facility-name>--face-id
>> or something.
>>
>> I think this second alternative is consistent with your views on the
>> global namespace.  It might be more work though.
>
> More importantly, I see no reason for such backward-incompatible
> change.  It is easier to change one package, flymake, than to
> potentially impose incompatible changes on external packages and user
> customizations, even though this is an internal usage.  It just is too
> veteran to change it for this reason.

Making this 'face property on random symbols special is bad design. It
affects only flymake *as far we know*, but that could be ignorance or
blind luck. Emacs shouldn't be using high-collision-probability names on
arbitrary symbols for internal purposes when other options are available.

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

On Tue, Jan 29, 2019 at 5:51 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > I think this second alternative is consistent with your views on the
> > global namespace.  It might be more work though.
>
> More importantly, I see no reason for such backward-incompatible
> change.  It is easier to change one package, flymake, than to
> potentially impose incompatible changes on external packages and user
> customizations, even though this is an internal usage.  It just is too
> veteran to change it for this reason.

The hypothetical change I was proposing would not be
backwards-incompatible, in fact, it would avoid the backward-
incompatibility that you are suggesting that I add to flymake.el.

This is of course assuming I understand "backward-incompatible"
the same way you do, as in: code and customization written by
third parties outside our control would break.

So we are probably taking about different things. But, since my
proposal is still vapourware (I haven't looked at the code yet),
there is no reason for discussing it any further right now.

-- 
João Távora

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

> Date: Tue, 29 Jan 2019 09:54:57 -0800
> From: "Daniel Colascione" <dancol@dancol.org>
> Cc: João Távora <joaotavora@gmail.com>,
>  emacs-devel@gnu.org
> 
> Making this 'face property on random symbols special is bad design.

I think I agree, but we are about 19 years late for that argument,
since this was introduced in Emacs 21.

Re: Fwd: Flymake and the 'face' property

[Johan Bockgård]

Eli Zaretskii <eliz@gnu.org> writes:

>> Now, flymake.el happened to be unlucky enough to store these
>> properties in the plist of a symbol which, by doubling as a face
>> symbol, already had some implementation-specific a meaning for some
>> of those properties.
>
> A 'face' property is documented for general use only for text, not for
> symbol plists.

The `category' feature for text/overlays uses symbol plists to define
its default properties. `face' is probably one the more common
properties to use in a category. Let's see...


...a few minutes later, I find this in rng-valid.el:


(defface rng-error '((t (:inherit font-lock-warning-face)))
[...]
(overlay-put overlay 'category 'rng-error)
[...]
(put 'rng-error 'face 'rng-error)

Re: Fwd: Flymake and the 'face' property

[Daniel Colascione]

On 1/29/19 10:30 AM, Eli Zaretskii wrote:
>> Date: Tue, 29 Jan 2019 09:54:57 -0800
>> From: "Daniel Colascione" <dancol@dancol.org>
>> Cc: João Távora <joaotavora@gmail.com>,
>>   emacs-devel@gnu.org
>>
>> Making this 'face property on random symbols special is bad design.
> 
> I think I agree, but we are about 19 years late for that argument,
> since this was introduced in Emacs 21.

Is it? I don't know how much would break if we fixed this misdesign, but 
my gut feeling is "not muhc".

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

Hello Eli and Daniel,

On Tue, Jan 29, 2019 at 5:54 PM Daniel Colascione <dancol@dancol.org> wrote:
> Making this 'face property on random symbols special is bad design. It
> affects only flymake *as far we know*, but that could be ignorance or
> blind luck. Emacs shouldn't be using high-collision-probability names on
> arbitrary symbols for internal purposes when other options are available.

Are you saying that no facilities in Emacs should reserve global
symbols for their own implementation details?  If so I agree, it is bad
design.

I though that flymake wasn't one of those packages, that it was
merely storing potential values for an overlay's plist somewhere else:
that there was no flymake-specific meaning given to 'face'.

But now I'm not so sure: looking at the code overlay-specific categories
*aren't* being stored in plists, so this should be some obsolete leftover
that we could in theory just remove.  I hope it is, but I have to double-check.

Doesn't change Daniel's "bad design" argument IMO, tho.

-- 
João Távora

Re: Fwd: Flymake and the 'face' property

[João Távora]

Thanks Johan, but this should be legal as long as rng-error
is *not* used as the name for a face.

But indeed I think Eli's rule-of-thumb is wrong there.

João

On Tue, Jan 29, 2019 at 6:34 PM Johan Bockgård <bojohan@gnu.org> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> Now, flymake.el happened to be unlucky enough to store these
> >> properties in the plist of a symbol which, by doubling as a face
> >> symbol, already had some implementation-specific a meaning for some
> >> of those properties.
> >
> > A 'face' property is documented for general use only for text, not for
> > symbol plists.
>
> The `category' feature for text/overlays uses symbol plists to define
> its default properties. `face' is probably one the more common
> properties to use in a category. Let's see...
>
>
> ...a few minutes later, I find this in rng-valid.el:
>
>
> (defface rng-error '((t (:inherit font-lock-warning-face)))
> [...]
> (overlay-put overlay 'category 'rng-error)
> [...]
> (put 'rng-error 'face 'rng-error)



-- 
João Távora

Re: Fwd: Flymake and the 'face' property

[Daniel Colascione]

On 1/29/19 10:47 AM, João Távora wrote:
> Hello Eli and Daniel,
> 
> On Tue, Jan 29, 2019 at 5:54 PM Daniel Colascione <dancol@dancol.org> wrote:
>> Making this 'face property on random symbols special is bad design. It
>> affects only flymake *as far we know*, but that could be ignorance or
>> blind luck. Emacs shouldn't be using high-collision-probability names on
>> arbitrary symbols for internal purposes when other options are available.
> 
> Are you saying that no facilities in Emacs should reserve global
> symbols for their own implementation details?  If so I agree, it is bad
> design.

Yeah. With respect to plists, it's okay to attach public symbols to 
private plists, and it's okay to put private symbols on public plists, 
but putting public symbols on public plists is creating a bug machine.

Re: Fwd: Flymake and the 'face' property

[João Távora]

On Tue, Jan 29, 2019 at 6:54 PM Daniel Colascione <dancol@dancol.org> wrote:
> > Are you saying that no facilities in Emacs should reserve global
> > symbols for their own implementation details?  If so I agree, it is bad
> > design.
>
> Yeah. With respect to plists, it's okay to attach public symbols to
> private plists, and it's okay to put private symbols on public plists,
> but putting public symbols on public plists is creating a bug machine.

Hmmm, attach == put, right?

I do think that putting public symbols on public plists *with
package-specific semantics* (public or not) is bad.  That's
the only nono I see.

BTW, CL packages solve this neatly blablabla (we've been through
that, I think).




--
João Távora

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: Johan Bockgård <bojohan@gnu.org>
> Cc: João Távora <joaotavora@gmail.com>,
>   emacs-devel@gnu.org
> Date: Tue, 29 Jan 2019 19:34:08 +0100
> 
> > A 'face' property is documented for general use only for text, not for
> > symbol plists.
> 
> The `category' feature for text/overlays uses symbol plists to define
> its default properties. `face' is probably one the more common
> properties to use in a category. Let's see...
> 
> 
> ...a few minutes later, I find this in rng-valid.el:
> 
> 
> (defface rng-error '((t (:inherit font-lock-warning-face)))
> [...]
> (overlay-put overlay 'category 'rng-error)
> [...]
> (put 'rng-error 'face 'rng-error)

I don't see how this contradicts what I said.  Do you?

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> Cc: joaotavora@gmail.com, emacs-devel@gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Tue, 29 Jan 2019 10:43:20 -0800
> 
> > I think I agree, but we are about 19 years late for that argument,
> > since this was introduced in Emacs 21.
> 
> Is it? I don't know how much would break if we fixed this misdesign, but 
> my gut feeling is "not muhc".

I wouldn't want even to consider this without a very good reason.
It's an obscure feature that doesn't bother anyone.

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 29 Jan 2019 18:47:19 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> this should be some obsolete leftover

It isn't.  It is used in face-id, which is used in display tables.

Re: Fwd: Flymake and the 'face' property (was: master cd06d17: Fix bug with face-id after restoring from pdump)

[João Távora]

On Tue, Jan 29, 2019 at 7:21 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Tue, 29 Jan 2019 18:47:19 +0000
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> >
> > this should be some obsolete leftover
>
> It isn't.  It is used in face-id, which is used in display tables.

We are miscommunicating again.  I meant obsolete in flymake.el,
not the conflicting package.  Thus all I would have to do is
remove it from flymake.

Except it isn't obsolete, unfortunately. More on that soon.

João



-- 
João Távora

Re: Fwd: Flymake and the 'face' property

[João Távora]

On Tue, Jan 29, 2019 at 7:18 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Johan Bockgård <bojohan@gnu.org>
> > Cc: João Távora <joaotavora@gmail.com>,
> >   emacs-devel@gnu.org
> > Date: Tue, 29 Jan 2019 19:34:08 +0100
> >
> > > A 'face' property is documented for general use only for text, not for
> > > symbol plists.
> > (put 'rng-error 'face 'rng-error)
>
> I don't see how this contradicts what I said.  Do you?

Well you did say that 'face should *not* be put on symbol plists,
but the documentation and the code states otherwise.  So the
guideline has to be adjusted, IMO:

  A 'face' property is documented for general use only for text or
  symbol plists, iff those symbols are guaranteed to never ever
  designate faces, i.e. there is no (defface symbol ...).

João

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 29 Jan 2019 19:33:33 +0000
> Cc: Johan Bockgård <bojohan@gnu.org>, 
> 	emacs-devel <emacs-devel@gnu.org>
> 
> > > > A 'face' property is documented for general use only for text, not for
> > > > symbol plists.
> > > (put 'rng-error 'face 'rng-error)
> >
> > I don't see how this contradicts what I said.  Do you?
> 
> Well you did say that 'face should *not* be put on symbol plists,
> but the documentation and the code states otherwise.

A single well-documented exception doesn't invalidate the rule.

Re: Fwd: Flymake and the 'face' property

[João Távora]

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

On Tue, Jan 29, 2019, 19:48 Eli Zaretskii <eliz@gnu.org wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Tue, 29 Jan 2019 19:33:33 +0000
> > Cc: Johan Bockgård <bojohan@gnu.org>,
> >       emacs-devel <emacs-devel@gnu.org>
> >
> > > > > A 'face' property is documented for general use only for text, not
> for
> > > > > symbol plists.
> > > > (put 'rng-error 'face 'rng-error)
> > >
> > > I don't see how this contradicts what I said.  Do you?
> >
> > Well you did say that 'face should *not* be put on symbol plists,
> > but the documentation and the code states otherwise.
>
> A single well-documented exception doesn't invalidate the rule.
>

You must mean the unwritten, inadvertently created rule, right? Ok, no
problem, exceptions are fine as long as we document them.

But I think flymake is using the exception that I meant to document, i.e.
it is using overlay categories. It can either stop using them or change the
face name. Both alternatives are backward incompatible to users, thought
perhaps not "by much", as that part of flymake might have been a wee bit
overdesigned. Have to check better.

I still wonder, as Daniel, if using some other symbol for the display code
would be very disruptive.

João

>

[-- Attachment #2: Type: text/html, Size: 2393 bytes --]

Re: Fwd: Flymake and the 'face' property

[Stefan Monnier]

> A 'face' property is documented for general use only for text, not for
> symbol plists.

But the `category` text-property uses as text-properties the properties
of a symbol, so there is a real conflict in our current design.

I think it might be worth taking a look at how hard/easy it would be to
change the place where we store the face-id.  IOW I wonder how much code out
there depends on it.  I have a vague recollection that many years ago
there might have been code that used (get <foo> 'face) in order to test
if `foo` is a face (because `facep` wasn't available yet, or only in
XEmacs or something like that), but I don't know if my memory is right,
nor do I know how much of that code might have survived until now.
Outside of this I can't remember ever seeing 3rd-party Elisp code
that depends on those internal details of our face system.

Also, even if there is code out there that looks at this `face`
property, maybe we change the property without breaking those packages
(e.g. by storing the face-id in both properties, but only looking them
up in the new property).


        Stefan

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 30 Jan 2019 04:37:32 -0500
> 
> > A 'face' property is documented for general use only for text, not for
> > symbol plists.
> 
> But the `category` text-property uses as text-properties the properties
> of a symbol, so there is a real conflict in our current design.

The 'category' symbols aren't faces, so there's no problem here.  the
'face' property has special meaning only on symbols that name a face.

> I think it might be worth taking a look at how hard/easy it would be to
> change the place where we store the face-id.

I think you greatly exaggerate the importance of this incident.  What
we have is a single package that happened to inadvertently bump into
this because it has a face and another symbol by the same name, _and_
that package also puts a 'face' property on the symbol.  The solution,
which was already proposed, is to rename the face.  Other than that,
this design is working flawlessly since the day it was introduced 19
years ago.

So I see no reason to waste any more energy on this.  We have more
important issues to tend to.

Re: Fwd: Flymake and the 'face' property

[João Távora]

On Wed, Jan 30, 2019 at 3:41 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Date: Wed, 30 Jan 2019 04:37:32 -0500
> >
> > > A 'face' property is documented for general use only for text, not for
> > > symbol plists.
> >
> > But the `category` text-property uses as text-properties the properties
> > of a symbol, so there is a real conflict in our current design.
>
> The 'category' symbols aren't faces, so there's no problem here.  the
> 'face' property has special meaning only on symbols that name a face.

Except when they are.  There is no rule in Emacs that says the same
symbol can't be used for multiple purposes. In fact, that is mostly the
Lisp way (functions, variables, fringe bitmaps, tests, etc, etc), at least
for the Lisps I'm familiar with.

So IMO it's not a trivial issue in neither theory or practice. Agreeing
with Stefan, if it's not too much trouble, I would like Emacs to not
violate this basic principle.

If it's not possible, then I think we must mention in the documentation
node about categories that 'face' is an obsolete alias for 'category-face'
(and add somewhere else that 'face', despite it's innocent-looking
name shouldn't be used at all).

João

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 30 Jan 2019 17:05:26 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> add somewhere else that 'face', despite it's innocent-looking
> name shouldn't be used at all

I did that already, two days ago.

Re: Fwd: Flymake and the 'face' property

[Stefan Monnier]

>> But the `category` text-property uses as text-properties the properties
>> of a symbol, so there is a real conflict in our current design.
> The 'category' symbols aren't faces,

They can be.  And even if they're not they may end up passed to `facep`.

> so there's no problem here.

I think you're minimizing the problem ;-)

> I think you greatly exaggerate the importance of this incident.

No I definitely do not claim it's a big deal.  But it points out a flaw
in a corner of our design, and it's pretty clear what is the right fix,
so I think it's worth evaluating how hard/easy it would be to fix
it right.

> So I see no reason to waste any more energy on this.

Please don't then.  Other people may be motivated by these kinds of
"unproductive" changes and I see no reason to discourage them.


        Stefan

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Wed, 30 Jan 2019 15:56:30 -0500
> 
> > So I see no reason to waste any more energy on this.
> 
> Please don't then.  Other people may be motivated by these kinds of
> "unproductive" changes and I see no reason to discourage them.

I don't want us to make changes for the sake of changes in a corner
issue that touches on the display code.  That's from POV a good reason
to discourage people from making such changes.

Re: Fwd: Flymake and the 'face' property

[Stefan Monnier]

> I don't want us to make changes for the sake of changes in a corner
> issue that touches on the display code.  That's from POV a good reason
> to discourage people from making such changes.

It should be a very cosmetic change, basically search&replace with the
only trick being in making sure we touch all places needed and
only them.  No structural change should be needed anywhere.
So, even if it can introduce breakage, it shouldn't make anything more
complex at all.

Software should be soft.  If we don't change it every once in a while,
it hardens,


        Stefan

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Sat, 02 Feb 2019 03:23:09 -0500
> 
> > I don't want us to make changes for the sake of changes in a corner
> > issue that touches on the display code.  That's from POV a good reason
> > to discourage people from making such changes.
> 
> It should be a very cosmetic change, basically search&replace with the
> only trick being in making sure we touch all places needed and
> only them.

I don't think I understand what replacement would you like to make.
Please elaborate.

Re: Fwd: Flymake and the 'face' property

[Stefan Monnier]

>> It should be a very cosmetic change, basically search&replace with the
>> only trick being in making sure we touch all places needed and
>> only them.
> I don't think I understand what replacement would you like to make.
> Please elaborate.

AFAIK we're more or less talking about replacing Qface with
Qinternal_face_id at those places where it is relevant.


        Stefan

Re: Fwd: Flymake and the 'face' property

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Sat, 02 Feb 2019 17:49:51 -0500
> 
> >> It should be a very cosmetic change, basically search&replace with the
> >> only trick being in making sure we touch all places needed and
> >> only them.
> > I don't think I understand what replacement would you like to make.
> > Please elaborate.
> 
> AFAIK we're more or less talking about replacing Qface with
> Qinternal_face_id at those places where it is relevant.

That'd be a backward-incompatible change.  We could _add_ the internal
property, though, keeping the 'face' one for now, then remove it in
the future.

Re: Fwd: Flymake and the 'face' property

[Stefan Monnier]

> That'd be a backward-incompatible change.

Yes.  But on an internal implementation detail which maybe noone depends
on.  That's why I said we should investigate its consequences in the
real world.

> We could _add_ the internal property, though, keeping the 'face' one
> for now, then remove it in the future.

I see we violently agree (I suggested the same some days ago).


        Stefan