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

Allow additional constraints in typeclass methods #1343

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

Conversation

Jason94
Copy link

@Jason94 Jason94 commented Jan 5, 2025

This pull request allows typeclass methods to provide additional constraints on the typeclass variables within individual method definitions.

The motivating use case is developing the collections library, to allow for a hierarchy of collection types so that Dictionary/Map like types are collections of tuples and so that linear collections like Vectors/Lists have the same interface as maps of UFix's.

The following is currently compiling (on this branch that fixes a bug):

  (define-class (Collection :m :a (:m -> :a)))

  (define-class (Collection :m (Tuple :k :v) => KeyedCollection :m :k :v (:m -> :k :v)))

  (define-class (KeyedCollection :m UFix :a => LinearCollection_ :m :a (:m -> :a))))

However, some collection functions rely on an Eq instance for the contained type. Under Coalton's current limitations on method constraints, supporting these functions requires creating an additional typeclass:

  (define-class ((Eq :a) (Collection :m :a) => EqCollection :m :a (:m -> :a)))

This PR follows a discussion on the best way to approach this problem in the Discord.


The actual change is small. This was essentially already supported, most of what needed to be done was removing checks against this use case.

See the tests in the commit for examples of what will/won't compile.

@Jason94 Jason94 force-pushed the typeclass-enhancements branch 2 times, most recently from 5792c29 to 82a0f16 Compare January 6, 2025 00:13
@stylewarning
Copy link
Member

@Jason94 Can you investigate the CCL errors? Allegro errors can be ignored.

@Jason94
Copy link
Author

Jason94 commented Jan 6, 2025

@Jason94 Can you investigate the CCL errors? Allegro errors can be ignored.

CCL & SBCL tests passing now 👍

@stylewarning
Copy link
Member

@eliaslfox Do you see any issue removing this extra check?

@@ -402,19 +402,6 @@
(tc-note method
"the method is ambiguous"))

;; Ensure that the type variables in each
Copy link
Author

@Jason94 Jason94 Jan 6, 2025

Choose a reason for hiding this comment

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

Found a regression. Previously this was typechecking, and now it's failing to unify. Fix and add a test case.

  (define-class (Collection :m :a (:m -> :a))
    (new-convert
     "Convert a collection of another type. If converting a LinearCollection to a LinearCollection, should preserve order."
     ((Collection :f :a) (itr:IntoIterator :f :a) => :f -> :m)))

    (define-instance (Collection (List :a) :a)
      (define (new-convert coll)
        (itr:collect! (itr:into-iter coll))))
;     Unknown instance for predicate ITR:INTOITERATOR #T25109 #T25379
;
;     (in form starting at line: 308, column: 0, position: 10581)

Copy link
Author

Choose a reason for hiding this comment

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

This seems to be the minimal code needed to reproduce it the error. I'm not sure if it is because of the functional dependencies or having multiple typeclass parameters:

Compiles:

  (define-type TestType X Y)

  (define-class (A :a)
    (id-a (:a -> :a)))

  (define-class (B :b)
    (a-to-b (A :a => :a -> :b -> :b)))

  (define-instance (A TestType)
    (define id-a id))

  (define-instance (B TestType)
    (define (a-to-b _ b) b))

Doesn't compile:

  (define-type (Box :a)
    (Box :a))

  (define-class (ClassA :m :a (:m -> :a))
    (get-a (:m -> :a)))

  (define-class (ClassB :m :a (:m -> :a))
    (convert (ClassA :n :a => :n -> :m)))

  (define-instance (ClassA (Box :a) :a)
    (define (get-a (Box a)) a))

  (define-instance (ClassB (Box :a) :a)
    (define (convert bx)
      (Box (get-a bx))))
==>
Compile-time error:
  (during macroexpansion of (COALTON-TOPLEVEL
  (DEFINE-TYPE #
    ...)
  ...))
Unknown instance for predicate CLASSA #T23431 #T23442

Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

changing my review status until a little more testing is done

@stylewarning stylewarning force-pushed the typeclass-enhancements branch from 5371386 to 5c900be Compare January 13, 2025 16:54
@stylewarning
Copy link
Member

@Jason94 I just rebased this; sorry I should have asked first. Is this PR ready for (re-)review?

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.

2 participants