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

Wingman removes functions in where clauses during refine hole #2183

Closed
santiweight opened this issue Sep 10, 2021 · 9 comments · Fixed by #2184
Closed

Wingman removes functions in where clauses during refine hole #2183

santiweight opened this issue Sep 10, 2021 · 9 comments · Fixed by #2184
Labels
component: wingman type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@santiweight
Copy link
Collaborator

foo = undefined
  where
    bar :: Maybe a -> b
    bar = _
      where
        pleaseDontHurtMe = undefined

Refine hole on the only hole (bar = _)

You get:

foo = undefined
  where
    bar :: Maybe a -> b
    bar m_a = _we

@isovector why did you hurt my nested where function :(

This might well be an issue with a Wingman dependency, I don't know Wingman's internals well enough to judge...

@isovector
Copy link
Collaborator

Confirmed. Dang, this looks like it's going to be hard to fix.

Nested where doesn't appear to be relevant:


foo :: Int -> Bool
foo = _
  where
    pleaseDontHurtMe = undefined

exhibits the same behavior

@isovector isovector added component: wingman type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Sep 10, 2021
@santiweight santiweight changed the title Wingman removes functions in nested where Wingman removes functions in where clauses during refine hole Sep 10, 2021
@santiweight
Copy link
Collaborator Author

Updated the issue - sucks man :(

I'm surprised to hear that this is a complicated fix... If you need any help/review let me know and I can try to help

@isovector
Copy link
Collaborator

My guess is that this is a weird interaction between exactprint (which does the AST manipulation for us) and wingman's "agda-split" machinery responsible for splitting case statements into matches.

@isovector
Copy link
Collaborator

But also, what SHOULD wingman do here? In the case of binding arguments I guess we can just add them. But what about if there's a case split?

bar :: Maybe a -> b
bar ma = _
  where
    pleaseDontHurtMe = undefined

should this split into an explicit case statement rather than a match? Otherwise the where clause isn't scoped properly anymore.

@santiweight
Copy link
Collaborator Author

In the case of a case split, my expected behaviour would be that a case statement is used - but that is a good point, wingman is caught between a rock and a hard place.

Otherwise, Wingman could either:

  1. provide a "insert case of statement"
  2. provide an error if such a situation arises, which fixes the bug quickly

I actually quite like option one personally. I'm the type of person who does single-variable pattern matches with a case of statement, and never via function definition pattern matching (I don't know what that syntax is called). i.e. I prefer:

foo = \case
  Just a -> _
  Nothing -> _

and I less-prefer

foo (Just a) = _
foo Nothing = _

Some problems I foresee would be the Wingman suggestion space outgrowing reason, and implementation difficulty.

@isovector
Copy link
Collaborator

The easiest fix today would be to disable match generation in the presence of a where clause. This fixes the problem, at the cost of adding a lambda in the original example. Eg, refining this:

foo :: Int -> Bool
foo = _
  where
    pleaseDontHurtMe = undefined

would produce

foo :: Int -> Bool
foo = \n -> _w1
  where
    pleaseDontHurtMe = undefined

It kinda sucks, but it sucks less than trashing the where clause.

@santiweight
Copy link
Collaborator Author

Agreed - if it's that an easy fix, that would be my vote also.

@santiweight
Copy link
Collaborator Author

Also, HLint will probably eventually help fix this back to a lambda case anyway.

@isovector
Copy link
Collaborator

Not the best fix, but it's a fix.

@mergify mergify bot closed this as completed in #2184 Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: wingman type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants