Refactor collectLiterals in AlternateNumberFormat.

[drsooch]

This is not the final iteration I'm just looking for some quick feedback and suggestions:

This commit resolves #2490, but exposes other issues.

• Re-enable the alternateNumberFormat plugin

• Refactor collectLiterals to look for HsLit and HsOverLit directly
  rather than HsExpr.

• This new change removes the issue of finding duplicate Literal
  entries, as well as drastically reducing the time to parse

ISSUE:

• Currently the parsing only captures literals on the RHS of the
  function declaration. Patterns are not being matched properly. This
  was found in the prior iteration, but was remediated (at the cost of
  performance). As a result, some tests will fail.

NOTES:

• There are two implementations of collectLiterals one is collectLiterals and the other is collectLiterals' both function the same and are "equally" performant. However both suffer from the issue mentioned above where patterns are not captured...

If anyone has some advice/help (especially regarding the missing LHS literals) that would be great.

[jneira]

Hi, thanks for working in the fix, maybe the best way to reenable the plugin could be start the pr with a revert of the commit disabling it (to restore docs and other changes)

[pepeiborra]

If anyone has some advice/help (especially regarding the missing LHS literals) that would be great.

Have you read the original SYB paper? I read it a long time ago and don't remember the details, but I suspect that a simple traversal cannot collect literals inside expressions inside patterns inside expressions.

So I think your mkQ2 traversal was a step in the right direction to handle both HsExpr and HsPat. Did you investigate using extQ instead of rolling your own thing?

[drsooch]

Hi, thanks for working in the fix, maybe the best way to reenable the plugin could be start the pr with a revert of the commit disabling it (to restore docs and other changes)

I'm going to close this once I have a fix. Then I'll reopen a PR with a clean history.

[drsooch]

If anyone has some advice/help (especially regarding the missing LHS literals) that would be great.

Have you read the original SYB paper? I read it a long time ago and don't remember the details, but I suspect that a simple traversal cannot collect literals inside expressions inside patterns inside expressions.

So I think your mkQ2 traversal was a step in the right direction to handle both HsExpr and HsPat. Did you investigate using extQ instead of rolling your own thing?

I'm ashamed... I didn't understand what you meant when you were comparing HsPat and HsExpr and then I realized the extraction function (getLiteral) is expecting an HsExpr and would never traverse a Pattern in the first place...

Now I realize my original error. I've made a new change to include Pat as a possible choice to extract from.

I chose to refrain from applying everything (<>) (mkQ [] getLiteral) inside of getPattern as I wanted to keep the types clean (refrain from making everything a list).

This seems to capture most literals. I can imagine a pattern has been missed but I haven't exhaustively searched.

[pepeiborra]

Sorry, I probably meant Pat where I said HsPat, I'm not overly familiar with the ghc api naming scheme.

How is the performance now?

[drsooch]

Performance looks good. It's back in line with other plugins. I don't have a heavy enough Haskell workload to do exhaustive testing. I went through large files in HLS to make sure there were no issues.

If everyones ok I will probably close this PR and reopen a new PR with the full changes (reverting prior commits etc)

[drsooch]

Nevermind, I cleaned up the history on this branch. This is ready for PR.

[Ailrun]

Looks good to me. You checked the performance on HLS repo itself, right? I think then it's good to go.

[jneira]

looks great, many thanks for take care, I am sure people doing numeric stuff are gonna love this plugin