Use less unsafeCoerce #321

treeowl · 2019-07-09T01:18:49Z

Get rid of a large fraction of calls to unsafeCoerce. Several
of the ones that remain are removed in my merge generalization
PR.

Get rid of a large fraction of calls to `unsafeCoerce`. Several of the ones that remain are removed in my merge generalization PR.

Ericson2314 · 2019-07-09T01:50:55Z

src/Reflex/Class.hs

@@ -296,9 +297,14 @@ class ( MonadHold t (PushM t)
  -- | Construct a 'Coercion' for a 'Dynamic' given an 'Coercion' for its
  -- occurrence type
  dynamicCoercion :: Coercion a b -> Coercion (Dynamic t a) (Dynamic t b)
+  -- | Construct a 'Coercion' for an 'Incremental'' given an 'Coercion' for its
+  -- occurrence type
+  incrementalCoercion :: Coercion t1 t2 -> Coercion p1 p2 -> Coercion (Incremental' t t1 p1) (Incremental' t t2 p2)


I take it something goes wrong if you do this instead?

incrementalCoercion :: Coercion (PatchTarget p1) (PatchTarget p2) -> Coercion p1 p2 -> Coercion (Incremental t p1) (Incremental t p2)

That's a very good question. That probably will work, and it's much less horrible.

Ericson2314 · 2019-07-09T01:54:53Z

src/Reflex/Profiled.hs

+  updatedIncremental (Incr (Incremental_Profiled i)) = coerce $ profileEvent $ updatedIncremental (Incr i)
+  incrementalToDynamic (Incr (Incremental_Profiled i)) = coerce $ incrementalToDynamic (Incr i)
+  behaviorCoercion (c :: Coercion a b) =
+    Coercion `trans` (behaviorCoercion c :: Coercion (Behavior t a) (Behavior t b)) `trans` Coercion


Can you do

case fooCoercion c :: ... of Coercion -> Coercion

for all of these to make GHC work more?

Can you give an example? Facing these somewhat involved coercion types that I couldn't prove directly with Coercion, I reached for the toolbox. It should certainly be possible to do it all with case, but I don't know if it'll be any nicer. You're more than welcome to try!

Oh, I think I see what you mean now. Coercion . fancy . Coercion should be case fancy of Coercion -> Coercion. But it seems potentially less maintainable.

Yeah that's what I mean. I was thinking more maintainable, since GHC can figure different things out there as needed. I guess I'm also bullish in GHC becoming smarter in the future. (I'm planning on writing a proposal for getting rid of roles.)

Ericson2314 · 2019-07-09T02:05:41Z

src/Reflex/Profiled.hs

@@ -144,16 +145,32 @@ instance Reflex t => Reflex (ProfiledTimeline t) where
  unsafeBuildIncremental (ProfiledM a0) (Event_Profiled a') = coerce $ unsafeBuildIncremental a0 a'
  mergeIncremental = Event_Profiled . mergeIncremental . (unsafeCoerce :: Incremental (ProfiledTimeline t) (PatchDMap k (Event (ProfiledTimeline t))) -> Incremental t (PatchDMap k (Event t)))
  mergeIncrementalWithMove = Event_Profiled . mergeIncrementalWithMove . (unsafeCoerce :: Incremental (ProfiledTimeline t) (PatchDMapWithMove k (Event (ProfiledTimeline t))) -> Incremental t (PatchDMapWithMove k (Event t)))


I think these can be derived with:

case (incrementalCoercion, eventCoercion) of (Coercion, Coercion) -> Coercion

I'd certainly hope the newtype coercion axioms are defined for data families too!

treeowl · 2019-07-09T02:33:27Z

I'll have to work through this again tomorrow.

…

On Mon, Jul 8, 2019, 10:22 PM John Ericson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Reflex/Profiled.hs <#321 (comment)>: > @@ -144,16 +145,32 @@ instance Reflex t => Reflex (ProfiledTimeline t) where unsafeBuildIncremental (ProfiledM a0) (Event_Profiled a') = coerce $ unsafeBuildIncremental a0 a' mergeIncremental = Event_Profiled . mergeIncremental . (unsafeCoerce :: Incremental (ProfiledTimeline t) (PatchDMap k (Event (ProfiledTimeline t))) -> Incremental t (PatchDMap k (Event t))) mergeIncrementalWithMove = Event_Profiled . mergeIncrementalWithMove . (unsafeCoerce :: Incremental (ProfiledTimeline t) (PatchDMapWithMove k (Event (ProfiledTimeline t))) -> Incremental t (PatchDMapWithMove k (Event t))) - currentIncremental (Incremental_Profiled i) = coerce $ currentIncremental i - updatedIncremental (Incremental_Profiled i) = coerce $ profileEvent $ updatedIncremental i - incrementalToDynamic (Incremental_Profiled i) = coerce $ incrementalToDynamic i - behaviorCoercion (c :: Coercion a b) = case behaviorCoercion c :: Coercion (Behavior t a) (Behavior t b) of - Coercion -> unsafeCoerce (Coercion :: Coercion (Behavior (ProfiledTimeline t) a) (Behavior (ProfiledTimeline t) a)) --TODO: Figure out how to make this typecheck without the unsafeCoerce - eventCoercion (c :: Coercion a b) = case eventCoercion c :: Coercion (Event t a) (Event t b) of - Coercion -> unsafeCoerce (Coercion :: Coercion (Event (ProfiledTimeline t) a) (Event (ProfiledTimeline t) a)) --TODO: Figure out how to make this typecheck without the unsafeCoerce - dynamicCoercion (c :: Coercion a b) = case dynamicCoercion c :: Coercion (Dynamic t a) (Dynamic t b) of - Coercion -> unsafeCoerce (Coercion :: Coercion (Dynamic (ProfiledTimeline t) a) (Dynamic (ProfiledTimeline t) a)) --TODO: Figure out how to make this typecheck without the unsafeCoerce - mergeIntIncremental = Event_Profiled . mergeIntIncremental . (unsafeCoerce :: Incremental (ProfiledTimeline t) (PatchIntMap (Event (ProfiledTimeline t) a)) -> Incremental t (PatchIntMap (Event t a))) + currentIncremental (Incr (Incremental_Profiled i)) = coerce $ currentIncremental (Incr i) + updatedIncremental (Incr (Incremental_Profiled i)) = coerce $ profileEvent $ updatedIncremental (Incr i) + incrementalToDynamic (Incr (Incremental_Profiled i)) = coerce $ incrementalToDynamic (Incr i) + behaviorCoercion (c :: Coercion a b) = + Coercion `trans` (behaviorCoercion c :: Coercion (Behavior t a) (Behavior t b)) `trans` Coercion Yeah that's what I mean. I was thinking *more* maintainable, since GHC can figure different things out there as needed. I guess I'm also bullish in GHC becoming smarter in the future. (I'm planning on writing a proposal for getting rid of roles.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321?email_source=notifications&email_token=AAOOF7ITYPHHNBQJFNJNCPDP6PY7LA5CNFSM4H7AVU2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5Z3IHI#discussion_r301372076>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOOF7NVORPJ2F7W6MXULLDP6PY7LANCNFSM4H7AVU2A> .

src/Reflex/Spider/Internal.hs

treeowl · 2019-07-09T19:13:35Z

Closing in favor of #322, based on @Ericson2314's suggestion regarding Dynamic.

Use less unsafeCoerce

9b6bc20

Get rid of a large fraction of calls to `unsafeCoerce`. Several of the ones that remain are removed in my merge generalization PR.

Ericson2314 reviewed Jul 9, 2019

View reviewed changes

src/Reflex/Spider/Internal.hs Show resolved Hide resolved

treeowl closed this Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use less unsafeCoerce #321

Use less unsafeCoerce #321

treeowl commented Jul 9, 2019

Ericson2314 Jul 9, 2019

treeowl Jul 9, 2019

Ericson2314 Jul 9, 2019

treeowl Jul 9, 2019 •

edited

Loading

treeowl Jul 9, 2019

Ericson2314 Jul 9, 2019

Ericson2314 Jul 9, 2019

treeowl commented Jul 9, 2019 via email

treeowl commented Jul 9, 2019

Use less unsafeCoerce #321

Use less unsafeCoerce #321

Conversation

treeowl commented Jul 9, 2019

Ericson2314 Jul 9, 2019

Choose a reason for hiding this comment

treeowl Jul 9, 2019

Choose a reason for hiding this comment

Ericson2314 Jul 9, 2019

Choose a reason for hiding this comment

treeowl Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

treeowl Jul 9, 2019

Choose a reason for hiding this comment

Ericson2314 Jul 9, 2019

Choose a reason for hiding this comment

Ericson2314 Jul 9, 2019

Choose a reason for hiding this comment

treeowl commented Jul 9, 2019 via email

treeowl commented Jul 9, 2019

treeowl Jul 9, 2019 •

edited

Loading