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

Deduplicate fundep environment entries #1082

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

jbouwman
Copy link
Collaborator

@jbouwman jbouwman commented Apr 3, 2024

Prevent addition of duplicate fundep entries by moving the no-match case inside the update block.

(A change to fundep environment type signature was proposed and rolled back.)

@jbouwman jbouwman changed the title Suppress duplicate fundep relations Simplify fundep environment metadata Apr 3, 2024
@eliaslfox
Copy link
Collaborator

eliaslfox commented Apr 3, 2024

I don't think this is correct, and I'm confused as to why the tests are passing.

The fundep environment was previously:

ImmutableMap<class, ImmutableMap<fundep-idx, ImmutableList<fundep-entry>>>

This pr changes it to:

ImmutableMap<class, List<fundep-entry>>

Knowing which relations are associated w/ which fundeps is be required. See the following example:

(in-package #:coalton-user)

(coalton-toplevel
  (define-class (C :a :b :c (:a -> :b) (:b -> :c)))
  
  (define-instance (C Integer String Double-Float)))

(in-package #:coalton-impl/typechecker/environment)

(lookup-fundep-environment coalton-impl/entry::*global-environment* 'coalton-user::C)

#S(IMMUTABLE-LISTMAP
   :DATA #{|
            (0
             #[
               #S(FUNDEP-ENTRY :FROM (COALTON:INTEGER) :TO (COALTON:STRING))
               #S(FUNDEP-ENTRY :FROM (COALTON:INTEGER) :TO (COALTON:STRING)) ])
            (1
             #[
               #S(FUNDEP-ENTRY
                  :FROM (COALTON:STRING)
                  :TO (COALTON:DOUBLE-FLOAT))
               #S(FUNDEP-ENTRY
                  :FROM (COALTON:STRING)
                  :TO (COALTON:DOUBLE-FLOAT)) ]) |}/#[ ])

Note how Integer -> String is attached to fundep #1 and String -> Double-Float is attached to fundep #2. I'm not sure about the duplicate entries, that looks like a bug lol.

@jbouwman jbouwman force-pushed the dedup-fundep-entries branch from 4c84d37 to bb28a82 Compare April 3, 2024 20:33
@jbouwman jbouwman changed the title Simplify fundep environment metadata Deduplicate fundep environment entries Apr 3, 2024
@jbouwman
Copy link
Collaborator Author

jbouwman commented Apr 3, 2024

I've reverted changes to fundep environment: this changeset only prevents duplicate entries.

Prevent addition of duplicate fundep entries by moving the no-match case
inside the update block.
@jbouwman jbouwman force-pushed the dedup-fundep-entries branch from bb28a82 to f4d07d0 Compare April 3, 2024 20:40
@eliaslfox eliaslfox merged commit 8c4db84 into coalton-lang:main Apr 3, 2024
17 checks passed
@jbouwman jbouwman deleted the dedup-fundep-entries branch September 27, 2024 22:38
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