Deal with unbreakable blocks in Applicative Do
ClosedPublic

Authored by dfeuer on Aug 29 2017, 6:17 PM.

Details

Summary

The renamer wasn't able to deal with more than a couple strict
patterns in a row with ApplicativeDo. This hack seems to work
around the problem, but it would be better to understand what's
going on a little better.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfeuer created this revision.Aug 29 2017, 6:17 PM
bgamari requested changes to this revision.Aug 29 2017, 6:21 PM

This deserves a comment with a reference to the ticket at very last.

This revision now requires changes to proceed.Aug 29 2017, 6:21 PM

This deserves a comment with a reference to the ticket at very last.

Let's try to figure out if this is even remotely the right fix.... But yeah, if we go with this, I can add a comment.

As I mentioned in the ticket, I am very curious why we're only seeing this problem with strict patterns, and not with other dependencies. It may well be that the best fix is to that part of the code, rather than this one. I don't know.

simonmar edited edge metadata.Aug 30 2017, 4:00 AM

Ok, I see what went wrong: the new constraint that prevented strict patterns from being a segment boundary caused an undocumented assumption to break here. The assumption was that slurpIndependentStatements would return a non-empty list snd, because at some point it should encounter a dependency.

The right fix I think is to modify slurpIndependentStatements so that it considers a strict pattern bind to be a form of dependency too, matching the logic used to detect dependencies in segments. This will be a better heuristic in some cases than the fix here.

Okay, I'll try that way.

dfeuer updated this revision to Diff 13693.Aug 31 2017, 10:21 PM
dfeuer edited edge metadata.

Move logic to splitIndependentStmts

@simonmar I think I probably did what you were suggesting, but I'd appreciate a double check. I don't yet have a solid understanding of exactly how all these pieces fit together.

simonmar requested changes to this revision.Sep 1 2017, 2:38 AM
simonmar added inline comments.
compiler/rename/RnExpr.hs
1829

This is too restrictive, it will bail out if it finds any strict pattern match even if there were previously some independent statements in the group. I think what we want is to change the following line to

| isEmptyNameSet (bndrs `intersectNameSet` fvs) && not (isStrictPattern pat)
This revision now requires changes to proceed.Sep 1 2017, 2:38 AM
dfeuer added a comment.Sep 1 2017, 8:53 AM

Oh, that makes much more sense. Not sure what I was thinking.

dfeuer updated this revision to Diff 13696.Sep 1 2017, 8:55 AM
dfeuer edited edge metadata.

Do it right this time.

dfeuer updated this revision to Diff 13700.Sep 1 2017, 11:13 AM

That was silly...

RyanGlScott added inline comments.
testsuite/tests/simplCore/should_run/T14178.hs
33 ↗(On Diff #13700)

I feel like this Diff accidentally brought in changes from other commits (for example, what is this doing here?).

dfeuer updated this revision to Diff 13703.Sep 1 2017, 1:52 PM

Base differential off correct commit

dfeuer added a comment.Sep 1 2017, 1:53 PM

Sorry about that. Should be fixed now.

simonmar accepted this revision.Sep 5 2017, 7:24 AM

LGTM

This revision was automatically updated to reflect the committed changes.