bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

I mistakenly had not removed an old version of a pcase clause that I
changed, so one of the two was redundant.  Compiling showed the
redundancy message in *Messages*.

Compiling also showed a message in *Compile Log* saying that a lexical
variable, which occurred only in the second of the clauses with
redundant patterns, was unused.

But the interpreted code worked as I wanted.  Apparently it was the
first of the two clauses (not the second, which had the "unused" lexical
variable) that was ignored when interpreting.  That lexical variable was
used when interpreting.

Can the messaging be improved, to tell you what the effect will be of
the redundant clauses when using the byte-compiled code, versus what it
might be for the interpreted code?  Can the compiler tell a user, for
example, which of a set of clauses with redundancy will (or will not) be
used?

The current messaging seems a bit confusing/incomplete.  It pointed to
the second of the two clauses, which was OK, and which had the "unused"
lexical variable (which was in fact used, at least when interpreting),
saying that it was redundant.

Can the messaging at least tell you (1) all of a set of clauses that are
mutually redundant and (2) which one of them will actually be used by
the compiled code, the others presumably having been pruned?

These are the two clauses in question:

 (`,a `(not,a))
 ((and a (guard (not recursivep))) `(not ,a))

Variable RECURSIVEP is an argument passed to the function that invokes
the `pcase'.  The second clause is the good one.  It is the one that was
flagged as redundant and having an unused lexical variable.  Is it the
clause that gets pruned when compiling?  When interpreting it seems like
the first of the two clauses is not used.

In GNU Emacs 27.0.50 (build 3, x86_64-w64-mingw32)
 of 2018-03-21
Repository revision: e70d0c9e66d7a8609450b2889869d16aeb0363b5
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install -C 'CFLAGS=-O2 -static -g3''

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Michael Heerdegen]

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

> But the interpreted code worked as I wanted.  Apparently it was the
> first of the two clauses (not the second, which had the "unused" lexical
> variable) that was ignored when interpreting.  That lexical variable was
> used when interpreting.

Are you sure?  I get

(macroexpand-1
 '(pcase foo
    (`,a `(not ,a))
    ((and a (guard (not recursivep))) `(not ,a))))

==>
 (let ((a foo))
  `(not ,a))

and when running the pcase form, the lexical binding of RECURSIVEP is
ignored for me, e.g.

(let ((recursivep t))
  (pcase 1
    (`,a `(not ,a))
    ((and a (guard (not recursivep))) `(not ,a))))
==> (not 1)   (instead of nil)


Michael.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

> > But the interpreted code worked as I wanted.  Apparently it was the
> > first of the two clauses (not the second, which had the "unused"
> > lexical variable) that was ignored when interpreting.  That lexical 
> > variable was used when interpreting.
> 
> Are you sure?

No, not at this point.

> I get
> (macroexpand-1
>  '(pcase foo
>     (`,a `(not ,a))
>     ((and a (guard (not recursivep))) `(not ,a))))
> ==>
>  (let ((a foo))
>   `(not ,a))
> 
> and when running the pcase form, the lexical binding of RECURSIVEP is
> ignored for me, e.g.
> 
> (let ((recursivep t))
>   (pcase 1
>     (`,a `(not ,a))
>     ((and a (guard (not recursivep))) `(not ,a))))
> ==> (not 1)   (instead of nil)

OK, thanks.  The rest of the bug report stands, though.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Lars Ingebrigtsen]

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

> I mistakenly had not removed an old version of a pcase clause that I
> changed, so one of the two was redundant.  Compiling showed the
> redundancy message in *Messages*.

[...]

> Can the messaging at least tell you (1) all of a set of clauses that are
> mutually redundant and (2) which one of them will actually be used by
> the compiled code, the others presumably having been pruned?
>
> These are the two clauses in question:
>
>  (`,a `(not,a))
>  ((and a (guard (not recursivep))) `(not ,a))

Can you post a complete test case that demonstrates the problem?

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

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > These are the two clauses in question:
> >
> >  (`,a `(not,a))
> >  ((and a (guard (not recursivep))) `(not ,a))
>
> Can you post a complete test case that demonstrates the problem?

Well, when we compile something like

(defun test (foo recursivep)
  (pcase foo
    (a `(not ,a))
    ((and a (guard recursivep))
     `(not ,a))))

the message Drew means says

  Redundant pcase pattern: (and a (guard recursivep))

I'm happy with that, I think it tells anything I need to know.  I'm not
happy about the fact that I missed the `message' because it's only a
message and I only see it when I look into the *Messages* buffer because
it's overwritten very soon in the echo area.

The compiler log only shows

Compiling file /home/micha/today/pctest.el at Sun Oct  4 00:44:51 2020
pctest.el:46:1: Warning: Unused lexical argument `recursivep'

which is confusing if you missed the message about the redundant
pattern.

Michael.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

> Well, when we compile something like
> 
> (defun test (foo recursivep)
>   (pcase foo
>     (a `(not ,a))
>     ((and a (guard recursivep))
>      `(not ,a))))
> 
> the message Drew means says
> 
>   Redundant pcase pattern: (and a (guard recursivep))
> 
> I'm happy with that, I think it tells anything I need to know.  I'm not
> happy about the fact that I missed the `message' because it's only a
> message and I only see it when I look into the *Messages* buffer because
> it's overwritten very soon in the echo area.
> 
> The compiler log only shows
> 
> Compiling file /home/micha/today/pctest.el at Sun Oct  4 00:44:51 2020
> pctest.el:46:1: Warning: Unused lexical argument `recursivep'
> 
> which is confusing if you missed the message about the redundant
> pattern.

The ephemeral (so hidden) message is another problem.

I'll repeat the request, which is what I think a user
would like to know:

  Can the messaging at least tell you:
  (1) all of a set of clauses that are mutually redundant and
  (2) which one of them will actually be used by the compiled
      code, the others presumably having been pruned?

IOW, what's the actual effect, for users?  How does pcase
deal with the redundancy?  Can that at least be documented
somewhere (maybe it is already)?

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Michael Heerdegen]

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

> I'll repeat the request, which is what I think a user
> would like to know:
>
>   Can the messaging at least tell you:
>   (1) all of a set of clauses that are mutually redundant and
>   (2) which one of them will actually be used by the compiled
>       code, the others presumably having been pruned?
>
> IOW, what's the actual effect, for users?  How does pcase
> deal with the redundancy?  Can that at least be documented
> somewhere (maybe it is already)?

I think you make an error in reasoning: we don't have mutual redundancy
here.  A case can be redundant when it will never match because whenever
would match, a previous case in the cases list always matches, so it is
effectively shadowed.  But this is not symmetric (cases are always tried
from first to last (!), and e.g. a `_' catchall pattern in one case will
make all following cases redundant, but not the other way round).

I think the message is more like a warning that a case can never match,
and in all cases where this happened to me, as also in your case, the
reason was a very obvious editing mistake.  I don't think there is much
to say here, Emacs just tells you: this case here will never be used,
look what you have done wrong.  There is a problem with your code
whenever you see that message.  And there is nothing to say about the
semantics as well.

BTW, I think the implementation only covers the most obvious and
simplistic cases, like those involving catchall patterns or duplicated
patterns.


Michael.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

> > I'll repeat the request, which is what I think a user
> > would like to know:
> >
> >   Can the messaging at least tell you:
> >   (1) all of a set of clauses that are mutually redundant and
> >   (2) which one of them will actually be used by the compiled
> >       code, the others presumably having been pruned?
> >
> > IOW, what's the actual effect, for users?  How does pcase
> > deal with the redundancy?  Can that at least be documented
> > somewhere (maybe it is already)?
> 
> I think you make an error in reasoning: we don't have mutual redundancy
> here.  A case can be redundant when it will never match because whenever
> would match, a previous case in the cases list always matches, so it is
> effectively shadowed.  But this is not symmetric (cases are always tried
> from first to last (!), and e.g. a `_' catchall pattern in one case will
> make all following cases redundant, but not the other way round).
> 
> I think the message is more like a warning that a case can never match,
> and in all cases where this happened to me, as also in your case, the
> reason was a very obvious editing mistake.  I don't think there is much
> to say here, Emacs just tells you: this case here will never be used,
> look what you have done wrong.  There is a problem with your code
> whenever you see that message.  And there is nothing to say about the
> semantics as well.
> 
> BTW, I think the implementation only covers the most obvious and
> simplistic cases, like those involving catchall patterns or duplicated
> patterns.

If what you say is the case then the help should tell
users that.  To me it's not obvious.

And even if a user (somehow) understands that later
cases are made redundant by earlier ones, how to tell
which earlier ones are implicated?

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I think the message is more like a warning that a case can never match,
> and in all cases where this happened to me, as also in your case, the
> reason was a very obvious editing mistake.  I don't think there is much
> to say here, Emacs just tells you: this case here will never be used,
> look what you have done wrong.

Perhaps the warning should be more explicit?  "Redundant pcase pattern"
is obvious if you already know what it means, but if you don't, it's
not.  :-)  So something like "pcase pattern shadowed by previous pcase
pattern" or something along those lines?

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

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

> Perhaps the warning should be more explicit?  "Redundant pcase pattern"
> is obvious if you already know what it means, but if you don't, it's
> not.  :-)  So something like "pcase pattern shadowed by previous pcase
> pattern" or something along those lines?

That's a start.  But as I said:

  And even if a user (somehow) understands that later
  cases are made redundant by earlier ones, how to tell
  which earlier ones are implicated?

Just saying that some previous pcase pattern makes
this one redundant doesn't tell you which previous
pattern that does that.  It should.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Perhaps the warning should be more explicit?  "Redundant pcase pattern"
> is obvious if you already know what it means, but if you don't, it's
> not.  :-)  So something like "pcase pattern shadowed by previous pcase
> pattern" or something along those lines?

Yeah, a better wording would be nice.

Michael.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Michael Heerdegen]

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

> Just saying that some previous pcase pattern makes
> this one redundant doesn't tell you which previous
> pattern that does that.  It should.

Note that the message is probably just a byproduct produced by the
current implementation, it's nice to have, but it's limited and might
vanish in the future.  I don't know if the the code knows which of the
branches is the shadowing one.  If I have the time I might have a look.

Michael.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Perhaps the warning should be more explicit?  "Redundant pcase pattern"
> is obvious if you already know what it means, but if you don't, it's
> not.  :-)  So something like "pcase pattern shadowed by previous pcase
> pattern" or something along those lines?

I've now done this in Emacs 28, and I'm closing this bug report.  There
was some talk about changing this warning in other ways, and if that's
something people want to pursue, opening a new bug report for that would
be fine.

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

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Drew Adams]

> I've now done this in Emacs 28, and I'm closing this bug report.  There
> was some talk about changing this warning in other ways, and if that's
> something people want to pursue, opening a new bug report for that would
> be fine.

I can't tell just what was fixed and what might not
have been, and the thread is too long to dig into now.

If something was reported that hasn't yet been fixed,
and you're aware of that, it would be great if you
filed the bug report for that.

It's good to keep a record of reports of outstanding
problems, as well as to keep a record of suggested
improvements.

Thx.

bug#31350: 27.0; `pcase' message: "Redundant pcase pattern"

[Eli Zaretskii]

> Date: Sat, 12 Dec 2020 09:00:34 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 31350@debbugs.gnu.org
> 
> > I've now done this in Emacs 28, and I'm closing this bug report.  There
> > was some talk about changing this warning in other ways, and if that's
> > something people want to pursue, opening a new bug report for that would
> > be fine.
> 
> I can't tell just what was fixed and what might not
> have been

You certainly can, if you look at the changes committed by Lars.  I've
shown the URL for doing that several times in the past.