GNU bug report logs - #65852
30.0.50; image-auto-resize :type has choices in wrong order

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65852 in the body.
You can then email your comments to 65852 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; image-auto-resize :type has choices in wrong order
Date: Sun, 10 Sep 2023 10:40:45 -0300

The :type for image-auto-resize looks like this:

:type '(choice (const :tag "No resizing" nil)
               (const :tag "Fit to window" fit-window)
               (other :tag "Scale down to fit window" t)
               (number :tag "Scale factor" 1))

The `other' choice should come last.  Otherwise Custom thinks that
that's the selected option, even when the value of image-auto-resize is
a number.

Message #8 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: 65852 <at> debbugs.gnu.org
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 10 Sep 2023 10:48:38 -0300

[Message part 1 (text/plain, inline)]

tags 65852 patch
quit


Patch attached.

[0001-Fix-image-auto-resize-type-Bug-65852.patch (text/x-patch, attachment)]

Message #11 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>, 65852 <at> debbugs.gnu.org
Subject: Re: bug#65852: 30.0.50;
 image-auto-resize :type has choices in wrong order
Date: Sun, 10 Sep 2023 06:49:27 -0700

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> The :type for image-auto-resize looks like this:
>
> :type '(choice (const :tag "No resizing" nil)
>                 (const :tag "Fit to window" fit-window)
>                 (other :tag "Scale down to fit window" t)
>                 (number :tag "Scale factor" 1))
>
> The `other' choice should come last.  Otherwise Custom thinks that
> that's the selected option, even when the value of image-auto-resize is
> a number.

ENOPATCH

Thanks for working on this.  Is it possible to detect issues like these
statically?

Message #14 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 65852 <at> debbugs.gnu.org
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 10 Sep 2023 10:58:17 -0300

On 10/9/23 10:49, Stefan Kangas wrote:
> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> The :type for image-auto-resize looks like this:
>>
>> :type '(choice (const :tag "No resizing" nil)
>>                 (const :tag "Fit to window" fit-window)
>>                 (other :tag "Scale down to fit window" t)
>>                 (number :tag "Scale factor" 1))
>>
>> The `other' choice should come last.  Otherwise Custom thinks that
>> that's the selected option, even when the value of image-auto-resize is
>> a number.
>
> ENOPATCH

I was waiting to get a bug number assigned, so I could add it in the
commit message.  I don't know if there's any other way to do it.

> Thanks for working on this.  Is it possible to detect issues like these
> statically?

I worked on something like that for elint.el, based on the wishlist on
Bug#2591: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=2591

But I know it might be better to make it so that the byte-compiler
warns, so I never posted it.

Message #17 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>, 65852 <at> debbugs.gnu.org
Cc: Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#65852: 30.0.50;
 image-auto-resize :type has choices in wrong order
Date: Sun, 10 Sep 2023 07:24:52 -0700

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> I was waiting to get a bug number assigned, so I could add it in the
> commit message.  I don't know if there's any other way to do it.

That's the only way to do it, as far as I know.  Thanks for doing that,
because it does save us time when installing patches.

I guess it was me that was a bit quick here.

> I worked on something like that for elint.el, based on the wishlist on
> Bug#2591: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=2591
>
> But I know it might be better to make it so that the byte-compiler
> warns, so I never posted it.

I'm not sure if elint.el is used all that much, but I might be wrong.

Mattias, do you think it would make sense to add some warnings for
defcustom types to the byte-compiler?  If so, I guess we'd want to limit
ourselves to flagging obvious errors.

Message #20 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 65852 <at> debbugs.gnu.org
Cc: Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 10 Sep 2023 11:47:23 -0300

[Message part 1 (text/plain, inline)]

On 10/9/23 11:24, Stefan Kangas wrote:
>> I worked on something like that for elint.el, based on the wishlist on
>> Bug#2591: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=2591
>>
>> But I know it might be better to make it so that the byte-compiler
>> warns, so I never posted it.
>
> I'm not sure if elint.el is used all that much, but I might be wrong.

A shame, if you ask me.  It helped me find two more options with this
problem: font-lock-verbose and message-openpgp-header.  I attach an
updated patch, but let me know if you prefer a separate bug report.

[0001-Fix-order-of-other-choice-in-defcustom-type.patch (text/x-patch, attachment)]

Message #23 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 65852 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 10 Sep 2023 17:38:40 +0200

10 sep. 2023 kl. 16.24 skrev Stefan Kangas <stefankangas <at> gmail.com>:

> Mattias, do you think it would make sense to add some warnings for
> defcustom types to the byte-compiler?  If so, I guess we'd want to limit
> ourselves to flagging obvious errors.

This appears to be about checking the specified type itself. Another frequently made mistake is having an initial value that does not match the type.

Both of these cases should indeed ideally be caught at compile time but the design of `defcustom` makes it impossible in general. In fact it's probably not even possible to type-check the initial value at load time.

However we can check when the type and value are given as compile-time constants, and we could postpone some of the linting to load time if it wasn't possible earlier.

This is just macro work; I don't see any reason why the byte-compiler itself should need to be involved directly.

Message #28 received at 65852-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Mauro Aranda <maurooaranda <at> gmail.com>, 65852-done <at> debbugs.gnu.org
Cc: Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#65852: 30.0.50;
 image-auto-resize :type has choices in wrong order
Date: Sun, 10 Sep 2023 08:38:03 -0700

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> On 10/9/23 11:24, Stefan Kangas wrote:
>  >> I worked on something like that for elint.el, based on the wishlist on
>  >> Bug#2591: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=2591
>  >>
>  >> But I know it might be better to make it so that the byte-compiler
>  >> warns, so I never posted it.
>  >
>  > I'm not sure if elint.el is used all that much, but I might be wrong.
>
> A shame, if you ask me.  It helped me find two more options with this
> problem: font-lock-verbose and message-openpgp-header.  I attach an
> updated patch, but let me know if you prefer a separate bug report.

Thanks, pushed to master.

Message #31 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 65852 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Tue, 12 Sep 2023 16:42:41 +0200

[Message part 1 (text/plain, inline)]

I now see that we have an existing defcustom check that runs very late in the compilation.
Although I prefer this kind of check to be carried out during macro-expansion, doing so has the disadvantage that actual values aren't always available. On the other hand, defcustom arguments are usually constants.

Anyway, I went overboard and wrote a sizeable expansion to the current set of warnings and now also checks :type args in define-widget (see attached patch). Try it out and tell me what you think. Maybe the regexp check is too ad-hocky.

Another warning that I rather like but may give too many false positives is that of `const` and `other` types without an actual value which is then assumed to be nil. This seems to be an undocumented 'feature' but it doesn't help readability; it's often unclear whether `nil` was intended or just a result of a forgotten value.

[more-defcustom-warnings.diff (application/octet-stream, attachment)]

Message #34 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 65852 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 17 Sep 2023 17:20:20 +0200

A slightly restrained version of the above patch is now on master. Please let me know if anything needs changing.

Message #37 received at 65852 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>,
 Stefan Kangas <stefankangas <at> gmail.com>
Cc: 65852 <at> debbugs.gnu.org
Subject: Re: bug#65852: 30.0.50; image-auto-resize :type has choices in wrong
 order
Date: Sun, 17 Sep 2023 18:53:15 -0300

On 17/9/23 12:20, Mattias Engdegård wrote:
> A slightly restrained version of the above patch is now on
> master. Please let me know if anything needs changing.

Thanks!

I think https://debbugs.gnu.org/cgi/bugreport.cgi?bug=2591 can be closed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.