Skip to content

getRestrictItem returns a lits of hints #1648

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parsonsmatt
Copy link

This PR addresses #1647

Thanks for the pull request!

By raising this pull request you confirm you are licensing your contribution under all licenses that apply to this project (see LICENSE) and that you have no patents covering your contribution.

If you care, my PR preferences are at https://github.com/ndmitchell/neil#contributions, but they're all guidelines, and I'm not too fussy - you don't have to read them.

@parsonsmatt
Copy link
Author

parsonsmatt commented May 21, 2025

The naive approach given in this PR fails, as some rules can be more specific and unrestrict things. Local test output:

TEST FAILURE IN tests/_import_style_1
DIFFER ON LINE: 43
GOT : tests/importStyle-1.hs:6:1-51: Warning: HypotheticalModule3.OtherSubModule should be imported unqualified
WANT: 6 hints
FULL OUTPUT FOR GOT:
tests/importStyle-1.hs:1:1-26: Warning: Avoid restricted alias
Found:
  import HypotheticalModule1
Perhaps:
  import HypotheticalModule1 as HM1
Note: may break the code

tests/importStyle-1.hs:2:1-26: Warning: HypotheticalModule2 should be imported qualified or with an explicit import list
Found:
  import HypotheticalModule2
Perhaps:
  import qualified HypotheticalModule2
Note: may break the code

tests/importStyle-1.hs:3:1-26: Warning: HypotheticalModule3 should be imported qualified
Found:
  import HypotheticalModule3
Perhaps:
  import qualified HypotheticalModule3
Note: may break the code

tests/importStyle-1.hs:4:1-47: Warning: HypotheticalModule3.SomeModule should be imported unqualified
Found:
  import qualified HypotheticalModule3.SomeModule
Perhaps:
  import HypotheticalModule3.SomeModule
Note: may break the code

tests/importStyle-1.hs:5:1-47: Warning: HypotheticalModule3.SomeModule should be imported unqualified
Found:
  import HypotheticalModule3.SomeModule qualified
Perhaps:
  import HypotheticalModule3.SomeModule
Note: may break the code

tests/importStyle-1.hs:6:1-51: Warning: HypotheticalModule3.OtherSubModule should be imported post-qualified or unqualified
Found:
  import qualified HypotheticalModule3.OtherSubModule
Perhaps:
  import HypotheticalModule3.OtherSubModule qualified
Note: may break the code

tests/importStyle-1.hs:6:1-51: Warning: HypotheticalModule3.OtherSubModule should be imported unqualified
Found:
  import qualified HypotheticalModule3.OtherSubModule
Perhaps:
  import HypotheticalModule3.OtherSubModule
Note: may break the code

7 hints

.
TEST FAILURE IN tests/_import_style_2
DIFFER ON LINE: 1
GOT : tests/importStyle-2.hs:6:1-51: Warning: HypotheticalModule3.OtherSubModule should be imported unqualified
WANT: No hints
FULL OUTPUT FOR GOT:
tests/importStyle-2.hs:6:1-51: Warning: HypotheticalModule3.OtherSubModule should be imported unqualified
Found:
  import HypotheticalModule3.OtherSubModule qualified
Perhaps:
  import HypotheticalModule3.OtherSubModule
Note: may break the code

1 hint

Relevant import yaml:

- modules:
  - {name: HypotheticalModule1, as: HM1, asRequired: true}
  - {name: HypotheticalModule2, importStyle: explicitOrQualified}
  - {name: HypotheticalModule3, importStyle: qualified}
  - {name: 'HypotheticalModule3.*', importStyle: unqualified}
  - {name: 'HypotheticalModule3.OtherSubModule', importStyle: unrestricted, qualifiedStyle: post}
  - {name: HypotheticalModule4, importStyle: qualified, as: HM4, asRequired: true}
  - {name: HypotheticalModule5, importStyle: qualified, qualifiedStyle: post}

So the idea is that we'd have the direct match rule for HypotheticalModule3.OtherSubModule, and also the wildcard match for HypotheticalModule3.*, and then we sconcat those RestrictItems.

I'm updating my fix to do sconcat if there's an exact module name match, and to otherwise use a list of rules if it's just wildcard matches.

@parsonsmatt
Copy link
Author

I've tested the patch on the reproduction repository and the patched version correctly reports the rule violation 🎉

Only sconcat if an exact match is found
Copy link
Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest reviewing with whitespace changes hidden

Comment on lines +256 to +257
Just exact ->
[sconcat (exact NonEmpty.:| wildcard)]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This preserves the old behavior: take the exact match and concat the rest of the rules into it.

Comment on lines +254 to +255
Nothing ->
wildcard
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this branch, we had no exact match, meaning that wildcard contains all of the wildcard matches that hit. This means we have multiple wildcard rules that should be checked, and we probably don't want to combine these...

I can see some logic where "nested" rules should be combined, but totally disparate rules should not be. Like, Handler.** and Handler.Foo.** maybe should be combined because of the shared literal prefix. But **.*Spec and Handler.** should not, even though they have overlaps in Handler.FooSpec.

@parsonsmatt parsonsmatt force-pushed the mattp/1647-dont-merge-restrict-items branch from 13e58fa to 1316708 Compare May 22, 2025 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant