I discussed this with @kritzcreek and came to the conclusion that it shouldn't be sitting in the middle of the desugaring code. That said, it makes sense to perform this optimization between typechecking (the source of the generated block) and the functional core, as alternative backends shouldn't need to duplicate the optimization. Any ideas?

Shall we close this then, since the approach is going to be different?

Shall we close this then, since the approach is going to be different?

sure

I'm not sure how much you talked with @kritzcreek about this, but I chatted about it briefly with him a while ago too. My thinking was that there would also be an UnusedIdent constructor in the names for the CoreFn (/ CoreImp? not sure how long we need it for) ASTs, perhaps. That'd give us more options for doing passes on the other form or something like that to apply the optimisation instead.

I was going to suggest making that change on the other PR even, and then keeping the "remove unused" pass in the optimizer there, but just having it less fragile since it'd have the constructor to match on... but in the end I figured it was fine to just not generate it at all.

My thinking was that there would also be an UnusedIdent constructor in the names for the CoreFn (/ CoreImp? not sure how long we need it for) ASTs, perhaps. That'd give us more options for doing passes on the other form or something like that to apply the optimisation instead.

I did have a chat with @kritzcreek about this one. I think what you are proposing makes sense broadly speaking. As it stands, CoreFn does "inherit" the UnusedIdent constructor from the unsugared AST... It gets erased in the transition to CoreImp.

That said, for this particular problem, I believe it makes sense to do the optimization before even getting to CoreFn. Recall that the only reason that this extra function is introduced is for type checking. Given that alternate backends might want to work with CoreFn, I can't see any reason for this particular artifact to be kept around after desugaring.

Thoughts?

Another clarification here is that the particular way that UnusedIdent is used in creating the partial constraints is fundamentally different from its usage elsewhere. Elsewhere, it is always a name given to a generated function parameter. Here, it is the name of a function itself.

CoreFn currently lacks an optimiser, but there's a bunch of things we could in theory do there, so it just seemed like this would be the first pass - any backend would still benefit from it, as it'd use the post-optimised version anyway. Unless I misunderstand?

The reason CoreFn exists is to make things like this easier, in fact! Since doing anything to the "full" AST is unpleasant just because of the number of cases that need considering. We're basically getting no benefit from CoreFn at all at the moment, as it goes directly into "CoreImp" (which is still just JS at this point really).

The reason CoreFn exists is to make things like this easier, in fact! Since doing anything to the "full" AST is unpleasant just because of the number of cases that need considering. We're basically getting no benefit from CoreFn at all at the moment, as it goes directly into "CoreImp" (which is still just JS at this point really).

Okay, cool. I will try to rewrite this in a CoreFn optimization pass.

If it doesn't work out for some reason, that's fine too - perhaps we'll need to reconsider how the intermediate parts fit together to make them more useful.