guicho271828 / trivia

Pattern Matcher Compatible with Optima
Other
332 stars 22 forks source link

The assoc pattern appears to mistakenly match anything at all, regardless of whether or not it should #52

Closed dfmorrison closed 8 years ago

dfmorrison commented 8 years ago

The assoc pattern appears to mistakenly match anything at all. It only binds the variable when it should match, but still pretends it did match even when it shouldn't have with the variable bound to nil.

CL-USER> (values (machine-type) (machine-version) (software-type) (software-version)
                 (lisp-implementation-type) (lisp-implementation-version))
"x86_64"
"Intel(R) Core(TM) i7 CPU       M 620  @ 2.67GHz"
"Linux"
"4.4.0-31-generic"
"Clozure Common Lisp"
"Version 1.11-r16635  (LinuxX8664)"
CL-USER> (asdf:component-version (asdf:find-system :trivia nil))
"0.1"
CL-USER> (defun ft (x)
           (trivia:match x
             ((assoc :foo val) (list 'found val))))
FT
CL-USER> (ft '((:foo . 1)))
(FOUND 1)
CL-USER> (ft '((foo . 2)))
(FOUND NIL)
CL-USER> (ft '(("foo". 3)))
(FOUND NIL)
CL-USER> (ft '((0 . 4)))
(FOUND NIL)

Compare this to the behavior of Optima, which seems to get it right:

CL-USER> (asdf:component-version (asdf:find-system :optima nil))
"1.0"
CL-USER> (defun fo (x)
           (optima:match x
             ((assoc :foo val) (list 'found val))))
FO
CL-USER> (fo '((:foo . 1)))
(FOUND 1)
CL-USER> (fo '((foo . 2)))
NIL
CL-USER> (fo '(("foo". 3)))
NIL
CL-USER> (fo '((0 . 4)))
NIL
guicho271828 commented 8 years ago

This should be fixed, thanks.

guicho271828 commented 8 years ago

fix commited, being CI tested.

guicho271828 commented 8 years ago

CI test passed, merging.

PuercoPop commented 8 years ago

@guicho271828 The documentation should be updated to reflect this change. Currently the docstring and wiki says it matches any list.

dfmorrison commented 8 years ago

​​On Fri, Jul 29, 2016 at 2:38 AM, Javier Olaechea notifications@github.com wrote:

@dfmorrison This works as documented, in the docstring and wiki, in trivia. So this is a suggestion for a behavior change for compatibility with optima?

My apologies, I was seduced by Trivia describing itself as a drop-in replacement for Optima, and it never occurred to me that the assoc pattern might deliberately be different between the two.

While I've not tried the latest, I gather Trivia has now be changed to have Optima's semantics for assoc. If so, thank you, I do think this is preferable, on several grounds, in addition to compatibility:

Don Morrison dfm@ringing.org "All good theoretical physicists put this number [the fine structure constant] up on their wall and worry about it." -- Richard Feynman, QED: The Strange Theory of Light and Matter

guicho271828 commented 8 years ago

@dfmorrison , this is clearly a bug, so I fixed it. Also, if my description gives you the misleading impression about the compatibility, I'm sorry. Apart from this, however, let me clarify the following:

It is always good to have suggestions. Also, in general, newly added test cases can be backported to Optima. Thank you.

guicho271828 commented 8 years ago

Well, rethinking --- I did made some changes to assoc pattern test cases. hmm...

guicho271828 commented 8 years ago

Anyways, I am trying to be as consistent as possible. Also, check for incompatibility in suite.lisp. semantic changes are in assoc and property patterns only (which is about if the key is evaluated or not -- not related to this issue). fail is just moved to the other package and 1-pass compilation issue is transparent to the user.