[PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Philipp Stephani]

---
 etc/NEWS | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 64b53d88c8..7cedd28ed9 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1462,6 +1462,20 @@ them through 'format' first.  Even that is discouraged: for ElDoc
 support, you should set 'eldoc-documentation-function' instead of
 calling 'eldoc-message' directly.
 
+** The introduction of record types (see below) brings several
+incompatibilities:
+
+*** The return value of 'type-of' is no longer a reliable indicator
+for the type of an object.  For example, a return value of 'integer'
+now means that the object in question is either an integer or a record
+named 'integer'.  To reliably test the type of an object, use the
+predicate functions such as 'integerp'.  Don't define record types
+whose names clash with primitive type names.
+
+*** The printed representation of a record named 'hash-table' now
+clashes with the printed representation of a hash table.  Don't define
+record types named 'hash-table'.
+
 \f
 * Lisp Changes in Emacs 26.1
 
-- 
2.15.1

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Clément Pit-Claudel]

On 2017-12-11 16:37, Philipp Stephani wrote:
> +*** … Don't define record types whose names clash with primitive type names.
> +*** … Don't define record types named 'hash-table'.

Could we just reserve these names and make it an error to define such records?
Or, could we make type-of return `record-foo` instead of `foo`?

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> +** The introduction of record types (see below) brings several
> +incompatibilities:
> +*** The return value of 'type-of' is no longer a reliable indicator
> +for the type of an object.  For example, a return value of 'integer'
> +now means that the object in question is either an integer or a record
> +named 'integer'.

No: if someone makes a record of type `integer` he's introduced a bug.

> Don't define record types
> +whose names clash with primitive type names.

Rather than "Don't" I'd say "it's a bug to ...".

> +*** The printed representation of a record named 'hash-table' now
> +clashes with the printed representation of a hash table.  Don't define
> +record types named 'hash-table'.

This is subsumed by the previous "it's a bug to ...".

Clement said:
> Could we just reserve these names and make it an error to define such records?
> Or, could we make type-of return `record-foo` instead of `foo`?

It *is* an error.  Just not one we bother to try and detect because it'd
imply making everyone pay all the time for the rare idiots who'll try to
create such a thing.  There are plenty of ways for a user to shoot
himself in the foot, and we don't bother checking all of them, by a long
stretch: we only check those that have very serious consequences, or
that occur often, or that can be checked at no cost (e.g. it'd be fine to
make `cl-defstruct` reject names like `integer` or `hash-table` since
that's dirt cheap, but it's not a serious enough problem to justify
slowing down `make-record`).


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Clément Pit-Claudel]

On 2017-12-11 22:21, Stefan Monnier wrote:
> e.g. it'd be fine to
> make `cl-defstruct` reject names like `integer` or `hash-table`

That's what I'm suggesting.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> e.g. it'd be fine to
>> make `cl-defstruct` reject names like `integer` or `hash-table`
> That's what I'm suggesting.

That sounds good (can be added either to cl-defstruct or to
cl-struct-define or both and could use the list of builtin types that's
in cl--generic-typeof-types).

Moving forward, I'd expect that "types" will get a more first-class
presence in Elisp, so you should be able to check if a given type name
is already used.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > +*** The return value of 'type-of' is no longer a reliable indicator
  > +for the type of an object.  For example, a return value of 'integer'
  > +now means that the object in question is either an integer or a record
  > +named 'integer'.  To reliably test the type of an object, use the
  > +predicate functions such as 'integerp'.  Don't define record types
  > +whose names clash with primitive type names.
  > +
  > +*** The printed representation of a record named 'hash-table' now
  > +clashes with the printed representation of a hash table.  Don't define
  > +record types named 'hash-table'.

This is very bad behavior.

It would be much less bad to signal an error if anyone tries to define
record types with these names.

How many such names are there?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Or, could we make type-of return `record-foo` instead of `foo`?

  > It *is* an error.  Just not one we bother to try and detect because it'd
  > imply making everyone pay all the time for the rare idiots who'll try to
  > create such a thing.

If programmers are not supposed to do it, let's detect it and report
an error when they try.  That way, they will know it's an error as
soon as they try, rather than needing to debug it to find out it's an error.



-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> If programmers are not supposed to do it, let's detect it and report
> an error when they try.  That way, they will know it's an error as
> soon as they try, rather than needing to debug it to find out it's an error.

When was the last time you saw a user try to redefine the
"integer" type?
[ I'll be happy to hear about cases in other languages than Elisp,
  since otherwise it's going to be depressingly silent I'm afraid.  ]


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> If programmers are not supposed to do it, let's detect it and report
> an error when they try.  That way, they will know it's an error as
> soon as they try, rather than needing to debug it to find out it's an error.

Why haven't we applied that same rule to (fset 'apply ...)?
How about (use-global-map (make-keymap))?


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Noam Postavsky]

On Tue, Dec 12, 2017 at 8:03 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> If programmers are not supposed to do it, let's detect it and report
>> an error when they try.  That way, they will know it's an error as
>> soon as they try, rather than needing to debug it to find out it's an error.
>
> Why haven't we applied that same rule to (fset 'apply ...)?
> How about (use-global-map (make-keymap))?

I thought you already agreed we should make "define an 'integer'
struct" an error, why are we still arguing about this?

https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00400.html

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> I thought you already agreed we should make "define an 'integer'
> struct" an error, why are we still arguing about this?

I agreed that it's OK to introduce such a check but that doesn't mean
I think we *should* have such a check.

I'm just really puzzled by all the strong reactions for a problem that's
so% hypothetical, and really very similar to lots of other "risks" that
we don't care about at all.

Maybe it's just because it's brand new, whereas the other risks have
been with us for so many years that we completely forgot about them?


        Stefan


PS: By the way, feel free to try to (cl-defstruct integer ...) and play
with it.  From what I can tell, it takes dedication to go from there to
actually experiencing an undesirable outcome.
If you want to shoot yourself in the foot

    (add-hook 'post-command-hook #'beginning-of-buffer)

is a much safer choice (and as bonus, it doesn't require cl-lib),

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Sam Steingold]

> * Stefan Monnier <zbaavre@veb.hzbagerny.pn> [2017-12-12 17:20:29 -0500]:
>
>> If programmers are not supposed to do it, let's detect it and report
>> an error when they try.  That way, they will know it's an error as
>> soon as they try, rather than needing to debug it to find out it's an error.
>
> When was the last time you saw a user try to redefine the
> "integer" type?
> [ I'll be happy to hear about cases in other languages than Elisp,
>   since otherwise it's going to be depressingly silent I'm afraid.  ]

What's so special about "integer"?
How many other built-in types does Emacs Lisp have?
Can you list them all by heart? Can everyone?
What about many different packages which may redefine each other's
types?  Especially since Emacs Lisp does not have the Common Lisp-style
package system, and it does not enforce the prefix conventions.

It seems to me that warning on redefining a type (or defining a type
with a name which already names a type) is a very good idea.
Emacs Lisp already warns on redefining functions with a different
signature.  Why not warn about types?

Thanks.

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net https://jihadwatch.org
http://camera.org http://memri.org http://www.memritv.org
At war time "salt of the earth" becomes "cannon fodder".

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>>> If programmers are not supposed to do it, let's detect it and report
>>> an error when they try.  That way, they will know it's an error as
>>> soon as they try, rather than needing to debug it to find out it's an error.
>> When was the last time you saw a user try to redefine the
>> "integer" type?
>> [ I'll be happy to hear about cases in other languages than Elisp,
>> since otherwise it's going to be depressingly silent I'm afraid.  ]
> What's so special about "integer"?

Oh, nothing, feel free to provide examples for other built-in types.

> How many other built-in types does Emacs Lisp have?

(length cl--generic-typeof-types) suggests 35.

> Can you list them all by heart? Can everyone?

Just because you can't name them doesn't mean you're likely to
accidentally use one of them by accident.

> What about many different packages which may redefine each other's
> types?

Thank you for this example: this problem has been with us since Emacs-19
got defstruct.  So what you all been doing all these years not trying to
solve this problem?

> Especially since Emacs Lisp does not have the Common Lisp-style
> package system, and it does not enforce the prefix conventions.

It's not enforced, but it's followed reasonably well these days.

> It seems to me that warning on redefining a type (or defining a type
> with a name which already names a type) is a very good idea.

As I said, I'm not opposed to it.  But I don't think the problem
deserves such a large blurb in NEWS about some very serious new problem
introduced by `make-record`.

> Emacs Lisp already warns on redefining functions with a different
> signature.

There's warning and there's warning.  Elisp doesn't warn when you
redefine with fset/defalias.  It only warns when you *compile* the code
that does.

> Why not warn about types?

As long as it's done at compile-time or equivalent (e.g. not in
`make-record`), I don't object (but given how rarely defclass/defstruct
is used I don't see a strong need for it either).


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stephen Leake]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> If you want to shoot yourself in the foot
>
>     (add-hook 'post-command-hook #'beginning-of-buffer)

Can I get that on a bumper sticker?


-- 
-- Stephe

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Does 'make-record' verify that the symbol is defined as a
record type?  If so, then if attempts to define a record type
called 'integer' or 'hash-table' give an error and don't define it,
the pitfall will be closed off.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Moving forward, I'd expect that "types" will get a more first-class
  > presence in Elisp, so you should be able to check if a given type name
  > is already used.

If type names are global, that won't be enough to avoid conflicts.
You must avoid conflicts with all the Lisp packages that _might_ get
loaded.

It's just like the situation with function definitions and global variables.

That means you should use your package's naming convention for record
types, just as for functions and global variables, to avoid conflicts
with other packages that you don't know about.

We should document this in the Emacs Lisp Reference Manual.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I'm just really puzzled by all the strong reactions for a problem that's
  > so% hypothetical, 

It is not hypothetical.  I am sure people will define such types in
their code, and get screwed.

		      and really very similar to lots of other "risks" that
  > we don't care about at all.

The similarity is only at an abstract level.

People understand that redefining primitive functions will break
things.

Redefining 'apply' in a careless way will break things very fast.  I
just tried (fset 'apply 'list), and then kill and yank did not work.
There is no danger people will develop Lisp packages which do this
and release them believing they work correctly.

If you define 'integer' as a record type, it will break some things,
but you might not notice them.  That makes it a more dangerous trap.
You could release a Lisp package which does this, not realizing that
it will cause problems for other users.

Since we agree that defining these symbols as record types should not
be done, let's make it easier for programmers to avoid doing that.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > When was the last time you saw a user try to redefine the
  > "integer" type?

Since this is a new feature, we have no evidence about how often that
will happen.

But that is a secondary question.  It is better to help people catch
errors early, and it doesn't cause any harm, so let's do it.

Just because there are many things one could do in Emacs that would
screw things up is not an argument for making more.  The most it argues
for is that we need not bend over backwards to avoid more.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> Does 'make-record' verify that the symbol is defined as a
> record type?

No and I vehemently don't want it to, just like `fset` makes no attempt
to check whether it's safe or not.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> Moving forward, I'd expect that "types" will get a more first-class
>> presence in Elisp, so you should be able to check if a given type name
>> is already used.
> If type names are global, that won't be enough to avoid conflicts.
> You must avoid conflicts with all the Lisp packages that _might_ get
> loaded.
> It's just like the situation with function definitions and global variables.

Exactly, and this has been with us for years and years.  This is not
a new problem, has standard solutions, and people already use those solutions.

> That means you should use your package's naming convention for record
> types, just as for functions and global variables, to avoid conflicts
> with other packages that you don't know about.

And that's already what has been done all these years.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> I'm just really puzzled by all the strong reactions for a problem that's
>> so% hypothetical, 
> It is not hypothetical.  I am sure people will define such types in
> their code, and get screwed.

[ Just because you're convinced it will happen doesn't make it less
  hypothetical.  AFAICT it has never happened so far.  ]
The 20 years of experience with cl.el's defstruct, plus the 10 years of
experience with eieio's defclass seem to disagree.

> Since we agree that defining these symbols as record types should not
> be done, let's make it easier for programmers to avoid doing that.

I won't get in the way of someone else doing that, *as long as* it's
done via compilation-time (or even load-time) checks rather than
run-time checks (i.e. not in make-record).


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> When was the last time you saw a user try to redefine the
>> "integer" type?
> Since this is a new feature, we have no evidence about how often that
> will happen.

No, it's not a new feature.  We've had defstruct and defclass for many
many years.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > That means you should use your package's naming convention for record
  > > types, just as for functions and global variables, to avoid conflicts
  > > with other packages that you don't know about.

  > And that's already what has been done all these years.

I can't follow you.  Records are a new feature.  But never mind that.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The 20 years of experience with cl.el's defstruct, plus the 10 years of
  > experience with eieio's defclass seem to disagree.

That is not a conclusive argument, just a sketch.  Please fill in the
gaps so we can see how valid that argument is.

The issue at hand is that defining a record called 'hashtable' or 'integer'
can alter how built-in types print.

If someone uses 'hashtable' or 'integer' as the name in cl's defining
or eieio's defclass, would that alter how built-in types print?  If not,
then they are not pertinent to the issue.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Since we agree that defining these symbols as record types should not
  > > be done, let's make it easier for programmers to avoid doing that.

  > I won't get in the way of someone else doing that, *as long as* it's
  > done via compilation-time (or even load-time) checks rather than
  > run-time checks (i.e. not in make-record).

We should do what is necessary to solve the problem.  Let's see what
is needed to prevent the problem.

The issue at hand is that defining a record called 'hashtable' or 'integer'
can alter how built-in types print.  That is a pitfall and we should put a
fence around it.

What are the concrete actions that would have this result?

Would using 'hashtable' or 'integer' in make-record cause that to occur?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Does the Emacs Lisp Manual say that that programs should use their
naming conventions for record type names?  If not, would someone please
add that, and ack?

This should go in the NEWS entry too.  If it isn't there, would
someone please add that too?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> > That means you should use your package's naming convention for record
>> > types, just as for functions and global variables, to avoid conflicts
>> > with other packages that you don't know about.
>> And that's already what has been done all these years.
> I can't follow you.  Records are a new feature.

The problem of global names for defstruct types has been with us since
Emacs-19.  The fact that defstruct now uses the new `records` system
instead of vectors doesn't make much difference in this respect.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The problem of global names for defstruct types has been with us since
  > Emacs-19.

The effect on printing makes it worse.  If one package defined a structure
type 'hast-table', even though that violated the conventions, that would
not cause an actual problem unless a second package defined a different
structure type with the same name.

Now, with the record types, a single package defining this type WILL
cause some problems, though they may not be visible for all users.

The constructs that define record types need to check for and reject
the names of built-in types.

Would someone please implement this for master and Emacs 26?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Philipp Stephani]

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

Clément Pit-Claudel <cpitclaudel@gmail.com> schrieb am Mo., 11. Dez. 2017
um 23:26 Uhr:

> On 2017-12-11 16:37, Philipp Stephani wrote:
> > +*** … Don't define record types whose names clash with primitive type
> names.
> > +*** … Don't define record types named 'hash-table'.
>
> Could we just reserve these names and make it an error to define such
> records?
> Or, could we make type-of return `record-foo` instead of `foo`?
>
>
Yes, I would strongly prefer either of those.
(Though I'd prefer something like '(record foo) or a custom struct to make
parsing a bit easier, but that's a nit.)

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

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 407 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Di., 12. Dez. 2017 um
21:50 Uhr:

> >> e.g. it'd be fine to
> >> make `cl-defstruct` reject names like `integer` or `hash-table`
> > That's what I'm suggesting.
>
> That sounds good (can be added either to cl-defstruct or to
> cl-struct-define or both and could use the list of builtin types that's
> in cl--generic-typeof-types).
>

OK, here's a patch.

[-- Attachment #1.2: Type: text/html, Size: 741 bytes --]

[-- Attachment #2: 0001-Prevent-name-clashes-between-CL-structures-and-builtin.txt --]
[-- Type: text/plain, Size: 5785 bytes --]

From 96befa75a7d14e46186df6b9ce5e553e0d31d647 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 21 Dec 2017 18:25:49 +0100
Subject: [PATCH] Prevent name clashes between CL structures and builtin types

* lisp/emacs-lisp/cl-preloaded.el (cl-struct-define): Don't allow
structures with the same names as builtin types.
(cl--struct-name-p): New helper function.

* lisp/emacs-lisp/cl-macs.el (cl-defstruct): Don't allow structures
with the same names as builtin types.

* test/lisp/emacs-lisp/cl-macs-tests.el (cl-defstruct/builtin-type):
* test/lisp/emacs-lisp/cl-preloaded-tests.el
(cl-struct-define/builtin-type): New unit tests.

* etc/NEWS: Document changed behavior.
---
 etc/NEWS                                   |  4 ++++
 lisp/emacs-lisp/cl-macs.el                 |  4 ++++
 lisp/emacs-lisp/cl-preloaded.el            |  8 ++++++++
 test/lisp/emacs-lisp/cl-macs-tests.el      |  9 ++++++++
 test/lisp/emacs-lisp/cl-preloaded-tests.el | 33 ++++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+)
 create mode 100644 test/lisp/emacs-lisp/cl-preloaded-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index 1ab1930ea7..1959e044e3 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -175,6 +175,10 @@ calling 'eldoc-message' directly.
 ** Old-style backquotes now generate an error.  They have been
 generating warnings for a decade.
 
+** Defining a Common Lisp structure using 'cl-defstruct' or
+'cl-struct-define' whose name clashes with a builtin type (e.g.,
+'integer' or 'hash-table') now signals an error.
+
 \f
 * Lisp Changes in Emacs 27.1
 
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index f5311041cc..4b9085afc0 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -43,6 +43,7 @@
 
 ;;; Code:
 
+(require 'cl-generic)
 (require 'cl-lib)
 (require 'macroexp)
 ;; `gv' is required here because cl-macs can be loaded before loaddefs.el.
@@ -2663,6 +2664,9 @@ cl-defstruct
 	 (forms nil)
          (docstring (if (stringp (car descs)) (pop descs)))
 	 pred-form pred-check)
+    ;; Can't use `cl-check-type' yet.
+    (unless (cl--struct-name-p name)
+      (signal 'wrong-type-argument (list 'cl-struct-name-p name 'name)))
     (setq descs (cons '(cl-tag-slot)
 		      (mapcar (function (lambda (x) (if (consp x) x (list x))))
 			      descs)))
diff --git a/lisp/emacs-lisp/cl-preloaded.el b/lisp/emacs-lisp/cl-preloaded.el
index e550f5a095..1f72d1e47e 100644
--- a/lisp/emacs-lisp/cl-preloaded.el
+++ b/lisp/emacs-lisp/cl-preloaded.el
@@ -36,6 +36,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl-generic))
 (eval-when-compile (require 'cl-lib))
 (eval-when-compile (require 'cl-macs))  ;For cl--struct-class.
 
@@ -50,6 +51,12 @@ cl--assertion-failed
         (apply #'error string (append sargs args))
       (signal 'cl-assertion-failed `(,form ,@sargs)))))
 
+(defun cl--struct-name-p (name)
+  "Return t if NAME is a valid structure name for `cl-defstruct'."
+  (and name (symbolp name) (not (keywordp name))
+       (not (memq name (eval-when-compile cl--generic-all-builtin-types)))
+       t))
+
 ;; When we load this (compiled) file during pre-loading, the cl--struct-class
 ;; code below will need to access the `cl-struct' info, since it's considered
 ;; already as its parent (because `cl-struct' was defined while the file was
@@ -110,6 +117,7 @@ cl--struct-register-child
 ;;;###autoload
 (defun cl-struct-define (name docstring parent type named slots children-sym
                               tag print)
+  (cl-check-type name cl--struct-name)
   (unless type
     ;; Legacy defstruct, using tagged vectors.  Enable backward compatibility.
     (cl-old-struct-compat-mode 1))
diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el
index 575f170af6..8aac25123f 100644
--- a/test/lisp/emacs-lisp/cl-macs-tests.el
+++ b/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -497,4 +497,13 @@
                           vconcat (vector (1+ x)))
                  [2 3 4 5 6])))
 
+\f
+(ert-deftest cl-defstruct/builtin-type ()
+  (should-error
+   (macroexpand '(cl-defstruct hash-table))
+   :type 'wrong-type-argument)
+  (should-error
+   (macroexpand '(cl-defstruct (hash-table (:predicate hash-table-p))))
+   :type 'wrong-type-argument))
+
 ;;; cl-macs-tests.el ends here
diff --git a/test/lisp/emacs-lisp/cl-preloaded-tests.el b/test/lisp/emacs-lisp/cl-preloaded-tests.el
new file mode 100644
index 0000000000..008a6e629f
--- /dev/null
+++ b/test/lisp/emacs-lisp/cl-preloaded-tests.el
@@ -0,0 +1,33 @@
+;;; cl-preloaded-tests.el --- unit tests for cl-preloaded.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+;; Author: Philipp Stephani <phst@google.com>
+
+;; This file is part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Unit tests for lisp/emacs-lisp/cl-preloaded.el.
+
+;;; Code:
+
+(ert-deftest cl-struct-define/builtin-type ()
+  (should-error
+   (cl-struct-define 'hash-table nil nil 'record nil nil
+                     'cl-preloaded-tests-tag 'cl-preloaded-tests nil)
+   :type 'wrong-type-argument))
+
+;;; cl-preloaded-tests.el ends here
-- 
2.15.1

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Philipp Stephani]

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

Noam Postavsky <npostavs@users.sourceforge.net> schrieb am Mi., 13. Dez.
2017 um 02:13 Uhr:

> On Tue, Dec 12, 2017 at 8:03 PM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
> >> If programmers are not supposed to do it, let's detect it and report
> >> an error when they try.  That way, they will know it's an error as
> >> soon as they try, rather than needing to debug it to find out it's an
> error.
> >
> > Why haven't we applied that same rule to (fset 'apply ...)?
> > How about (use-global-map (make-keymap))?
>
> I thought you already agreed we should make "define an 'integer'
> struct" an error, why are we still arguing about this?
>
> https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00400.html
>
>
Even after changing defstruct, you can still use make-record to create the
behavior described here:

(let ((o (make-record 'integer 0 nil)))
  (type-of o))
integer

(let ((o (make-record 'hash-table 0 nil)))
  (prin1-to-string o))
"#s(hash-table)"

My suggestion would be to either at least document this breaking change
(this patch), or forbid such records, or change the return value in a
non-breaking way (such as using '(record integer) and "#r(integer)").

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

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

> My suggestion would be to either at least document this breaking change
> (this patch), or forbid such records,

Hello?  We've been through this discussion enough times, I believe.
I'm absolutely firmly completely totally opposed to such a check in
`make-record`.

Yes, a record of type `integer` or `hash-table` is not allowed.
No, we don't want to enforce this with a check in `make-record`.

> or change the return value in a non-breaking way (such as using
> '(record integer) and "#r(integer)").

Other parts of Emacs rely on the fact that (eq (type-of X) (type-of X)),
so if you want to return cons cells, you'll need to hash-cons them or
something.  And the use of `integer` as record type is incorrect in any
case, so it would add complexity only to handle a situation that's
already incorrect.

You're making a mountain of a mole-hill, really.


        Stefan

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Thu, 14 Dec 2017 17:17:28 -0500
> 
> Does the Emacs Lisp Manual say that that programs should use their
> naming conventions for record type names?  If not, would someone please
> add that, and ack?

Ack.

> This should go in the NEWS entry too.  If it isn't there, would
> someone please add that too?

Done.

Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Hello?  We've been through this discussion enough times, I believe.
  > I'm absolutely firmly completely totally opposed to such a check in
  > `make-record`.

The change I want is in the constructs that define record types,
which I think is not 'make-record'.

Does `make-record' check that the specified record type is defined?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

[SUSPECTED SPAM] Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Stefan Monnier]

>> Hello?  We've been through this discussion enough times, I believe.
>> I'm absolutely firmly completely totally opposed to such a check in
>> `make-record`.
> The change I want is in the constructs that define record types,
> which I think is not 'make-record'.

Such a check is in a patch posted earlier.

> Does `make-record' check that the specified record type is defined?

No, `make-record` doesn't even really know what is a type.


        Stefan

Re: [SUSPECTED SPAM] Re: [PATCH] * etc/NEWS: Document incompatibilities introduced by record types.

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > The change I want is in the constructs that define record types,
  > > which I think is not 'make-record'.

  > Such a check is in a patch posted earlier.

That's good.  Has that patch been installed?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

make-record

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Does `make-record' check that the specified record type is defined?

  > No, `make-record` doesn't even really know what is a type.

Not checking the type name for validity is asking for trouble.

Perhaps some programs make a lot of records and need the extra speed
of a low-level primitive that doesn't check the validity.  But surely
most programs that use records won't spend much of their time creating
records.  In those programs, a function that does validate the type
would cause no visible slowdown.

So how about making 'make-record' validate the type,
and having another function 'make-record-fast' which doesn't validate?
I think this gets the best of both worlds.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: make-record

[Stefan Monnier]

> So how about making 'make-record' validate the type,
> and having another function 'make-record-fast' which doesn't validate?
> I think this gets the best of both worlds.

No program uses directly `make-record` (or `record` for that matter).

They are used internally by cl-lib and eieio to implement cl-defstruct
and eieio's defclass.  So, `make-record` and `record` should be of the
"fast" kind, since the extra check can be in defclass and cl-defstruct
(where it's very cheap anyway).


        Stefan

Re: make-record

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > They are used internally by cl-lib and eieio to implement cl-defstruct
  > and eieio's defclass.  So, `make-record` and `record` should be of the
  > "fast" kind, since the extra check can be in defclass and cl-defstruct
  > (where it's very cheap anyway).

Maybe you're right.  Do cl-lib and eieio take care only to create
records for types they have defined?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

Re: make-record

[Stefan Monnier]

> Maybe you're right.  Do cl-lib and eieio take care only to create
> records for types they have defined?

Yes.


        Stefan

Re: make-record

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Maybe you're right.  Do cl-lib and eieio take care only to create
  > > records for types they have defined?

  > Yes.

This means the problem I was concerned about doesn't exist.
That's a relief.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.