[Patch+test] Fix bug with postqualified imports and qualifiedStyle=unrestricted

[ulidtko]

Hi!

I'll be brief. The bug:

# testcase-rule.yaml

- modules:
  - name: Data.Text
    importStyle: qualified
    qualifiedStyle: unrestricted
    asRequired: True
    as: T

on the single-line file:

import Data.Text qualified as T

emits, erroneously, a silly hint:

> hlint -h testcase-rule.yaml test.hs
test.hs:1:1-31: Warning: Data.Text should be imported qualified
Found:
  import Data.Text qualified as T
Perhaps:
  import qualified Data.Text as T
Note: may break the code

1 hint

I submit a direct testcase for the bug, then a fix, then more tests for better regression coverage.

Hoping for easy review & merge; release isn't urgent, batch up at will 🙌 Thanks in advance

Fixes #1499

[ulidtko]

My ID of the root cause is this line 206 👀

Right here, QualifiedStyleUnrestricted (no pre-/post- preference) was getting mapped into QualifiedPre.

[ulidtko]

Hey @zliu41 @ndmitchell — I noticed somebody has run CI on the PR; thanks.

I've checked all 4 workflows — none of them failed because of changes in the PR. Timeout; 143 oomkill; exitcode 251 (?) when building yaml haddock on Windows; build oomkilled on MacOS too.

Tests do pass ✔️ on this branch; I explicitly centered the bugfix around tests. See PR diff.

---

As a sidenote, the CI setup could use a brush-up. Misuse of actions/cache caught my eye right away — when used properly, it will dramatically speed up builds, reduce OOMs, and that without build integrity issues (from stale cache), when done properly.

[ulidtko]

@zliu41 @ndmitchell ping. Please review.

[googleson78]

Also hitting this same problem

[zliu41]

I'm inclined to remove the "should be imported post-qualified" hint, because there are tools available (e.g., stylish Haskell) for converting pre-qualified imports to post-qualified imports (or vice versa). If one cares about using pre-qualified or post-qualified imports consistently in their project, they can simply run the tool in the pre-commit hook.

That said, we can still merge this. @ulidtko the CI has been fixed since this PR was opened, can you rebase?

[ulidtko]

Hi @zliu41, thanks for review.

I'm inclined to remove the "should be imported post-qualified" hint [...]

No, please don't. In a large enough codebase, a "stop the world" PR with mass-rewrite of most imports has negligible chances to ever land, due to pervasive conflicts.

That's why an incremental approach can be preferred, based on hlint rules about individual modules.

That's also why qualifiedStyle=unrestricted is an important feature: it enables smooth nondisruptive incremental transitions. That can be the only way in some conditions (e.g. big repo with moderate code-churn velocity and strict hlint-compliance) to get PostQualifiedImports.

@ulidtko the CI has been fixed since this PR was opened, can you rebase?

Sure. Done.

I'll going to add clarifying comments in next commit.

[shayne-fletcher]

I'm inclined to remove the "should be imported post-qualified" hint, because there are tools available (e.g., stylish Haskell) for converting pre-qualified imports to post-qualified imports (or vice versa). If one cares about using pre-qualified or post-qualified imports consistently in their project, they can simply run the tool in the pre-commit hook.

Note also that the flag -Wprepositive-qualified-module (off by default) will warn on any prepositive qualified annotations.

[ulidtko]

Note also that the flag -Wprepositive-qualified-module (off by default) will warn on any prepositive qualified annotations.

Good point. With {-# OPTIONS_GHC -Wprepositive-qualified-module #-} — is possible to migrate module-by-module (as opposed to, across entire project at once).

To explain a motivating usecase further; my point to keep (and fix) qualifiedStyle=unrestricted is with rules like this:

# (project-local) .hlint.yaml

- modules:
  # ...
  - name: Core.Effects,
    importStyle: explicitOrQualified,
    asRequired: True,
    as: Effects,

— which encodes the intent to allow e.g. import Core.Effects (WithMetrics) as well as import qualified Core.Effects as Effects... but, forbids import Core.Effects qualified as Effects, completely by accident.

[zliu41]

Looks good. CI is somehow broken again, so I'll just merge it.

No, please don't. In a large enough codebase, a "stop the world" PR with mass-rewrite of most imports has negligible chances to ever land, due to pervasive conflicts.

A pre-commit hook can be made to run a program (hlint or a code formatter) only on the changed or added files. Alternatively, one can run a program (hlint or a code formatter) only on selected files, which allows them to do the migration piece by piece.

So the real question is, whether something is a formatting issue and can be taken care of by a code formatter, or a code smell that can only be detected by hlint.