bug#20784: 25.0.50; pcase documentation on t and nil

bug#20784: 25.0.50; pcase documentation on t and nil

[Artur Malabarba]

In pcase's docstirng, the second line of pcase patterns says:

SELFQUOTING matches itself. This includes keywords, numbers, and strings.

However, that doesn't apply to t and nil, instead, they are covered by
the SYMBOL clause.
I've always thought of t and nil as self-quoting, but if that's not
the case, I think the docstring should explicitly state:

SELFQUOTING matches itself. This includes keywords, numbers, and
strings (but not t or nil).

bug#20784: 25.0.50; pcase documentation on t and nil

[Stefan Monnier]

> SELFQUOTING matches itself. This includes keywords, numbers, and strings.

> However, that doesn't apply to t and nil, instead, they are covered by
> the SYMBOL clause.

I think the problem is in the wording: t and nil are neither keywords,
nor numbers, nor strings, which is why they aren't treated as
SELFQUOTING.  IOW the wording shouldn't say "including" but make it
clear that these are the (currently) only accepted selfquoting entities.
E.g. vectors (and friends) aren't considered as SELFQUOTING by
pcase either.


        Stefan

bug#20784: 25.0.50; pcase documentation on t and nil

[Artur Malabarba]

I've also had reports that (pcase something ('nil 1)) complains of
invalid pattern in 24.5, while it works fine for me in 25.0.
Is there a proper way to match nil? If I just use nil, I get errors on
25, but using 'nil gives errors on 24.5.

bug#20784: 25.0.50; pcase documentation on t and nil

[Michael Heerdegen]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

> I've also had reports that (pcase something ('nil 1)) complains of
> invalid pattern in 24.5, while it works fine for me in 25.0.
> Is there a proper way to match nil? If I just use nil, I get errors on
> 25, but using 'nil gives errors on 24.5.

AFAICT, `nil is the correct way.

Michael.

bug#20784: 25.0.50; pcase documentation on t and nil

[Stefan Monnier]

> I've also had reports that (pcase something ('nil 1)) complains of
> invalid pattern in 24.5, while it works fine for me in 25.0.

IIRC the "quote" pattern is new in Emacs-25.  You have to use backquote
patterns instead in Emacs<25.


        Stefan

bug#20784: 25.0.50; pcase documentation on t and nil

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> > SELFQUOTING matches itself. This includes keywords, numbers, and
> > strings.
>
> > However, that doesn't apply to t and nil, instead, they are covered by
> > the SYMBOL clause.
>
> I think the problem is in the wording: t and nil are neither keywords,
> nor numbers, nor strings, which is why they aren't treated as
> SELFQUOTING.  IOW the wording shouldn't say "including" but make it
> clear that these are the (currently) only accepted selfquoting entities.
> E.g. vectors (and friends) aren't considered as SELFQUOTING by
> pcase either.

Just an opinion from another user:

I think the semantic of SELFQUOTING is so non-obvious and non-intuitive
that it's not worth it to confuse people who learn pcase with it, for
the sake that it's more or less the same as the pattern 'VAL, or a
subset from it:

SELFQUOTING applies to some atomic expressions that eval to themselves,
to others not.  SELFQUOTING "matches itself", which again depends on the
kind of thing.  It doesn't always mean `eq' (as the wording "itself"
suggests), because strings don't just match only themselves but any
other `equal' string.  But it also doesn't mean "equal" because in the
case of numbers, comparison is done with `eq':

(pcase 5.0
  (5 5)
  (5.0 5.0))  => 5.0

The case of t is especially non-obvious; it's neither treated as
SELFQUOTING nor as SYMBOL, but as a pattern matching anything.  While
`t' meaning "applies always" makes sense in `cond', because there the
right sides are interpreted as booleans, it can be IMHO confusing to
beginners in pcase because it could make you think that pcase is about
booleans, but it's about pattern matching.  I found myself trying stuff
like

  (pcase expr
    ((and x (symbolp x)) (symbol-name x))),

maybe having the `t' pattern in mind?

And `nil' OTOH is different again.  It is not interpreted as "applies
never", but raises an error when used  ("applies never" could make
sense when the code is generated by a macro).

I think I would consider to trivialize the semantic, merging the
SELFQUOTING and 'VAL case.

I would remove the SELFQUOTING thing as it is now.  I would keep the
semantic of 'VAL as it is now, as it is perfectly clear.  To substitute
the SELFQUOTING thing, we could allow something like ATOM - which can be
any atomic expression that is not a symbol that can be bound - and make
it simply an abbreviation for 'ATOM.  (FWIW, unifying `ATOM and 'ATOM
would IMHO also be worth it; currently 'ATOM is compared always with
`equal', whereby `ATOM is compared with `equal' for strings and `eq'
else.  You can always use guard when you want a different comparison
predicate.)

I know that would all would break existing code, but it would make pcase
easier to understand and to learn.

Just my two cents.


Regards,

Michael.

bug#20784: 25.0.50; pcase documentation on t and nil

[Stefan Monnier]

> (pcase 5.0
>   (5 5)
>   (5.0 5.0))  => 5.0

Hmm... indeed, I used "numbers" (and `numberp') in pcase, where I really
meant "integers".  That's a bug.  The above code should (IMO) signal an
error indicating that floats aren't accepted as patterns.  The intention
is that SELFQUOTING should always be matched with an `equal' semantics
(`eq' is used on integers and keywords as a plain optimization which
should not affect the semantics).
E.g. using `eq' on strings makes string patterns simply unusable.

> The case of t is especially non-obvious; it's neither treated as

Yes, it was a mistake to accept t to mean the same as `_'.
Not sure if it's too late to fix it, but t should not be accepted as
a special pattern.  We could handle it like nil (i.e. pretend it's
a normal variable which then signals an error when we try to let-bind
it) or emit a warning.

> never", but raises an error when used  ("applies never" could make
> sense when the code is generated by a macro).

We currently don't have a special "fail" pattern which simply never
matches.  Of course, you can make one up (e.g. (guard nil)).  I think
making nil such a pattern would probably lead to more errors/confusion
than anything, since it's extremely rare to need such a pattern.

> I think I would consider to trivialize the semantic, merging the
> SELFQUOTING and 'VAL case.

We could drop the SELFQUOTING case, but I added it because it lets you
reduce the clutter of quoting, which tends to be fairly high in
pcase patterns.

> semantic of 'VAL as it is now, as it is perfectly clear.  To substitute
> the SELFQUOTING thing, we could allow something like ATOM - which can be
> any atomic expression that is not a symbol that can be bound - and make
> it simply an abbreviation for 'ATOM.

That's pretty much what SELFQUOTING is.  IF you think renaming it to
ATOM would help, then we could do that.

> (FWIW, unifying `ATOM and 'ATOM would IMHO also be worth it;

They should behave identically, yes.

> currently 'ATOM is compared always with
> `equal', whereby `ATOM is compared with `equal' for strings and `eq'
> else.

As explained the use of `eq' is supposed to be a pure optimization with
no semantics effect.  The fact that we accept floats and compare them
with `eq' is a plain bug.


        Stefan

bug#20784: 25.0.50; pcase documentation on t and nil

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> We currently don't have a special "fail" pattern which simply never
> matches.  Of course, you can make one up (e.g. (guard nil)).

(or) would also do it, but raises (error "Please avoid it")  (BTW
(or _) raises that error, too).  Why are those (error "Please avoid it")
calls useful?

> I think making nil such a pattern would probably lead to more
> errors/confusion than anything, since it's extremely rare to need such
> a pattern.

Agreed.

> That's pretty much what SELFQUOTING is.  IF you think renaming it to
> ATOM would help, then we could do that.

I would prefer that over SELFQUOTING because

  SELFQUOTING	is a shorthand for 'SELFQUOTING.

sounds like a tautology.

> As explained the use of `eq' is supposed to be a pure optimization with
> no semantics effect.

I see.  Then I guess it would make sense to merge these two lines in the
` doc?

  STRING                matches if the object is ‘equal’ to STRING.
  ATOM                  matches if the object is ‘eq’ to ATOM.

into

  ATOM                  matches if the object is ‘equal’ to ATOM.


Regards,

Michael.

bug#20784: 25.0.50; pcase documentation on t and nil

[Stefan Monnier]

>> We currently don't have a special "fail" pattern which simply never
>> matches.  Of course, you can make one up (e.g. (guard nil)).
> (or) would also do it, but raises (error "Please avoid it")  (BTW
> (or _) raises that error, too).  Why are those (error "Please avoid it")
> calls useful?

IIRC handling them well (i.e. generate efficient code) required more
effort than I was willing to invest, or something like that, and IIRC
they should only show up in unusual cases, so I made them signal errors,
to try and see when/where those cases show up.
So far I never bumped into them and haven't received any bug report
about them either.  We should probably remove them, tho, because those
corner cases may be annoying to avoid when doing metaprogramming.

>> That's pretty much what SELFQUOTING is.  IF you think renaming it to
>> ATOM would help, then we could do that.
> I would prefer that over SELFQUOTING

OK, changed.

> because
>   SELFQUOTING	is a shorthand for 'SELFQUOTING.
> sounds like a tautology.

As a teacher I was taught that redundancy in explanations is good ;-)

>> As explained the use of `eq' is supposed to be a pure optimization with
>> no semantics effect.

> I see.  Then I guess it would make sense to merge these two lines in the
> ` doc?

>   STRING                matches if the object is ‘equal’ to STRING.
>   ATOM                  matches if the object is ‘eq’ to ATOM.

Yes, thank you, done.


        Stefan

bug#20784: 25.0.50; pcase documentation on t and nil

[Stefan Monnier]

How 'bout the following patch?


        Stefan


   * lisp/emacs-lisp/pcase.el (pcase): Reword the SELFQUOTING case.
   (pcase--split-pred, pcase--self-quoting-p): Restrict selfquoting
   numbers to integers.
   (pcase--u1): Deprecate t patterns.  Reject nil as an invalid pattern.


diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index ab82b7e..a6994a6 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -112,11 +112,12 @@ CASES is a list of elements of the form (PATTERN CODE...).
 
 Patterns can take the following forms:
   _		matches anything.
-  SELFQUOTING	matches itself.  This includes keywords, numbers, and strings.
   SYMBOL	matches anything and binds it to SYMBOL.
   (or PAT...)	matches if any of the patterns matches.
   (and PAT...)	matches if all the patterns match.
   'VAL		matches if the object is `equal' to VAL
+  SELFQUOTING	is a shorthand for 'SELFQUOTING.
+		SELFQUOTING can be a keyword, an integer, or a string.
   (pred FUN)	matches if FUN applied to the object returns non-nil.
   (guard BOOLEXP)	matches if BOOLEXP evaluates to non-nil.
   (let PAT EXP)	matches if EXP matches PAT.
@@ -620,7 +621,7 @@ MATCH is the pattern that needs to be matched, of the form:
      ((and (eq 'pred (car upat))
            (eq 'quote (car-safe pat))
            (symbolp (cadr upat))
-           (or (symbolp (cadr pat)) (stringp (cadr pat)) (numberp (cadr pat)))
+           (or (symbolp (cadr pat)) (stringp (cadr pat)) (integerp (cadr pat)))
            (get (cadr upat) 'side-effect-free)
            (ignore-errors
              (setq test (list (funcall (cadr upat) (cadr pat))))))
@@ -638,7 +639,7 @@ MATCH is the pattern that needs to be matched, of the form:
     res))
 
 (defun pcase--self-quoting-p (upat)
-  (or (keywordp upat) (numberp upat) (stringp upat)))
+  (or (keywordp upat) (integerp upat) (stringp upat)))
 
 (defun pcase--app-subst-match (match sym fun nsym)
   (cond
@@ -770,7 +771,12 @@ Otherwise, it defers to REST which is a list of branches of the form
            (sym (car cdrpopmatches))
            (upat (cdr cdrpopmatches)))
       (cond
-       ((memq upat '(t _)) (pcase--u1 matches code vars rest))
+       ((memq upat '(t _))
+        (let ((code (pcase--u1 matches code vars rest)))
+          (if (eq upat '_) code
+            (macroexp--warn-and-return
+             "Pattern t is deprecated.  Use `_' instead"
+             code))))
        ((eq upat 'pcase--dontcare) :pcase--dontcare)
        ((memq (car-safe upat) '(guard pred))
         (if (eq (car upat) 'pred) (pcase--mark-used sym))
@@ -784,7 +790,7 @@ Otherwise, it defers to REST which is a list of branches of the form
                        (pcase--eval (cadr upat) vars))
                      (pcase--u1 matches code vars then-rest)
                      (pcase--u else-rest))))
-       ((symbolp upat)
+       ((and (symbolp upat) upat)
         (pcase--mark-used sym)
         (if (not (assq upat vars))
             (pcase--u1 matches code (cons (cons upat sym) vars) rest)
@@ -854,7 +860,7 @@ Otherwise, it defers to REST which is a list of branches of the form
                      (pcase--u rest))
                    vars
                    (list `((and . ,matches) ,code . ,vars))))
-       (t (error "Unknown internal pattern `%S'" upat)))))
+       (t (error "Unknown pattern `%S'" upat)))))
    (t (error "Incorrect MATCH %S" (car matches)))))
 
 (def-edebug-spec

bug#20784: 25.0.50; pcase documentation on t and nil

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> How 'bout the following patch?
>
>    * lisp/emacs-lisp/pcase.el (pcase): Reword the SELFQUOTING case.
>    (pcase--split-pred, pcase--self-quoting-p): Restrict selfquoting
>    numbers to integers.
>    (pcase--u1): Deprecate t patterns.  Reject nil as an invalid pattern.

Thanks, that looks good to me.  Comments in my message above.


Michael.

bug#20784: 25.0.50; pcase documentation on t and nil

[npostavs]

tags 20784 fixed
close 20784 25.1
quit

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> How 'bout the following patch?
>>
>>    * lisp/emacs-lisp/pcase.el (pcase): Reword the SELFQUOTING case.
>>    (pcase--split-pred, pcase--self-quoting-p): Restrict selfquoting
>>    numbers to integers.
>>    (pcase--u1): Deprecate t patterns.  Reject nil as an invalid pattern.
>
> Thanks, that looks good to me.  Comments in my message above.

Closing, since this appears to have been applied.