bug#8638: 24.0.50; Imenu should not include vacuous defvars

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

Could we please improve `lisp-imenu-generic-expression so that it does
not include vacuous defvars such as (defvar foobar), which are generally
used only to quiet the byte-compiler?

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2011-04-25 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.5) --no-opt --cflags
-Ic:/imagesupport/include'
 

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

On Sun, May 8, 2011 at 20:15, Drew Adams <drew.adams@oracle.com> wrote:

> Could we please improve `lisp-imenu-generic-expression so that it does
> not include vacuous defvars such as (defvar foobar), which are generally
> used only to quiet the byte-compiler?

With lexical binding, (defvar foobar) is used to tell the bytecompiler
that the variable has dynamic scope.

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> With lexical binding, (defvar foobar) is used to tell the bytecompiler
> that the variable has dynamic scope.

It's still a vacuous definition.  And any defvar tells the byte compiler that a
variable has dynamic scope, no?

This seems irrelevant to the bug report.  I'd still suggest removing vacuous
defvars from the menu.  Mixing in vacuous entries with entries that really
define variables distracts users.  You want to think that accessing a menu item
will take you to a real variable definition.

If someone wants to provide vacuous defvars in a different submenu from
`Variables' (e.g. `Vacuous Vars') I have no problem with that.  But I don't
really think that's needed.

FWIW, this is what I use in my code (imenu+.el):

(concat "^\\s-*("
        (regexp-opt
          '("defvar" "defconst" "defconstant" "defcustom"
            "defparameter" "define-symbol-macro") t)
        "\\s-+\\(\\sw\\(\\sw\\|\\s_\\)+\\)"
        "\\s-+[^) \t\n]")

Not perfect, perhaps, but it seems to do the job OK so far.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

> It's still a vacuous definition.  And any defvar tells the byte compiler that a
> variable has dynamic scope, no?

It also tells the user, so having them in the imenu doesn't seem
superfluous to me (I'm talking again of lexically-scoped packages).

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> > It's still a vacuous definition.  And any defvar tells the 
> > byte compiler that a variable has dynamic scope, no?
> 
> It also tells the user, so having them in the imenu doesn't seem
> superfluous to me (I'm talking again of lexically-scoped packages).

Submenu `Variables' should be for variable definitions, not vacuous defvars that
might be used to indicate something to the byte compiler.

As I said, if you want to also present those to the user, then let's put them in
a  separate submenu.  They amount to byte-compiler declarations.  Perhaps there
are other byte-compiler-related constructs that could also be added to the same
submenu.  Call it `Byte-Compiler' or `Declarations', perhaps.

But these are not variable definitions in the same sense as full defvars are.
Mixing them in with full definitions, in the same submenu, just amounts to
noise.  Separating them out in a separate menu would be fine.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

On Sun, May 8, 2011 at 21:36, Drew Adams <drew.adams@oracle.com> wrote:

> But these are not variable definitions in the same sense as full defvars are.

I disagree. IMHO, in a lexical binding package, yes, there are
variable definitions. In some cases the variables are documented in
the docstring of a function or somesuch, but they are real variables
nonetheless. Instead of sweeping them under the carpet, perhaps it
would be better to suggest the programmer to add proper docstrings and
initial values to them.

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

> I disagree. IMHO, in a lexical binding package, yes, there are

s/there/they/

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> I disagree. IMHO, in a lexical binding package, yes, there are
> variable definitions. In some cases the variables are documented in
> the docstring of a function or somesuch, but they are real variables
> nonetheless. Instead of sweeping them under the carpet, perhaps it
> would be better to suggest the programmer to add proper docstrings and
> initial values to them.

No one is sweeping anything under the carpet.
If you want to show them in an Imenu menu, fine; just don't mix them in with
definitions that people will want to visit to see doc strings and initial
values.

That programmers should be encouraged to use doc strings and specify initial
values is a separate issue.  That does not imply that vacuous defvars should be
included in the Variables menu.

As a signal to the byte compiler, a vacuous definition is useful - as such.
That's what it is for.  By definition it should not have an initial value or doc
string.

That a vacuous definition, like a full one, is now used also to declare a
variable special (dynamic scoping) does not mean that vacuous defvars should be
included in the Variables menu.

Whether a vacuous definition should indicate dynamic scope to the byte-compiler
is another question.  As you say, in most cases what we want to suggest is that
programmers use a full definition for that, instead.  But using a vacuous
definition to quiet undefined var warnings is legitimate - in that case the
programmer does _not_ want to include any initial value.

IMO, you are mixing in things that don't belong to this thread.  A defvar that
is used _only_ as a byte-compiler declaration and not to provide an initial
value (and hopefully a doc string) does not belong in the same submenu as full
definitions.

When you follow a Variables menu entry to its code, you want to see what the
code for the variable is.  You do not want to see only a vacuous defvar that
provides no more information than the menu item itself.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

On Sun, May 8, 2011 at 22:03, Drew Adams <drew.adams@oracle.com> wrote:

> When you follow a Variables menu entry to its code, you want to see what the
> code for the variable is.  You do not want to see only a vacuous defvar that
> provides no more information than the menu item itself.

The imenu index is not documentation. It is an index to code. So if
the "vacuous variable" has twenty lines of comment explaining its
purpose and why it does not have or require a docstring or an initial
value, I fully expect imenu to help me get there too, just as if it
were a normal, "full" variable. Your expectations for imenu are just
that, your expectations. I prefer to be the judge of what it is
interesting in the code and what is not, not some imenu filter.

But that's just my opinion. I'm not going to complain if someone fixes
this bug to your liking.

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> > When you follow a Variables menu entry to its code, you 
> > want to see what the code for the variable is.  You do not
> > want to see only a vacuous defvar that
> > provides no more information than the menu item itself.
> 
> The imenu index is not documentation. It is an index to code. So if
> the "vacuous variable" has twenty lines of comment explaining its
> purpose and why it does not have or require a docstring or an initial
> value, I fully expect imenu to help me get there too, just as if it
> were a normal, "full" variable. Your expectations for imenu are just
> that, your expectations. I prefer to be the judge of what it is
> interesting in the code and what is not, not some imenu filter.
> 
> But that's just my opinion. I'm not going to complain if someone fixes
> this bug to your liking.

I understand.  How would you feel if Imenu included entries for defuns and
defvars that are commented out?  Wouldn't you want Imenu to judge that you are
not interested in those?

Certainly comments that include defuns and such could be important and
interesting to developers.  The question is what Imenu should include, and yes,
it is a judgment call based on expectations of what most users will want/expect.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

On Sun, May 8, 2011 at 22:39, Drew Adams <drew.adams@oracle.com> wrote:

> I understand.  How would you feel if Imenu included entries for defuns and
> defvars that are commented out?  Wouldn't you want Imenu to judge that you are
> not interested in those?

Well, we're not talking about commented out variables, so the
comparison isn't entirely fair.

But, to be honest, sometimes I think that entries for commented out
defuns and defvars would be useful... :-)

> Certainly comments that include defuns and such could be important and
> interesting to developers.  The question is what Imenu should include, and yes,
> it is a judgment call based on expectations of what most users will want/expect.

Not users. Developers. And developers are usually interested in a
package's variables. More so in a lexical-binding context, where the
fact that the defvar exists can (and does) alter semantics.

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> > it is a judgment call based on expectations of what most 
> > users will want/expect.
> 
> Not users. Developers. And developers are usually interested in a
> package's variables. More so in a lexical-binding context, where the
> fact that the defvar exists can (and does) alter semantics.

Users includes users who develop, and more.  Imenu is for all Emacs users,
including developers.  And the question is not whether developers are interested
in a package's variables.  It is whether the Variables submenu should include
entries for vacuous defvars.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Stefan Monnier]

>> Could we please improve `lisp-imenu-generic-expression so that it does
>> not include vacuous defvars such as (defvar foobar), which are generally
>> used only to quiet the byte-compiler?
> With lexical binding, (defvar foobar) is used to tell the bytecompiler
> that the variable has dynamic scope.

While it is true some some (defvar <foo>) are actually declarations that
<foo> is a locally-used dynamically bound variable (in which case,
maybe it could make sense to see it in imenu), the overwhelming
majority is to declare the existence of some variable in some other
package, in which case I don't think it deserves to be in imenu.
And even when it might make sense, its usefulness seems dubious since
those defvars don't hold much valuable info (like initial value,
docstring, or something).
So I'd tend to agree with Drew.  For completion purposes, we'd want to
pay attention to those defvars, but not for imenu.


        Stefan

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Juanma Barranquero]

On Mon, May 9, 2011 at 16:19, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> So I'd tend to agree with Drew.  For completion purposes, we'd want to
> pay attention to those defvars, but not for imenu.

I had already conceded (Warnock style, interpretation #1 ;-)

    Juanma

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> FWIW, this is what I use in my code (imenu+.el):
> (concat "^\\s-*("
>         (regexp-opt
>           '("defvar" "defconst" "defconstant" "defcustom"
>             "defparameter" "define-symbol-macro") t)
>         "\\s-+\\(\\sw\\(\\sw\\|\\s_\\)+\\)"
>         "\\s-+[^) \t\n]")
> Not perfect, perhaps, but it seems to do the job OK so far.


FWIW, I use this now.
Changed the whitespace match after var name.

(concat "^\\s-*("
        (regexp-opt
          '("defvar" "defconst" "defconstant" "defcustom"
            "defparameter" "define-symbol-macro") t)
        "\\s-+\\(\\sw\\(\\sw\\|\\s_\\)+\\)"
        "\\(\\s-\\|[\n]\\)+" ; \n has char syntax `>', not `-'
        "[^) \t\n]")

\s-+ does not match newlines in Lisp modes (newlines have comment-end syntax),
so I changed \s-+ to \(\s-\|[\n]\)+.  No doubt still not perfect, but seems to
work OK.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Chong Yidong]

"Drew Adams" <drew.adams@oracle.com> writes:

> FWIW, I use this now.
> Changed the whitespace match after var name.

I committed a slightly different fix to trunk.  Thanks.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> > FWIW, I use this now.
> > Changed the whitespace match after var name.
> 
> I committed a slightly different fix to trunk.  Thanks.

OK, thanks.  But I wonder why you treated defvar differently from defconst,
defconstant, defcustom, defparameter, and define-symbol-macro here.  Shouldn't
the same thing apply to them?

Just wondering.  Not that their use of a vacuous definition would mean the same
thing, or even necessarily by correct syntax.  But shouldn't a vacuous
definition using one of those others also be ignored by Imenu?

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Chong Yidong]

"Drew Adams" <drew.adams@oracle.com> writes:

> OK, thanks.  But I wonder why you treated defvar differently from
> defconst, defconstant, defcustom, defparameter, and
> define-symbol-macro here.  Shouldn't the same thing apply to them?

There is no such thing as (defconst foo), or (defcustom foo); the second
argument is non-optional.  So the reasoning which was used for defvar,
i.e. that a defvar with an omitted second arg is commonly used just to
silence the compiler, does not apply.

bug#8638: 24.0.50; Imenu should not include vacuous defvars

[Drew Adams]

> > OK, thanks.  But I wonder why you treated defvar differently from
> > defconst, defconstant, defcustom, defparameter, and
> > define-symbol-macro here.  Shouldn't the same thing apply to them?
> 
> There is no such thing as (defconst foo), or (defcustom foo); 
> the second argument is non-optional.

Yes, I know.

> So the reasoning which was used for defvar, i.e. that a defvar
> with an omitted second arg is commonly used just to silence the
> compiler, does not apply.

But the reasoning that such things, if they ever occurred, would not represent
proper definitions, so Imenu should not index them, holds.  They would represent
incorrect syntax (i.e., errors), and should not be indexed.

Anyway, I'm OK with them being falsely indexed, and I would agree if you made
the argument that we do not try to prevent indexing of incorrect syntax in
general.