bug#42411: Bug with M-x compile

bug#42411: Bug with M-x compile

[Gregory Heytings via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks for fixing (the second part of) bug#42383.

The first part remains unfixed, however:

There are too many completion candidates in the list of targets when 
completing M-x compile.  For example, for the Makefile 
"foo:\n\techo\x20bar:\n" three candidates are displayed: "foo", "echo" and 
"bar".  The regexp in pcmpl-gnu-make-targets is too large, and should be 
fixed as follows:

--- pcmpl-gnu.el.orig   2020-06-29 17:39:26.000000000 +0000
+++ pcmpl-gnu.el        2020-07-15 22:43:14.368938346 +0000
@@ -118,7 +118,7 @@
  Return the new list."
    (goto-char (point-min))
    (while (re-search-forward
-         "^\\s-*\\([^\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
+          "^\\([^\t\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
      (setq targets (nconc (split-string (match-string-no-properties 1))
                           targets)))
    targets)

I see no reason to allow one or more TABs or spaces at the beginning of 
targets, as does the "^\\s-*".  If one really wants to allow spaces (but 
not TABs) at the beginning of a target label, the following regexp could 
also be used: "^ *\\([^\t\n#%.$][^:=\n]*\\)\\s-*:[^=]".

The current regexp is an old one (since Emacs 21 at least), and is 
inconsistent with for example how bash computes completions (see 
_make_target_extract_script).

If changing the regexp is not an option, please make it a configuration 
option.

Gregory

bug#42411: Bug with M-x compile

[Eli Zaretskii]

> Date: Sat, 18 Jul 2020 09:01:48 +0000
> From: Gregory Heytings via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> There are too many completion candidates in the list of targets when 
> completing M-x compile.  For example, for the Makefile 
> "foo:\n\techo\x20bar:\n" three candidates are displayed: "foo", "echo" and 
> "bar".  The regexp in pcmpl-gnu-make-targets is too large, and should be 
> fixed as follows:

I'm not sure your proposed change is TRT.  It could very well cause
the opposite problem: valid targets are not proposed as completion
candidates.

You seem to assume that the valid candidates are only those that
appear as explicit targets in a Makefile.  But that disregards
implicit targets, which Make intuits on its own.  In the example you
show, those implicit targets make no sense, but that's not the only
use case we might want to support.  In a more general case, there
could be targets collected that way which are non-trivial, and which
users may wish to have as completion candidates.

Another situation we need to consider is the X makefiles, where
target names were preceded by whitespace.

And there could be other situations as well.  I'm not an expert; if we
want to review all the possible use cases, perhaps we should ask Paul
Smith, the GNU Make maintainer, to join this discussion and help us
enumerate the possible cases.

So I think we could have the change you propose as an optional
feature, but certainly not as the only behavior.  We could later
discuss whether this would be an opt-in or opt-out, but IMO it must be
controlled by a user option.

Thanks.

bug#42411: Bug with M-x compile

[Gregory Heytings via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>
>> There are too many completion candidates in the list of targets when 
>> completing M-x compile.  For example, for the Makefile 
>> "foo:\n\techo\x20bar:\n" three candidates are displayed: "foo", "echo" 
>> and "bar".  The regexp in pcmpl-gnu-make-targets is too large, and 
>> should be fixed as follows:
>
> I'm not sure your proposed change is TRT.
>

I proposed three options: (1) the regexp "^\\([^\t\n#%.$][^:=\n]*\\)\\s-*:[^=]", (2) the regexp "^ *\\([^\t\n#%.$][^:=\n]*\\)\\s-*:[^=]", (3) make that regexp a configuration option.

>
> It could very well cause the opposite problem: valid targets are not 
> proposed as completion candidates.
>

I agree with you that in more complex cases, it is possible that valid 
targets would not be proposed as completion candidates anymore.  But OTOH 
I don't think it is possible to have a complete list of valid targets only 
by parsing the Makefile with a regexp.  To have such a list it is 
necessary to use make itself (for example by using the output of make 
--print-data-base for GNU Make).

>
> Another situation we need to consider is the X makefiles, where target 
> names were preceded by whitespace.
>

Yes, that was my option (2).

>
> And there could be other situations as well.  I'm not an expert; if we 
> want to review all the possible use cases, perhaps we should ask Paul 
> Smith, the GNU Make maintainer, to join this discussion and help us 
> enumerate the possible cases.
>

I'm not an expert either, so yes, please ask Paul Smith for advice on 
this.  I do think that the way to compute completion candidates should be 
improved.

There will always be exceptional cases (for example, for GNU Make, 
.RECIPEPREFIX with which it is possible to use another prefix character 
instead of TAB can apparently be used multiple times), but for something 
like 99.9% cases, a line starting with a TAB is a recipe element and not a 
target, a line starting with a '#' is a comment, and a line starting with 
a '.' sets a special variable.  The current regexp correctly excludes the 
last two, but includes the first one.

>
> So I think we could have the change you propose as an optional feature, 
> but certainly not as the only behavior.  We could later discuss whether 
> this would be an opt-in or opt-out, but IMO it must be controlled by a 
> user option.
>

Yes, that was my option (3).  I certainly don't want to impose a change on 
everyone, especially as it has been Emacs's behavior for a long time. 
Ideally I think that it should be controlled by a variable, with a 
docstring presenting a number of typical values.

Gregory

bug#42411: Bug with M-x compile

[Eli Zaretskii]

> Date: Sat, 25 Jul 2020 23:29:09 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: Eli Zaretskii <eliz@gnu.org>
> 
> > And there could be other situations as well.  I'm not an expert; if we 
> > want to review all the possible use cases, perhaps we should ask Paul 
> > Smith, the GNU Make maintainer, to join this discussion and help us 
> > enumerate the possible cases.
> >
> 
> I'm not an expert either, so yes, please ask Paul Smith for advice on 
> this.  I do think that the way to compute completion candidates should be 
> improved.
> 
> There will always be exceptional cases (for example, for GNU Make, 
> .RECIPEPREFIX with which it is possible to use another prefix character 
> instead of TAB can apparently be used multiple times), but for something 
> like 99.9% cases, a line starting with a TAB is a recipe element and not a 
> target, a line starting with a '#' is a comment, and a line starting with 
> a '.' sets a special variable.  The current regexp correctly excludes the 
> last two, but includes the first one.

Paul, could you please chime in and share your views on this?  If you
want to read the discussion from the beginning, you can find it at

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42411

or, if you prefer to read all of it in your MUA, you can download all
the messages in the mbox format:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42411;mbox=yes

TIA

bug#42411: Bug with M-x compile

[Paul Smith]

On Sun, 2020-07-26 at 16:55 +0300, Eli Zaretskii wrote:
> > > And there could be other situations as well.  I'm not an expert; if we 
> > > want to review all the possible use cases, perhaps we should ask Paul 
> > > Smith, the GNU Make maintainer, to join this discussion and help us 
> > > enumerate the possible cases.
> > I'm not an expert either, so yes, please ask Paul Smith for advice on 
> > this.  I do think that the way to compute completion candidates should be 
> > improved.
> > There will always be exceptional cases (for example, for GNU Make, 
> > .RECIPEPREFIX with which it is possible to use another prefix character 
> > instead of TAB can apparently be used multiple times), but for something 
> > like 99.9% cases, a line starting with a TAB is a recipe element and not a 
> > target, a line starting with a '#' is a comment, and a line starting with 
> > a '.' sets a special variable.  The current regexp correctly excludes the 
> > last two, but includes the first one.
> 
> 
> Paul, could you please chime in and share your views on this?  If you
> want to read the discussion from the beginning, you can find it at
> 
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42411
> 
> or, if you prefer to read all of it in your MUA, you can download all
> the messages in the mbox format:
> 
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42411;mbox=yes

Sorry for the delay in response: it's been a week at $DAYJOB.

I guess I'm not exactly sure what the ask is here.

It's definitely true that technically, it is possible to have targets
that are indented by TABs.  A line indented by a TAB is only considered
part of a recipe if it appears in the "recipe context", which means
somewhere that a recipe is legal in the syntax.

If it's not legal for a recipe command to appear there then TABs are
treated like any other whitespace.

In practice, I think it's highly unlikely that anyone would
intentionally use TABs to indent targets because it's so fragile: any
reordering of the makefile or adding new lines could cause that
makefile to break.

So, as a simplifying assumption it makes sense to me to ignore any line
starting with TAB when trying to detect targets.

Of course, as Eli points out there are certainly a large number of
potential targets which cannot be determined using this type of simple
investigation.  The most obvious are targets that match patterns.

However I'll say two things about this:

First, I think it's unlikely that users would really want to see all
the potential matches of targets when doing completion.  It's most
likely that they are interested in the "top level" intended command
line goals rather than every possible object, source, etc. file that
make considers a target due to pattern or suffix rules.

Second, I don't think there's currently any good way to list those
targets anyway.  Just using --print-database by itself won't do it.
 Using the -n option will help, but many makefiles are not carefully
written to ensure that -n is really idempotent, and make -n could
delete files or similar operations.  And of course this still only
finds the targets that are available "by default"; providing a target
on the command line could cause more pattern rules to generate more
targets that the "default" goal target doesn't.

I hope that helps but if I completely missed the point please feel free
to redirect me!

Cheers, and stay safe;
Paul

bug#42411: Bug with M-x compile

[Eli Zaretskii]

> From: Paul Smith <psmith@gnu.org>
> Cc: 42411@debbugs.gnu.org
> Date: Fri, 31 Jul 2020 14:20:38 -0400
> 
> It's definitely true that technically, it is possible to have targets
> that are indented by TABs.  A line indented by a TAB is only considered
> part of a recipe if it appears in the "recipe context", which means
> somewhere that a recipe is legal in the syntax.
> 
> If it's not legal for a recipe command to appear there then TABs are
> treated like any other whitespace.
> 
> In practice, I think it's highly unlikely that anyone would
> intentionally use TABs to indent targets because it's so fragile: any
> reordering of the makefile or adding new lines could cause that
> makefile to break.
> 
> So, as a simplifying assumption it makes sense to me to ignore any line
> starting with TAB when trying to detect targets.
> 
> Of course, as Eli points out there are certainly a large number of
> potential targets which cannot be determined using this type of simple
> investigation.  The most obvious are targets that match patterns.
> 
> However I'll say two things about this:
> 
> First, I think it's unlikely that users would really want to see all
> the potential matches of targets when doing completion.  It's most
> likely that they are interested in the "top level" intended command
> line goals rather than every possible object, source, etc. file that
> make considers a target due to pattern or suffix rules.
> 
> Second, I don't think there's currently any good way to list those
> targets anyway.  Just using --print-database by itself won't do it.
>  Using the -n option will help, but many makefiles are not carefully
> written to ensure that -n is really idempotent, and make -n could
> delete files or similar operations.  And of course this still only
> finds the targets that are available "by default"; providing a target
> on the command line could cause more pattern rules to generate more
> targets that the "default" goal target doesn't.
> 
> I hope that helps but if I completely missed the point please feel free
> to redirect me!

Thanks for the feedback.

So you think the current regexp is trying to match too much, and the
proposed change is TRT and we should make it unconditionally?

bug#42411: Bug with M-x compile

[Paul Smith]

On Fri, 2020-07-31 at 21:42 +0300, Eli Zaretskii wrote:
> So you think the current regexp is trying to match too much, and the
> proposed change is TRT and we should make it unconditionally?

I think so yes.

bug#42411: Bug with M-x compile

[Lars Ingebrigtsen]

Gregory Heytings <ghe@sdf.org> writes:

>    (while (re-search-forward
> -         "^\\s-*\\([^\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
> +          "^\\([^\t\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
>      (setq targets (nconc (split-string (match-string-no-properties 1))

Paul Smith <psmith@gnu.org> writes:

> On Fri, 2020-07-31 at 21:42 +0300, Eli Zaretskii wrote:
>> So you think the current regexp is trying to match too much, and the
>> proposed change is TRT and we should make it unconditionally?
>
> I think so yes.

OK; I've now applied Gregory's patch to Emacs 28 (after checking a bit).

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