unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
@ 2018-11-07 13:21 João Távora
  2018-11-08  0:05 ` Noam Postavsky
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-07 13:21 UTC (permalink / raw)
  To: 33301

Hi maintainers,

This doesn't seem the correct indentation for the following elisp forms:

(let ((bla
       (ok))
      (defan
	(strange))))

(cond (bla
       ok)
      (defan
        strange))

...but that's the way Emacs -Q does it.  I'd be suprised if this weren't
a duplicate, but I thought I'd report it just in case.

Thanks,
João





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-07 13:21 bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.." João Távora
@ 2018-11-08  0:05 ` Noam Postavsky
  2018-11-08  0:35   ` Michael Heerdegen
  0 siblings, 1 reply; 24+ messages in thread
From: Noam Postavsky @ 2018-11-08  0:05 UTC (permalink / raw)
  To: João Távora; +Cc: 33301

found 33301 24.3
tags 33301 + confirmed
quit

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

> This doesn't seem the correct indentation for the following elisp forms:
>
> (let ((bla
>        (ok))
>       (defan
>         (strange))))
>
> (cond (bla
>        ok)
>       (defan
>         strange))
>
> ...but that's the way Emacs -Q does it.  I'd be suprised if this weren't
> a duplicate, but I thought I'd report it just in case.

I can't find any duplicate, though it's certainly not new.  Bug#9622 and
Bug#23108 are sort of related.






^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-08  0:05 ` Noam Postavsky
@ 2018-11-08  0:35   ` Michael Heerdegen
  2018-11-08  9:52     ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Michael Heerdegen @ 2018-11-08  0:35 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 33301, João Távora

Noam Postavsky <npostavs@gmail.com> writes:

> > (cond (bla
> >        ok)
> >       (defan
> >         strange))

That's explicitly done in 'lisp-indent-function':

(cond ((or (eq method 'defun)
		   (and (null method)
			(> (length function) 3)
			(string-match "\\`def" function))) ;; <==
	       (lisp-indent-defform state indent-point))
	      ((integerp method)
	       (lisp-indent-specform method state
				     indent-point normal-indent))
	      (method
		(funcall method indent-point state)))


Michael.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-08  0:35   ` Michael Heerdegen
@ 2018-11-08  9:52     ` João Távora
  2018-11-09  0:13       ` Michael Heerdegen
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-08  9:52 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 33301, Noam Postavsky

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>
>> > (cond (bla
>> >        ok)
>> >       (defan
>> >         strange))
>
> That's explicitly done in 'lisp-indent-function':
> 		   (and (null method)
> 			(> (length function) 3)
> 			(string-match "\\`def" function))) ;; <==

Ah, that's unfortunate.  Still, coundn't we improve the heuristic by
asking if the "function" has a macro definition?  Isn't that closer to
the intended behaviour?

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index afb7cbd1dd..e7373ece85 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1104,7 +1104,8 @@ lisp-indent-function
 	(cond ((or (eq method 'defun)
 		   (and (null method)
 			(> (length function) 3)
-			(string-match "\\`def" function)))
+			(string-match "\\`def" function)
+			(macrop (intern function))))
 	       (lisp-indent-defform state indent-point))
 	      ((integerp method)
 	       (lisp-indent-specform method state

João








^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-08  9:52     ` João Távora
@ 2018-11-09  0:13       ` Michael Heerdegen
  2018-11-09  0:41         ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Michael Heerdegen @ 2018-11-09  0:13 UTC (permalink / raw)
  To: João Távora; +Cc: 33301, Noam Postavsky

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

> Ah, that's unfortunate.  Still, coundn't we improve the heuristic by
> asking if the "function" has a macro definition?  Isn't that closer to
> the intended behaviour?
>
> diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
> index afb7cbd1dd..e7373ece85 100644
> --- a/lisp/emacs-lisp/lisp-mode.el
> +++ b/lisp/emacs-lisp/lisp-mode.el
> @@ -1104,7 +1104,8 @@ lisp-indent-function
>  	(cond ((or (eq method 'defun)
>  		   (and (null method)
>  			(> (length function) 3)
> -			(string-match "\\`def" function)))
> +			(string-match "\\`def" function)
> +			(macrop (intern function))))
>  	       (lisp-indent-defform state indent-point))
>  	      ((integerp method)
>  	       (lisp-indent-specform method state

If that macro is defined, shouldn't it already be indented correctly
without heuristics?

Michael.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09  0:13       ` Michael Heerdegen
@ 2018-11-09  0:41         ` João Távora
  2018-11-09  1:45           ` Michael Heerdegen
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-09  0:41 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 33301, Noam Postavsky

Michael Heerdegen <michael_heerdegen@web.de> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> Ah, that's unfortunate.  Still, coundn't we improve the heuristic by
>> asking if the "function" has a macro definition?  Isn't that closer to
>> the intended behaviour?
>>
>> diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
>> index afb7cbd1dd..e7373ece85 100644
>> --- a/lisp/emacs-lisp/lisp-mode.el
>> +++ b/lisp/emacs-lisp/lisp-mode.el
>> @@ -1104,7 +1104,8 @@ lisp-indent-function
>>  	(cond ((or (eq method 'defun)
>>  		   (and (null method)
>>  			(> (length function) 3)
>> -			(string-match "\\`def" function)))
>> +			(string-match "\\`def" function)
>> +			(macrop (intern function))))
>>  	       (lisp-indent-defform state indent-point))
>>  	      ((integerp method)
>>  	       (lisp-indent-specform method state
>
> If that macro is defined, shouldn't it already be indented correctly
> without heuristics?

I don't think so, not without an explicit indent declaration spec in the
macro definition.

This may explain the string-match hack in the first place. I don't know
the exact motivation of the hack, but it's been there since the initial
2001 revision of the file.  Possibly before declare/indent existed?

If you're suggesting removing it entirely, I don't oppose it.  There's
the downside that indentation relying on it would start to fail, but
diffs normally spot that and this would encourage users to add proper
indent (and edebug) specs to their macros.

Otherwise, I think my macrop tweak does a slightly better job at
avoiding false positives.

João





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09  0:41         ` João Távora
@ 2018-11-09  1:45           ` Michael Heerdegen
  2018-11-09  9:04             ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Michael Heerdegen @ 2018-11-09  1:45 UTC (permalink / raw)
  To: João Távora; +Cc: 33301, Noam Postavsky

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

> This may explain the string-match hack in the first place. I don't know
> the exact motivation of the hack, but it's been there since the initial
> 2001 revision of the file.  Possibly before declare/indent existed?

But wait, this is in lisp-mode.el which I remember is used not only for
Elisp but also for other Lisps, right?  So your patch could make things
worse for editing Common Lisp, for example.

For Elisp the heuristic doesn't make much sense, though, if the edited
file is not loaded, it also prevents false negatives for macro uses of
macros defined in that file.


Michael.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09  1:45           ` Michael Heerdegen
@ 2018-11-09  9:04             ` João Távora
  2018-11-09  9:51               ` Michael Heerdegen
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-09  9:04 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 33301, Noam Postavsky

Michael Heerdegen <michael_heerdegen@web.de> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> This may explain the string-match hack in the first place. I don't know
>> the exact motivation of the hack, but it's been there since the initial
>> 2001 revision of the file.  Possibly before declare/indent existed?
>
> But wait, this is in lisp-mode.el which I remember is used not only for
> Elisp but also for other Lisps, right?

Well it's lisp/emacs-lisp/... ;-)

> So your patch could make things worse for editing Common Lisp, for
> example.

OK, just add (derived-mode-p 'emacs-lisp-mode), as is done elsewhere in
that file.

Or I would suggest (setq-local lisp-indent-function
'common-lisp-indent-function) in you hypothetical fancy-lisp-mode hook
and has much better heuristics that don't cause the bug I've described.

(But, as someone who writes CL for a living, if you're indenting CL with
these heuristics, you've already lost.  You should use SLY/SLIME which
looks at the macroexpansion to understand what you're trying to indent.)

> For Elisp the heuristic doesn't make much sense, though, if the edited
> file is not loaded, it also prevents false negatives for macro uses of
> macros defined in that file.

I don't fully understand the "it also" part, but here's my take on this:
If you're not loading the code, all things being equal, it's better to
incorrectly re-indent existing "def"-macros (not defmacro) than to
incorrectly indent new arbitrary "def"-forms anywhere in the AST.
That's because it's a bad idea to re-indent code anyway, but indent new
code happens all the time.

Also, it's not a very good idea to indent without some form of
evaluation anyway.  Because of the indentation declaration, that ship
has sailed long ago (and bon voyage).

João





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09  9:04             ` João Távora
@ 2018-11-09  9:51               ` Michael Heerdegen
  2018-11-09 12:28                 ` Noam Postavsky
  0 siblings, 1 reply; 24+ messages in thread
From: Michael Heerdegen @ 2018-11-09  9:51 UTC (permalink / raw)
  To: João Távora; +Cc: 33301, Noam Postavsky

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

> OK, just add (derived-mode-p 'emacs-lisp-mode), as is done elsewhere
> in that file.

I think we could do that, but I don't feel qualified to decide.  Maybe
Noam can help.

A quick search shows that the "\\`def" trick is also performed
elsewhere, btw: `common-lisp-indent-function-1' does it in one place,
and also `scheme-indent-function'.

As far as the example in your bug report is concerned, I think it would
also be an improvement if elisp-mode wouldn't try be clever in such a
way when indenting branches in a cond or variable associations in a let.


Michael.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09  9:51               ` Michael Heerdegen
@ 2018-11-09 12:28                 ` Noam Postavsky
  2018-11-09 19:39                   ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Noam Postavsky @ 2018-11-09 12:28 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 33301, João Távora

Michael Heerdegen <michael_heerdegen@web.de> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> OK, just add (derived-mode-p 'emacs-lisp-mode), as is done elsewhere
>> in that file.
>
> I think we could do that, but I don't feel qualified to decide.  Maybe
> Noam can help.

I think it would be acceptable.

> As far as the example in your bug report is concerned, I think it would
> also be an improvement if elisp-mode wouldn't try be clever in such a
> way when indenting branches in a cond or variable associations in a let.

The problem is that we currently don't have any way of specifying
indentation for subforms of macro arguments, which is also the core
problem of the other bugs I mentioned about indentation of cl-flet and
friends.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09 12:28                 ` Noam Postavsky
@ 2018-11-09 19:39                   ` João Távora
  2018-11-10  4:48                     ` Michael Heerdegen
                                       ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: João Távora @ 2018-11-09 19:39 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Michael Heerdegen, 33301

Noam Postavsky <npostavs@gmail.com> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
>>> OK, just add (derived-mode-p 'emacs-lisp-mode), as is done elsewhere
>>> in that file.
>>
>> I think we could do that, but I don't feel qualified to decide.  Maybe
>> Noam can help.
>
> I think it would be acceptable.

OK.  If noone opposes I will commit the patch after my sig in a couple
of days time.

>
>> As far as the example in your bug report is concerned, I think it would
>> also be an improvement if elisp-mode wouldn't try be clever in such a
>> way when indenting branches in a cond or variable associations in a let.
>
> The problem is that we currently don't have any way of specifying
> indentation for subforms of macro arguments, which is also the core
> problem of the other bugs I mentioned about indentation of cl-flet and
> friends.

Well, after some testing with

   (setq lisp-indent-function 'common-lisp-indent-function)

things seem to work as they should.  Though the name is "common lisp",
cl-indent.el's header says it can be used with emacs-lisp-mode, and
indeed it seems to be the case.  In Emacs -Q:

   (setq lisp-indent-function 'common-lisp-indent-function)
    
   (flet ((blablabla
              (correct)
            also-correct))
     ...)
    
   (defmacro deffoo (name args &rest body)
     ;; no indent spec needed
     `(defun ,name ,args ,@body))
    
   (deffoo test
       (correct)
     also-correct)
    
   (defmacro defbla (name args moreargs &rest body)
     (declare (indent 3))
     (frobnicate moreargs)
     `(defun ,name ,args ,@body))
    
   (defbla test
       (correct)
       (also-correct)
     also-also-correct)

   (cond (defoo
          correct))
    
   (let ((defoo
          correct)))

But I don't know if I'm missing anything very important here.  Are there
emacs-lisp indentation tests somewhere?

João

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index afb7cbd1dd..b1a99351ed 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1063,8 +1063,8 @@ lisp-indent-function
 it specifies how to indent.  The property value can be:
 
 * `defun', meaning indent `defun'-style
-  (this is also the case if there is no property and the function
-  has a name that begins with \"def\", and three or more arguments);
+  (this is also the case if there is no property and a macro has
+  a name that begins with \"def\", and three or more arguments);
 
 * an integer N, meaning indent the first N arguments specially
   (like ordinary function arguments), and then indent any further
@@ -1104,7 +1104,9 @@ lisp-indent-function
 	(cond ((or (eq method 'defun)
 		   (and (null method)
 			(> (length function) 3)
-			(string-match "\\`def" function)))
+			(string-match "\\`def" function)
+                        (or (not (derived-mode-p 'emacs-lisp-mode))
+                            (macrop (intern function)))))
 	       (lisp-indent-defform state indent-point))
 	      ((integerp method)
 	       (lisp-indent-specform method state

	    





^ permalink raw reply related	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09 19:39                   ` João Távora
@ 2018-11-10  4:48                     ` Michael Heerdegen
  2018-11-10 10:28                       ` João Távora
  2018-11-10 10:54                     ` Andreas Schwab
  2020-08-22 14:58                     ` Lars Ingebrigtsen
  2 siblings, 1 reply; 24+ messages in thread
From: Michael Heerdegen @ 2018-11-10  4:48 UTC (permalink / raw)
  To: João Távora; +Cc: 33301, Noam Postavsky

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

> +                        (or (not (derived-mode-p 'emacs-lisp-mode))
> +                            (macrop (intern function)))))

That should better be `intern-soft' I think, right?

Michael.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-10  4:48                     ` Michael Heerdegen
@ 2018-11-10 10:28                       ` João Távora
  0 siblings, 0 replies; 24+ messages in thread
From: João Távora @ 2018-11-10 10:28 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 33301, Noam Postavsky

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

On Sat, Nov 10, 2018, 04:48 Michael Heerdegen <michael_heerdegen@web.de
wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > +                        (or (not (derived-mode-p 'emacs-lisp-mode))
> > +                            (macrop (intern function)))))
>
> That should better be `intern-soft' I think, right?
>

Probably, otherwise you intern some def-symbols even before reading then.
Hope nil doesn't break macrop.

And it has to ask for special-form-p, too otherwise your defvars are messed
up...

João

>
> Michael.
>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09 19:39                   ` João Távora
  2018-11-10  4:48                     ` Michael Heerdegen
@ 2018-11-10 10:54                     ` Andreas Schwab
  2018-11-10 12:46                       ` João Távora
  2020-08-22 14:58                     ` Lars Ingebrigtsen
  2 siblings, 1 reply; 24+ messages in thread
From: Andreas Schwab @ 2018-11-10 10:54 UTC (permalink / raw)
  To: João Távora; +Cc: Michael Heerdegen, 33301, Noam Postavsky

On Nov 09 2018, João Távora <joaotavora@gmail.com> wrote:

> @@ -1104,7 +1104,9 @@ lisp-indent-function
>  	(cond ((or (eq method 'defun)
>  		   (and (null method)
>  			(> (length function) 3)
> -			(string-match "\\`def" function)))
> +			(string-match "\\`def" function)
> +                        (or (not (derived-mode-p 'emacs-lisp-mode))
> +                            (macrop (intern function)))))

Why is a defining function required to be a macro?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-10 10:54                     ` Andreas Schwab
@ 2018-11-10 12:46                       ` João Távora
  2018-11-10 12:53                         ` Andreas Schwab
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-10 12:46 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Michael Heerdegen, 33301, Noam Postavsky

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Nov 09 2018, João Távora <joaotavora@gmail.com> wrote:
>
>> @@ -1104,7 +1104,9 @@ lisp-indent-function
>>  	(cond ((or (eq method 'defun)
>>  		   (and (null method)
>>  			(> (length function) 3)
>> -			(string-match "\\`def" function)))
>> +			(string-match "\\`def" function)
>> +                        (or (not (derived-mode-p 'emacs-lisp-mode))
>> +                            (macrop (intern function)))))
>
> Why is a defining function required to be a macro?

Do you know any that aren't?  Of course you can name a function however
you want, and give it your own "definition" semantics, but if you want
to do it in the lisp sense, generally the properties of macros are good:

1. They are evaluated at compilation time, so the byte compiler can
   understand the uses of your new definition down the line;

2. The arguments of a macro don't get evaluated unless you want them to,
   which is crucial for defining formal arguments.

João

PS: a defining function can also be a special form, which also has the
above properties





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-10 12:46                       ` João Távora
@ 2018-11-10 12:53                         ` Andreas Schwab
  2018-11-10 16:05                           ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Andreas Schwab @ 2018-11-10 12:53 UTC (permalink / raw)
  To: João Távora; +Cc: Michael Heerdegen, 33301, Noam Postavsky

On Nov 10 2018, João Távora <joaotavora@gmail.com> wrote:

> Do you know any that aren't?

define-widget

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-10 12:53                         ` Andreas Schwab
@ 2018-11-10 16:05                           ` João Távora
  2018-11-10 16:18                             ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2018-11-10 16:05 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Michael Heerdegen, 33301, Noam Postavsky

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Nov 10 2018, João Távora <joaotavora@gmail.com> wrote:
>
>> Do you know any that aren't?
>
> define-widget

A poorly chosen name for what should have been named `make-widget-type'
(and define-widget should have been a macro relieving the user of all
that useless quoting).

Anyhoo, it's a question of

  (function-put 'define-widget 'lisp-indent-function 'defun)

Any objections to that? Do you know any more?

João










^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-10 16:05                           ` João Távora
@ 2018-11-10 16:18                             ` João Távora
  0 siblings, 0 replies; 24+ messages in thread
From: João Távora @ 2018-11-10 16:18 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Michael Heerdegen, 33301, Noam Postavsky

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

Never mind  that, I just grepped for `(defun def` and there are a lot more,
though not quite as much as `defmacro def`

Indeed I don't know how to proceed.  There seem to be around 34.
Add indent specs for these 34 symbols? I suspect that some have exclusively
one-line uses that don't need them.

Perhaps someone else can weigh in.

João

On Sat, Nov 10, 2018 at 4:05 PM João Távora <joaotavora@gmail.com> wrote:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
> > On Nov 10 2018, João Távora <joaotavora@gmail.com> wrote:
> >
> >> Do you know any that aren't?
> >
> > define-widget
>
> A poorly chosen name for what should have been named `make-widget-type'
> (and define-widget should have been a macro relieving the user of all
> that useless quoting).
>
> Anyhoo, it's a question of
>
>   (function-put 'define-widget 'lisp-indent-function 'defun)
>
> Any objections to that? Do you know any more?
>
> João
>
>
>
>
>
>

-- 
João Távora

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2018-11-09 19:39                   ` João Távora
  2018-11-10  4:48                     ` Michael Heerdegen
  2018-11-10 10:54                     ` Andreas Schwab
@ 2020-08-22 14:58                     ` Lars Ingebrigtsen
  2020-08-22 16:19                       ` João Távora
  2 siblings, 1 reply; 24+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-22 14:58 UTC (permalink / raw)
  To: João Távora; +Cc: Michael Heerdegen, 33301, Noam Postavsky

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

> -			(string-match "\\`def" function)))
> +			(string-match "\\`def" function)
> +                        (or (not (derived-mode-p 'emacs-lisp-mode))
> +                            (macrop (intern function)))))

As others noted, this means that indentation changes when you've
loaded/not loaded the file that defines the macro, which may be
awkward.  But also, as your grep showed, there's oodles of functions in
Emacs the define stuff, and you'd have to add indentation specs to all
of them.  And that doesn't take out-of-tree definitions into account.

So I don't really see how this can be fixed in any sensible way --
changing this heuristic will just annoy more than it fixes things, I
think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2020-08-22 14:58                     ` Lars Ingebrigtsen
@ 2020-08-22 16:19                       ` João Távora
  2020-08-23 12:26                         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2020-08-22 16:19 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Michael Heerdegen, 33301, Noam Postavsky

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

Hi Lars, thanks for your review of old bugs.

I wouldn't count 34 as "oodles" and don't think a new line for each
occurrence of what is essentially a breach of convention is a high price to
pay. Even converting some of those to macros or "make-foo" could be worth
it if it would enable non-surprising indentation.

As for the problem of needing to load macros before indenting forms where
they appears, that's already very much a thing. We wouldn't be creating new
problems there, it's just the way it is.

As for out-of-tree definitions, we could be lenient and have this saner
indentation be controlled by a variable which we would default to 'insane,
but to  'sane inside Emacs's source, via directory local variables.

So I don't think we should throw in the towel on this one.

João

On Sat, Aug 22, 2020, 15:58 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > -                     (string-match "\\`def" function)))
> > +                     (string-match "\\`def" function)
> > +                        (or (not (derived-mode-p 'emacs-lisp-mode))
> > +                            (macrop (intern function)))))
>
> As others noted, this means that indentation changes when you've
> loaded/not loaded the file that defines the macro, which may be
> awkward.  But also, as your grep showed, there's oodles of functions in
> Emacs the define stuff, and you'd have to add indentation specs to all
> of them.  And that doesn't take out-of-tree definitions into account.
>
> So I don't really see how this can be fixed in any sensible way --
> changing this heuristic will just annoy more than it fixes things, I
> think.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2020-08-22 16:19                       ` João Távora
@ 2020-08-23 12:26                         ` Lars Ingebrigtsen
  2020-08-23 13:39                           ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-23 12:26 UTC (permalink / raw)
  To: João Távora; +Cc: Michael Heerdegen, 33301, Noam Postavsky

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

> I wouldn't count 34 as "oodles" and don't think a new line for each
> occurrence of what is essentially a breach of convention is a high
> price to pay. Even converting some of those to macros or "make-foo"
> could be worth it if it would enable non-surprising indentation.

Changing functions to macros, or renaming functions from def* to make*,
just because we have a slightly odd heuristic in Emacs Lisp mode doesn't
sound quite right to me.

> As for the problem of needing to load macros before indenting forms
> where they appears, that's already very much a thing. We wouldn't be
> creating new problems there, it's just the way it is.

That's true, but it would exacerbate the problem.  But the main problem
is that it would indent the code differently than it does now, and that
leads to whitespace churn in the vc, which we should avoid unless we
have a very, very good reason not to.  And this just doesn't seem like a
good enough reason...

> As for out-of-tree definitions, we could be lenient and have this
> saner indentation be controlled by a variable which we would default
> to 'insane, but to 'sane inside Emacs's source, via directory local
> variables.

I'd be against that -- again, because it leads to whitespace VC churn.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2020-08-23 12:26                         ` Lars Ingebrigtsen
@ 2020-08-23 13:39                           ` João Távora
  2020-08-24 13:12                             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 24+ messages in thread
From: João Távora @ 2020-08-23 13:39 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Michael Heerdegen, 33301, Noam Postavsky

Lars Ingebrigtsen <larsi@gnus.org> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> I wouldn't count 34 as "oodles" and don't think a new line for each
>> occurrence of what is essentially a breach of convention is a high
>> price to pay. Even converting some of those to macros or "make-foo"
>> could be worth it if it would enable non-surprising indentation.
>
> Changing functions to macros, or renaming functions from def* to make*,
> just because we have a slightly odd heuristic in Emacs Lisp mode doesn't
> sound quite right to me.

This wouldn't be the only reason to do that.  As I explained
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33301#51 this convention
is good in itself.

>> As for the problem of needing to load macros before indenting forms
>> where they appears, that's already very much a thing. We wouldn't be
>> creating new problems there, it's just the way it is.
>
> That's true, but it would exacerbate the problem.

Really doubt that. I think regular use of macros (like, say with-foo) is
prevalent enough that people already know not to mass-reindent existing
code anyway.

> But the main problem is that it would indent the code differently than
> it does now, and that leads to whitespace churn in the vc, which we
> should avoid unless we have a very, very good reason not to.

What exactly do you mean "whitespace churn"?  Can you illustrate this
hypothetical scenario?  I don't expect whitespace/indentation beyond
fixing the akward cases, at least that's the entire point of this
report.

> good enough reason...

It is a somewhat infrequent but annoying bug when one decides to use a
variable name that happens to start with "def".  I use "default" and
"deferred" a lot, for instance.  In Common Lisp mode, I don't have this
problem.

>> As for out-of-tree definitions, we could be lenient and have this
>> saner indentation be controlled by a variable which we would default
>> to 'insane, but to 'sane inside Emacs's source, via directory local
>> variables.
>
> I'd be against that -- again, because it leads to whitespace VC churn.

Again, I'm missing something: this option wouldn't lead to that, I think

João

PS: another entirely different approach would just limit the current
hacky heuristic to calls/expansions that happen at top-level, i.e. at
"column 0".  I believe this to be the vast majority (though not the
entirety) of cases.





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2020-08-23 13:39                           ` João Távora
@ 2020-08-24 13:12                             ` Lars Ingebrigtsen
  2020-08-25 19:59                               ` João Távora
  0 siblings, 1 reply; 24+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-24 13:12 UTC (permalink / raw)
  To: João Távora; +Cc: Michael Heerdegen, 33301, Noam Postavsky

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

>> But the main problem is that it would indent the code differently than
>> it does now, and that leads to whitespace churn in the vc, which we
>> should avoid unless we have a very, very good reason not to.
>
> What exactly do you mean "whitespace churn"?  Can you illustrate this
> hypothetical scenario?  I don't expect whitespace/indentation beyond
> fixing the akward cases, at least that's the entire point of this
> report.

It means indenting some things in a different way than today?  That
leads to whitespace changes.

>>> As for out-of-tree definitions, we could be lenient and have this
>>> saner indentation be controlled by a variable which we would default
>>> to 'insane, but to 'sane inside Emacs's source, via directory local
>>> variables.
>>
>> I'd be against that -- again, because it leads to whitespace VC churn.
>
> Again, I'm missing something: this option wouldn't lead to that, I think

If some people have the variable set to 'insane, they would indent the
code they're writing differently than the rest, which would lead to
whitespace churn.

> PS: another entirely different approach would just limit the current
> hacky heuristic to calls/expansions that happen at top-level, i.e. at
> "column 0".  I believe this to be the vast majority (though not the
> entirety) of cases.

That's probably true...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 24+ messages in thread

* bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.."
  2020-08-24 13:12                             ` Lars Ingebrigtsen
@ 2020-08-25 19:59                               ` João Távora
  0 siblings, 0 replies; 24+ messages in thread
From: João Távora @ 2020-08-25 19:59 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Michael Heerdegen, 33301, Noam Postavsky

Lars Ingebrigtsen <larsi@gnus.org> writes:

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

> It means indenting some things in a different way than today?  That
> leads to whitespace changes.

Hmmm, an indentation bug such as this is, by definition, about an
incorrect amount of ... whitespace.  Right?

>>> I'd be against that -- again, because it leads to whitespace VC churn.
>> Again, I'm missing something: this option wouldn't lead to that, I think
> If some people have the variable set to 'insane, they would indent the
> code they're writing differently than the rest, which would lead to
> whitespace churn.

Well, they did set it to 'insane :-).  I don't see the problem here,
this variable would be similar to indent-tabs-mode.  If some people set
that differently it'll be equally disastrous.  But anyway, we probably
don't need the variable since I don't expect out of tree code to be
particularly affected: that's because, according to code conventions,
definitions should start with a package-specific prefix anyway.

João





^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-08-25 19:59 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-07 13:21 bug#33301: 27.0.50; broken elisp indentation for non-definition symbols starting with "def.." João Távora
2018-11-08  0:05 ` Noam Postavsky
2018-11-08  0:35   ` Michael Heerdegen
2018-11-08  9:52     ` João Távora
2018-11-09  0:13       ` Michael Heerdegen
2018-11-09  0:41         ` João Távora
2018-11-09  1:45           ` Michael Heerdegen
2018-11-09  9:04             ` João Távora
2018-11-09  9:51               ` Michael Heerdegen
2018-11-09 12:28                 ` Noam Postavsky
2018-11-09 19:39                   ` João Távora
2018-11-10  4:48                     ` Michael Heerdegen
2018-11-10 10:28                       ` João Távora
2018-11-10 10:54                     ` Andreas Schwab
2018-11-10 12:46                       ` João Távora
2018-11-10 12:53                         ` Andreas Schwab
2018-11-10 16:05                           ` João Távora
2018-11-10 16:18                             ` João Távora
2020-08-22 14:58                     ` Lars Ingebrigtsen
2020-08-22 16:19                       ` João Távora
2020-08-23 12:26                         ` Lars Ingebrigtsen
2020-08-23 13:39                           ` João Távora
2020-08-24 13:12                             ` Lars Ingebrigtsen
2020-08-25 19:59                               ` João Távora

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).