Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coalton:match to codegen cl:case when possible #1399

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

garlic0x1
Copy link
Collaborator

No description provided.

(when (and (equalp (node-type (node-match-expr expr)) tc:*boolean-type*)
(= 2 (length (node-match-branches expr)))
(equalp (match-branch-pattern (first (node-match-branches expr)))
(make-pattern-constructor :type tc:*boolean-type* :name 'coalton:False :patterns nil))
Copy link
Collaborator

@YarinHeffes YarinHeffes Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we only codegen if when

(match bool
  ((True) ...)
  ((False) ...))

but not for

(match bool
  (True ...)
  (False ...))

I think this simple change could allow both:

Suggested change
(make-pattern-constructor :type tc:*boolean-type* :name 'coalton:False :patterns nil))
(or (make-pattern-constructor :type tc:*boolean-type* :name 'coalton:False :patterns nil))
(make-pattern-literal ...))

EDIT:

Actually, no, this won't work! In the main branch

(match True
  (True "hi")
  (False "bye"))

returns "hi"... but so does this:

(match True
  (False "hi")
  (True "bye"))

because False is being read as a variable!

So actually, I think we should emit an "unused variable" error in this case. And, that's a separate issue not related to this PR.

EDIT no. 2:

I think this problem only creates confusion for Enums and Booleans, so your PR will actually fix this, and if this is merged, I don't think there is a reason to open a separate issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a long-standing issue that pattern variables can be confused as constructors by the programmer and we emit no good warning for this.

`(the ,(tc:lisp-type (node-type (node-match-expr expr)) env) ,subexpr)
subexpr)))
(declare (ignorable ,match-var))
(case ,(codegen-expression (node-match-expr expr) env)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be

Suggested change
(case ,(codegen-expression (node-match-expr expr) env)
(case ,subexpr

or even

Suggested change
(case ,(codegen-expression (node-match-expr expr) env)
(case ,match-var

(let* ((name (pattern-constructor-name pattern))
(entry (tc:lookup-constructor env name)))
`(,(tc:constructor-entry-compressed-repr entry)
,expr)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a bad error message for this example

(coalton-toplevel
  (repr :enum)
  (define-type MyEnum A B C))

(coalton
  (match A
    (A "hi")
    (B "bye")))

Because the patterns are read as pattern-vars and fall into t, and codegen otherwise for both of them:

(COMMON-LISP:LET ((#:MATCH4047
                   (COMMON-LISP:THE
                    (COMMON-LISP:MEMBER MYENUM/D MYENUM/C MYENUM/B MYENUM/A)
                    A)))
  (COMMON-LISP:DECLARE (COMMON-LISP:IGNORABLE #:MATCH4047))
  (COMMON-LISP:CASE A
    (COMMON-LISP:OTHERWISE
     (COMMON-LISP:LET ((A-3464 #:MATCH4047))
       "hi"))
    (COMMON-LISP:OTHERWISE
     (COMMON-LISP:LET ((B-3465 #:MATCH4047))
       "bye"))))

I think there should be a check to ensure that a variable or _ is only passed in the last branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add this check (it makes sense) we should do this before codegen.

,@(unless (member-if (lambda (pat)
(or (pattern-wildcard-p pat)
(pattern-var-p pat)))
(node-match-branches expr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a check is done to ensure that a variable or _ is only passed in the last branch (see comment above), then this check here can just be a check on the last branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants