create UnusedIdent by matthewleon · Pull Request #3194 · purescript/purescript

matthewleon · 2018-01-05T15:00:32Z

Rather than using the magic string "__unused" to represent generated names that are meant to go unused, we create a new Ident constructor for them. Instead of eliminating unused function arguments in the optimization phase, they are stripped during the conversion from CoreFn to CoreImp.

matthewleon · 2018-01-05T19:24:25Z

This PR is a successor to #3188. Rather than attempting to address the issue by choosing a smarter magic string, it attempts to avoid optimizing based on matching Ident names in the first place.

kritzcreek

I've added a couple of comments, thanks!

kritzcreek · 2018-01-06T18:29:20Z

examples/passing/3187-UnusedNameClash.purs

+
+-- the __unused parameter used to get optimized away
+abuseUnused :: forall a. a -> a
+abuseUnused __unused = __unused


I don't understand why this is inside passing, shouldn't this explode now that UnusedIdent is a special, generated name inside the compiler?

No, the opposite. It exploded before this PR, because "__unused" was a magic string. This PR makes it so __unused is no longer a magic name, but a name like any other. So this test now passes.

Oh I see :D I somehow thought this was a self recursive ever looping binding. My bad.

kritzcreek · 2018-01-06T18:32:39Z

src/Language/PureScript/CodeGen/JS/Common.hs

 identToJs (GenIdent _ _) = internalError "GenIdent in identToJs"
+identToJs UnusedIdent = C.__unused
+
+-- | Convert an 'Ident' into a valid JavaScript identifier, or Nothing.


A function returning both a Maybe AND throwing exceptions is a red flag to me, which is almost guaranteed to be used wrong. And why do we still need the old version of this function?

Since this function is only used in a single place, I think it would be better to define it inline where it has more context.

Agreed. Will fix.

kritzcreek · 2018-01-06T18:36:15Z

src/Language/PureScript/Names.hs

 freshIdent' :: MonadSupply m => m Ident
 freshIdent' = GenIdent Nothing <$> fresh

+unusedIdent :: Ident


Why this indirection? You could just use the constructor.

kritzcreek · 2018-01-06T18:36:16Z

src/Language/PureScript/Names.hs

 runIdent (Ident i) = i
 runIdent (GenIdent Nothing n) = "$" <> T.pack (show n)
 runIdent (GenIdent (Just name) n) = "$" <> name <> T.pack (show n)
+runIdent UnusedIdent = "__unused"


Now that this is a special identifier, shouldn't this also be prefixed with a $ so it can't be mixed up with Ident "__unused"

good point. Will fix.

matthewleon · 2018-01-06T20:21:39Z

I've implemented the requested changes. Eager for any further feedback.

Fixes purescript#3187 Rather than using the magic string "__unused" to represent generated names that are meant to go unused, we create a new Ident constructor for them. Instead of eliminating them as arguments in the optimization phase, they are stripped during the conversion from CoreFn to CoreImp.

kritzcreek · 2018-01-07T13:14:28Z

Looks good to me, but I'd appreciate one more person with a better idea of Codegen to take a look and merge.

hdgarrood · 2018-01-07T13:41:53Z

Should we ever hit the cases where we return "$__unused", or should UnusedIdent constructors have been eliminated by then?

matthewleon · 2018-01-07T13:45:33Z

We will hit that case when the compiler infers a Partial constraint. My next PR, #3195, which depends on this, then eliminates that case (to be discussed) and changes the $__unused to an error, like in the case of the other generated Names.

matthewleon · 2018-01-07T13:48:30Z

To clarify: __unused was employed for two rather different purposes:

Unused function arguments, which is what this PR optimizes away.
Assigning an id-style function to the __unused name in a Let binding, and then calling it with the "old" function body wrapped in an IIFE, which is used to propagate the inferred Partial constraint. This PR leaves that as an UnusedIdent, which will then be turned into $__unused in JS. Then my next PR is meant to get rid of that, leaving the generated JS with no $__unuseds.

hdgarrood · 2018-01-07T13:57:54Z

Do we need to come to a consensus on #3182 first?

matthewleon · 2018-01-07T14:00:37Z

Do we need to come to a consensus on #3182 first?

I don't think so? The only behavioral change this PR introduces is that $__unused will be output for the Partial constraint rather than __unused, and arguments named __unused should never be "optimized" away. This shouldn't have any effect on foreign imports, I don't think?

hdgarrood · 2018-01-07T14:12:10Z

Sorry, I guess I really mean #2768; whether or not we do that seems like it would have implications for what we do here, or at least has implications for the approach taken in #3195, right? And the decision we make about #2768 also influences #3182 as well, which is why I mentioned that one in the first place.

hdgarrood · 2018-01-07T14:16:34Z

Ok so I'm just untangling my own confusion here. AFAICT we're pretty much all agreed that we want to do #2768, right? That's a breaking change at the moment, but only because you're allowed to use constraints in foreign import declarations. If we did #3182 first (which is also breaking), it wouldn't be.

matthewleon · 2018-01-07T14:20:23Z

#2768 is not necessarily breaking. The approach brought up by @paf31 simply converts the parameter to "unused" (UnusedIdent with this PR), which means that the function goes from fn(emptyDictionary)(param) before to fn()(param) after – there is still a function there. The more radical approach that I bring up would convert fn(emptyDictionary)(param) to fn(param), which is indeed breaking. But I believe that is orthogonal to this PR. It does have an effect on #3195 , as you mention, but we can discuss that there (there is a test).

matthewleon · 2018-01-07T14:21:35Z

As for #3182 , you're right of course. But I don't see how any of these impact this PR.

hdgarrood · 2018-01-07T15:34:46Z

Ok, well ignore me for now then, and if I manage to sort out my confusion before we want to merge this then I'll let you all know, but otherwise please don't let me keep this from being merged.

paf31 · 2018-01-07T19:07:42Z

I wouldn't recommend trying to optimize as far as fn(param) if you ever want to support polymorphism over constraints, since it makes the representation of => non-parametric. Inlining and specialization is probably a better way to go IMO.

matthewleon · 2018-01-11T01:32:11Z

@garyb or @natefaubion any thoughts on this?

matthewleon force-pushed the UnusedIdent branch 3 times, most recently from 1958dc5 to d6bf7b0 Compare January 5, 2018 19:17

matthewleon mentioned this pull request Jan 6, 2018

eliminate id function for inferred partial constraints in desugaring #3195

Closed

kritzcreek requested changes Jan 6, 2018

View reviewed changes

matthewleon force-pushed the UnusedIdent branch from d6bf7b0 to 0c382c6 Compare January 6, 2018 20:17

matthewleon force-pushed the UnusedIdent branch from 0c382c6 to 9a9e623 Compare January 6, 2018 20:23

kritzcreek approved these changes Jan 7, 2018

View reviewed changes

garyb merged commit 8369db4 into purescript:master Jan 20, 2018

Conversation

matthewleon commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewleon commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kritzcreek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewleon commented Jan 6, 2018

Uh oh!

kritzcreek commented Jan 7, 2018

Uh oh!

hdgarrood commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdgarrood commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdgarrood commented Jan 7, 2018

Uh oh!

hdgarrood commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 7, 2018

Uh oh!

hdgarrood commented Jan 7, 2018

Uh oh!

paf31 commented Jan 7, 2018

Uh oh!

matthewleon commented Jan 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matthewleon commented Jan 5, 2018 •

edited

Loading

matthewleon commented Jan 5, 2018 •

edited

Loading

matthewleon commented Jan 7, 2018 •

edited

Loading

matthewleon commented Jan 7, 2018 •

edited

Loading