Changing a cl-defstruct definition in a published package

Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

Hi all,

I'm sure this has already been discussed, but I couldn't find the relevant discussion.  I'm running into issues trying to update a package without breaking other packages that depend on it.  Advice would be very welcome.

I maintain package A, which contains this:

  (cl-defstruct aaa-info
    line message)
  
  (provide 'aaa)

Someone else wrote a package B that contains this:

  (require 'aaa)

  (defun bbb-print-message (info)
    (message (aaa-info-message info)))

  (provide 'bbb)

Users have installed both packages through package.el.  I update A by adding a new field to the definition of aaa-info, and push the update to ELPA or MELPA:

  (cl-defstruct aaa-info
    line column message)

Users update using package.el, but this does not cause b to be recompiled.  From this point on, any subsequent call to bbb-print-message to bbb-print-message fails: for example, (bbb-print-message (make-aaa-info :line 1 :column 2 :message "XYZ")) prints this:

  Debugger entered--Lisp error: (wrong-type-argument stringp 2)
    message(2)
    bbb-print-message(#s(aaa-info :line 1 :column 2 :message "XYZ"))

Similarly, if I update the constructor of aaa-info to give the new 'column' field a default value, instances created by the previously-compiled B will still be missing the 'column' slot, leading to all sorts of errors.

What is the recommended way to change a cl-defstruct definition without running into these issues?

Thanks!
Clément. 

Re: Changing a cl-defstruct definition in a published package

[João Távora]

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

On Thu, Jul 12, 2018, 21:13 Clément Pit-Claudel <cpitclaudel@gmail.com>
wrote:

> Hi all,
>
> I'm sure this has already been discussed, but I couldn't find the relevant
> discussion.  I'm running into issues trying to update a package without
> breaking other packages that depend on it.  Advice would be very welcome.
>
> I maintain package A, which contains this:
>
>   (cl-defstruct aaa-info
>     line message)
>
>   (provide 'aaa)
>
> Someone else wrote a package B that contains this:
>
>   (require 'aaa)
>
>   (defun bbb-print-message (info)
>     (message (aaa-info-message info)))
>
>   (provide 'bbb)
>
> Users have installed both packages through package.el.  I update A by
> adding a new field to the definition of aaa-info, and push the update to
> ELPA or MELPA:
>
>   (cl-defstruct aaa-info
>     line column message)
>
> Users update using package.el, but this does not cause b to be
> recompiled.  From this point on, any subsequent call to bbb-print-message
> to bbb-print-message fails: for example, (bbb-print-message (make-aaa-info
> :line 1 :column 2 :message "XYZ")) prints this:
>
>   Debugger entered--Lisp error: (wrong-type-argument stringp 2)
>     message(2)
>     bbb-print-message(#s(aaa-info :line 1 :column 2 :message "XYZ"))
>
> Similarly, if I update the constructor of aaa-info to give the new
> 'column' field a default value, instances created by the
> previously-compiled B will still be missing the 'column' slot, leading to
> all sorts of errors.
>
> What is the recommended way to change a cl-defstruct definition without
> running into these issues?
>
> Thanks!
> Clément.
>

Hi Clément,

There's no way to fix the problem now without breaking backward
compatibility because by now B's use of your accessor has been compiled
into something that probably looks like an aref into an array.  So if you
change your object layout in A, you break a compiled B.

The way to avoid this beforehand is not to expose the defstruct's accessors
as a public interface.  You can't think of them as functions, because they
have compiler macros associated.  The easiest "solution" now is to make
proper functions for those accessors that are public, and then having B
recompiled.  Also consider if you really need defstructs for their speed,
because they're usually an array in disguise.  Perhaps a (slightly, in most
cases) slower eieio class would do.

João

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

Re: Changing a cl-defstruct definition in a published package

[Basil L. Contovounesios]

João Távora <joaotavora@gmail.com> writes:

> On Thu, Jul 12, 2018, 21:13 Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:
>
>>  I maintain package A, which contains this:
>>
>>    (cl-defstruct aaa-info
>>      line message)
>>
>>    (provide 'aaa)
>>
>>  Someone else wrote a package B that contains this:
>>
>>    (require 'aaa)
>>
>>    (defun bbb-print-message (info)
>>      (message (aaa-info-message info)))
>>
>>    (provide 'bbb)
>>
>>  Users have installed both packages through package.el.  I update A
>>  by adding a new field to the definition of aaa-info, and push the
>>  update to ELPA or MELPA:
>>
>>    (cl-defstruct aaa-info
>>      line column message)
>>
>>  Users update using package.el, but this does not cause b to be
>>  recompiled.  From this point on, any subsequent call to
>>  bbb-print-message to bbb-print-message fails: for example,
>>  (bbb-print-message (make-aaa-info :line 1 :column 2 :message "XYZ"))
>>  prints this:
>>
>>    Debugger entered--Lisp error: (wrong-type-argument stringp 2)
>>      message(2)
>>      bbb-print-message(#s(aaa-info :line 1 :column 2 :message "XYZ"))
>>
>>  Similarly, if I update the constructor of aaa-info to give the new
>>  'column' field a default value, instances created by the
>>  previously-compiled B will still be missing the 'column' slot,
>>  leading to all sorts of errors.
>>
>>  What is the recommended way to change a cl-defstruct definition
>>  without running into these issues?
>
> There's no way to fix the problem now without breaking backward
> compatibility because by now B's use of your accessor has been
> compiled into something that probably looks like an aref into an
> array.  So if you change your object layout in A, you break a compiled
> B.
>
> The way to avoid this beforehand is not to expose the defstruct's
> accessors as a public interface.  You can't think of them as
> functions, because they have compiler macros associated.  The easiest
> "solution" now is to make proper functions for those accessors that
> are public, and then having B recompiled.  Also consider if you really
> need defstructs for their speed, because they're usually an array in
> disguise.  Perhaps a (slightly, in most cases) slower eieio class
> would do.

If cl-defstructs boil down to an array in disguise (I don't know because
I've never looked into them), would appending (as opposed to prepending
or inserting) new slots maintain backward compatibility?  E.g. going
from:

  (cl-defstruct aaa-info
    line message)

to:

  (cl-defstruct aaa-info
    line message column)

instead of:

  (cl-defstruct aaa-info
    line column message)

(despite my finding the latter aesthetically nicer, as it groups line
and column together).

-- 
Basil

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-13 13:38, Basil L. Contovounesios wrote:
> If cl-defstructs boil down to an array in disguise (I don't know because
> I've never looked into them), would appending (as opposed to prepending
> or inserting) new slots maintain backward compatibility?

I don't think so, unfortunately, because of the second issue I mentioned at the end of my email: constructors called on B's side have been compiled down to array initializations, which means that even if the new constructor for my defstruct has a default value set, B will still pass me arrays that are one element too short :/

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-13 13:01, João Távora wrote:
> There's no way to fix the problem now without breaking backward compatibility because by now B's use of your accessor has been compiled into something that probably looks like an aref into an array.

Indeed, I suspected as much…

> So if you change your object layout in A, you break a compiled B.

Right, but I can't afford that.  Do you have ideas on workarounds?
Some other source-based package managers recompile dependencies on update.  IIUC, package.el doesn't do that.  Maybe it should?  But it sounds like a lot of extra work.

What would you do? I don't think breaking all packages that depend on FlyCheck is a valid option :/

Clément.
 

Re: Changing a cl-defstruct definition in a published package

[João Távora]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 2018-07-13 13:01, João Távora wrote:
>> There's no way to fix the problem now without breaking backward
>> compatibility because by now B's use of your accessor has been
>> compiled into something that probably looks like an aref into an
>> array.
>
> Indeed, I suspected as much…
>
>> So if you change your object layout in A, you break a compiled B.
>
> Right, but I can't afford that.  Do you have ideas on workarounds?
> Some other source-based package managers recompile dependencies on
> update.  IIUC, package.el doesn't do that.  Maybe it should?  But it
> sounds like a lot of extra work.
>
> What would you do? I don't think breaking all packages that depend on
> FlyCheck is a valid option :/

You wouldn't be breaking the packages totally, you would be breaking the
compiled code.  You could argue that it's package.el's responsibility to
notice, knowing the dependency chain, that some things need to be
recompiled if a dependency changes upstream.  Like Make does, basically.
But arguing of course won't solve the immediate problem you have at
hand.

I don't know what you're trying to do, but the only way is not to change
the object layout, and to guarantee that the value that the user
packages are looking for is indeed found there at the right time.  A
potentially horrible amount of hookage awaits you, but you could at
least also add your new feature/change and get rid of the hacks later.

João

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-13 15:38, João Távora wrote:
> I don't know what you're trying to do, but the only way is not to change
> the object layout, and to guarantee that the value that the user
> packages are looking for is indeed found there at the right time.  A
> potentially horrible amount of hookage awaits you, but you could at
> least also add your new feature/change and get rid of the hacks later.

:(. I'm going to wait for a bit and hope that someone thinks of a clever workaround :)
Is there a way to tell package.el to recompile another package, maybe?

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

>> I don't know what you're trying to do, but the only way is not to change
>> the object layout, and to guarantee that the value that the user
>> packages are looking for is indeed found there at the right time.  A
>> potentially horrible amount of hookage awaits you, but you could at
>> least also add your new feature/change and get rid of the hacks later.
>
> :(. I'm going to wait for a bit and hope that someone thinks of
> a clever workaround :) Is there a way to tell package.el to recompile
> another package, maybe?

I don't think I could come up with a clever workaround without knowing
the details (because the clever workaround will almost invariably
take advantage of some aspect of those details).


        Stefan

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-13 16:40, Stefan Monnier wrote:
>>> I don't know what you're trying to do, but the only way is not to change
>>> the object layout, and to guarantee that the value that the user
>>> packages are looking for is indeed found there at the right time.  A
>>> potentially horrible amount of hookage awaits you, but you could at
>>> least also add your new feature/change and get rid of the hacks later.
>>
>> :(. I'm going to wait for a bit and hope that someone thinks of
>> a clever workaround :) Is there a way to tell package.el to recompile
>> another package, maybe?
> 
> I don't think I could come up with a clever workaround without knowing
> the details (because the clever workaround will almost invariably
> take advantage of some aspect of those details).

I'm happy to provide more details, then.  The currently problematic implementation is at https://github.com/flycheck/flycheck/pull/1400

Flycheck currently stores its error list as a list of error structs:

(cl-defstruct (flycheck-error
               (:constructor flycheck-error-new)
               (:constructor flycheck-error-new-at
                             (line column
                                   &optional level message
                                   &key checker id group
                                   (filename (buffer-file-name))
                                   (buffer (current-buffer)))))
  buffer checker filename line column message level id group)

We're hoping to change it to this:

  …
  buffer checker filename -coordinates -region message level id group)

`-coordinates' is another struct with 4 fields (line-start, column-start, line-end, and column-end). `-region' is a cons cell of two buffer positions.  The idea is that checker can supply either line/column coordinates or directly a region.

These two are prefixed with `-' because they are wrapped by accessors, to support either representation and lazily convert between them: if code needs a region it uses the wrapper for -region, which ensures that that field is populated, and conversely when code (e.g. the error list) needs a line number, it gets it through a wrapper that guarantees that the number has been computed.

Most Flycheck checkers are defined using a standardized macro and do not have to worry about creating or accessing individual error structures, so that code is fine.  More complex error checkers, on the other hand, do create error objects directly (concrete examples include merlin, or any of the checkers that maintain a persistent background process).

Let me know if there's any extra info I can provide.

Cheers,
Clément.

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

>   buffer checker filename line         column  message level id group)
[...]
>   buffer checker filename -coordinates -region message level id group)

OK, so the issue is with `line` and `column` which get replaced by
`-coordinates` and `-region`, the rest stays unchanged.  So maybe you
can arrange to auto-detect objects created with the old format.

> Most Flycheck checkers are defined using a standardized macro and do
> not have to worry about creating or accessing individual error
> structures, so that code is fine.  More complex error checkers, on the
> other hand, do create error objects directly (concrete examples
> include merlin, or any of the checkers that maintain a persistent
> background process).

IIUC, outside Flycheck the main operation called (and hence inlined) is
the constructor(s).  Are field accessors also used outside Flycheck?
If so, is there a chance that they are only ever used on those objects
that were created by the outside code as well (i.e. those inlined
accessors only see objects created by the compatible inline
constructors)?

I get the impression that maybe you can write a wrapper to access
`-coordinates` and `-region` which will check if either of those is of
the wrong type (but of the right type for `line` and `column` instead)
so it will know to fallback on the new code?


        Stefan

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-13 23:36, Stefan Monnier wrote:
>>   buffer checker filename line         column  message level id group)
> [...]
>>   buffer checker filename -coordinates -region message level id group)
> 
> OK, so the issue is with `line` and `column` which get replaced by
> `-coordinates` and `-region`, the rest stays unchanged.  So maybe you
> can arrange to auto-detect objects created with the old format.

Thanks Stefan, your help and advice are much appreciated.

>> Most Flycheck checkers are defined using a standardized macro and do
>> not have to worry about creating or accessing individual error
>> structures, so that code is fine.  More complex error checkers, on the
>> other hand, do create error objects directly (concrete examples
>> include merlin, or any of the checkers that maintain a persistent
>> background process).
> 
> IIUC, outside Flycheck the main operation called (and hence inlined) is
> the constructor(s).  Are field accessors also used outside Flycheck?
> If so, is there a chance that they are only ever used on those objects
> that were created by the outside code as well (i.e. those inlined
> accessors only see objects created by the compatible inline
> constructors)?

I don't think so.  We have multiple types of extensions, including error checkers (which create new objects and use accessors on them, then pass them to Flycheck core which also accesses fields on them) and UI plugins that take errors produced by the core or plugins and access their fields.

> I get the impression that maybe you can write a wrapper to access
> `-coordinates` and `-region` which will check if either of those is of
> the wrong type (but of the right type for `line` and `column` instead)
> so it will know to fallback on the new code?

This might be possible.  I will investigate and possibly follow up.
I wonder if there may be a trick to force recompilation of dependencies.  Maybe this is something package.el should support ? A field like Must-recompile-version: 0.1 would mean that when upgrading from anything before 0.1 to 0.1 or later package.el should recompile all dependencies, too.

Is there a setting to tell cl-defstruct to not generate macros for constructors and accessors?

Clément.

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

>> IIUC, outside Flycheck the main operation called (and hence inlined) is
>> the constructor(s).  Are field accessors also used outside Flycheck?
>> If so, is there a chance that they are only ever used on those objects
>> that were created by the outside code as well (i.e. those inlined
>> accessors only see objects created by the compatible inline
>> constructors)?
> I don't think so.  We have multiple types of extensions, including error
> checkers (which create new objects and use accessors on them, then pass them
> to Flycheck core which also accesses fields on them) and UI plugins that
> take errors produced by the core or plugins and access their fields.

Hmm... then it's going to be difficult: old UI plugins will see objects
created by the new core, and there's no much we can do at that point,
I think.

> This might be possible.  I will investigate and possibly follow up.
> I wonder if there may be a trick to force recompilation of dependencies.
> Maybe this is something package.el should support ?

There's no doubt that package.el should support recompilation.
Patches welcome for that (first to add some code which does the actual
recompilation, and then maybe some way to trigger such recompilation of
dependencies).

> Is there a setting to tell cl-defstruct to not generate macros for
> constructors and accessors?

Not currently, no.  But João would also like to have such a thing.

Note that the recompilation issue can be solved outside of package.el:
- "recompile package" doesn't have to be in package.el (tho it is its
  most natural place).
- you could have code in flycheck.el that does:

    (eval-when-compile
      (dolist (pkg (find-pkgs-using-the-old-flycheck-object-layout))
        (recompile-package pkg)))
  
  so it will be executed when flycheck is compiled.


-- Stefan

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-15 09:11, Stefan Monnier wrote:
> Hmm... then it's going to be difficult: old UI plugins will see objects
> created by the new core, and there's no much we can do at that point,
> I think.

:'( good point

>> Is there a setting to tell cl-defstruct to not generate macros for
>> constructors and accessors?
> 
> Not currently, no.  But João would also like to have such a thing.

I see. That would be very neat.

> Note that the recompilation issue can be solved outside of package.el:
> - "recompile package" doesn't have to be in package.el (tho it is its
>   most natural place).
> - you could have code in flycheck.el that does:
> 
>     (eval-when-compile
>       (dolist (pkg (find-pkgs-using-the-old-flycheck-object-layout))
>         (recompile-package pkg)))
>   
>   so it will be executed when flycheck is compiled.

That's a interesting idea, and a good point.
But wouldn't it have to run *after* Flycheck is compiled? (Can you call the byte-compiler from code that's currently being run by the byte-compiler?)

Thanks again for your help,
Clément.

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

>> Note that the recompilation issue can be solved outside of package.el:
>> - "recompile package" doesn't have to be in package.el (tho it is its
>>   most natural place).
>> - you could have code in flycheck.el that does:
>>
>>     (eval-when-compile
>>       (dolist (pkg (find-pkgs-using-the-old-flycheck-object-layout))
>>         (recompile-package pkg)))
>>
>>   so it will be executed when flycheck is compiled.
>
> That's a interesting idea, and a good point.
> But wouldn't it have to run *after* Flycheck is compiled?

I don't think so, no (by the time we're compiling the file, the package
is already installed and activated).

You'd need to add some mechanism to avoid infinite-recursion, ideally by
somehow checking that we're running this eval-when-compile because of
a compilation and not because we're loading the .el file.

We could use an ugly hack like (guaranteed 100% untested):

     (eval-when-compile
       ;; When handling eval-when-compile, the byte-compiler compiles
       ;; the form before evaluating it, so in that case the
       ;; (lambda (x) (+ x 1)) below will be turned into byte-code before
       ;; we get to the `if` test.  In contrast, when we're just loading
       ;; the .el file, the `lambda` is either left as is or turned into
       ;; a (closure ...) object (depending on lexical-binding), but in
       ;; either case it won't be turned into a byte-code.
       ;; Of course, all of this can be subject to change :-(
       (when (byte-code-function-p (lambda (x) (+ x 1)))
         ;; We're in the middle of byte-compiling this file.
         (dolist (pkg (find-pkgs-using-the-old-flycheck-object-layout))
           (recompile-package pkg))))

Better would be to introduce some dedicated feature, like
`eval-only-when-compile`.


        Stefan

Re: Changing a cl-defstruct definition in a published package

[Jake]

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

I've seen another work around to detect whether or not a file is being
byte-compiled in use-package
<https://github.com/jwiegley/use-package/blob/master/use-package-ensure.el#L192-L197>:
(bound-and-true-p byte-compile-current-file).

It seems to work, as illustrated by this example:

(message
 (eval-when-compile
   (if (bound-and-true-p byte-compile-current-file)
       "I'm being byte-compiled!"
     "I'm being evaluated :(")))

When I byte compile that and then evaluate

(progn
  (load-file "~/byte-compile-test.elc")
  (load-file "~/byte-compile-test.el"))

It prints out

Loading /Users/jake.waksbaum/byte-compile-test.elc...
I’m being byte-compiled!
Loading /Users/jake.waksbaum/byte-compile-test.elc...done
Loading /Users/jake.waksbaum/byte-compile-test.el (source)...
I’m being evaluated :(
Loading /Users/jake.waksbaum/byte-compile-test.el (source)...done
t

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

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

> (message
>  (eval-when-compile
>    (if (bound-and-true-p byte-compile-current-file)
>        "I'm being byte-compiled!"
>      "I'm being evaluated :(")))

Let's say, this is in a file foo.el.  And let's say we have a file
bar.el which contains:

   (require 'foo)
   ...

Then byte-compiling bar.el (when foo.el has not been byte-compiled)
will emit a message "I'm being byte-compiled!".

But yes, there are other ways.  I think I remember using something like

    (setq my-witness t)

    (eval-when-compile
      (message (if (bound-and-true-p my-witness)
                   "Loading this file non-compiled"
                 "Byte-compiling this file")))

    (setq my-witness nil)

in the past,


        Stefan

Re: Changing a cl-defstruct definition in a published package

[Jake]

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

Oh I see. I think that I've actually misunderstood the use-package code and
that's precisely the behavior they're going for: they want to install the
package immediately not when use-package is being byte-compiled, but when
it is being used from any file that is being byte-compiled. So it works for
them, but it's not the behavior we want here.

On Thu, Jul 19, 2018 at 5:11 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > (message
> >  (eval-when-compile
> >    (if (bound-and-true-p byte-compile-current-file)
> >        "I'm being byte-compiled!"
> >      "I'm being evaluated :(")))
>
> Let's say, this is in a file foo.el.  And let's say we have a file
> bar.el which contains:
>
>    (require 'foo)
>    ...
>
> Then byte-compiling bar.el (when foo.el has not been byte-compiled)
> will emit a message "I'm being byte-compiled!".
>
> But yes, there are other ways.  I think I remember using something like
>
>     (setq my-witness t)
>
>     (eval-when-compile
>       (message (if (bound-and-true-p my-witness)
>                    "Loading this file non-compiled"
>                  "Byte-compiling this file")))
>
>     (setq my-witness nil)
>
> in the past,
>
>
>         Stefan
>
>
>

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

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-15 21:51, Stefan Monnier wrote:
> I don't think so, no (by the time we're compiling the file, the package
> is already installed and activated).

Great, thanks :)

I've made a bit of progress on this.  It's indeed fairly easy to force recompilation of all packages that Flycheck depends on:

(let ((dependencies nil))
  (dolist (name-descs package-alist)
    (let ((reqs (package-desc-reqs (cadr name-descs))))
      (when (assq 'flycheck reqs)
        (push desc dependencies))))
  (message "Flycheck: forcing recompilation of the following packages: %s"
           (mapconcat (lambda (d) (symbol-name (package-desc-name d)))
                      dependencies ", "))
  (dolist (desc (nreverse dependencies))
    ;; The following fragment was taken from `package-unpack':
    ;; -------------------------------------------------------
    ;; Activation has to be done before compilation, so that if we're
    ;; upgrading and macros have changed we load the new definitions
    ;; before compiling.
    (when (package-activate-1 desc :reload :deps)
      ;; FIXME: Compilation should be done as a separate, optional, step.
      ;; E.g. for multi-package installs, we should first install all packages
      ;; and then compile them.
      (package--compile desc)
      ;; After compilation, load again any files loaded by
      ;; `activate-1', so that we use the byte-compiled definitions.
      (package--load-files-for-activation desc :reload))))

I'm now looking for a way to make sure this only happens once — that is, that not all future Flycheck updates cause the recompilation, only the first one introducing the breaking change.

Is there a way to know during byte-compilation which version of the package was previously installed?

Thanks!
Clément.

Re: Changing a cl-defstruct definition in a published package

[Stefan Monnier]

> Is there a way to know during byte-compilation which version of the
> package was previously installed?

I don't think you can get this info directly, no :-(


        Stefan

Re: Changing a cl-defstruct definition in a published package

[Clément Pit-Claudel]

On 2018-07-30 18:49, Stefan Monnier wrote:
>> Is there a way to know during byte-compilation which version of the
>> package was previously installed?
> 
> I don't think you can get this info directly, no :-(

Ah, snap.