Wildcard rules for imports have their relevant properties unioned, including exclusions

[parsonsmatt]

We have two rules:

- modules:
    - name:
          - "**.*Spec"
      within: 
          - Main
          - Spec
    - name:
          - "Handler.**"
      within: 
          - "Handler.**"
          - "**.*Spec"

The intention of these rules are:

1. You should not import Spec modules except in defining tests. Shared helpers should be extracted to utility modules.
2. You should not import Handler modules except in Handler code and in tests for Handler code.

However, the rules appear to be merged. We recently discovered that rules matching the first were not being triggered except in pre-commit, which was only loading that rule. However, these rules were not being enforced in CI or in our make hlint or make hlint-all commands, which loaded all of the rules.

I am working up a minimal reproduction of this behavior.

EDIT:

• Does not appear to trigger with hlint-3.8 from Hackage. We are using a patched hlint - verifying that this isn't something specific to us...

[parsonsmatt]

OK, I have a reproduction over here: https://github.com/parsonsmatt/hlint-repro-1647

I need to figure out if there's more going on, but at least one problematic behavior: when an import matches two rules, then the import will be allowed if either rule has a within exclusion match. The above reproduction has two rules:

- modules:
    - name: "Handler.**"
      within: 
        - "Handler.**"
        - "**.*Spec"
    - name: "**.*Spec"
      within:
        - Spec 

This file and import should report an error due to a violation of the rule **.*Spec.

module Handler.FooSpec where

import Handler.BarSpec

However, becaues the import also matches the rule for Handler.**, which is allowed in **.*Spec and also allowed in Handler.**, this prevents the rule for "import **.*Spec" from firing.

[parsonsmatt]

OK, I think I found the issue:

hlint/src/Hint/Restrict.hs

Lines 239 to 248 in 55adfee

	lookupRestrictItem :: LocatedA ModuleName -> Map.Map String RestrictItem -> Maybe RestrictItem
	lookupRestrictItem ideclName mp =
	let moduleName = moduleNameString $ unLoc ideclName
	exact = Map.lookup moduleName mp
	wildcard = nonEmpty
	. fmap snd
	. reverse -- the hope is less specific matches will end up last, but it's not guaranteed
	. filter (liftA2 (&&) (elem '*') (`wildcardMatch` moduleName) . fst)
	$ Map.toList mp
	in exact <> sconcat (sequence wildcard)

lookupRestrictItem :: LocatedA ModuleName -> Map.Map String RestrictItem -> Maybe RestrictItem
lookupRestrictItem ideclName mp =
    let moduleName = moduleNameString $ unLoc ideclName
        exact = Map.lookup moduleName mp
        wildcard = nonEmpty
            . fmap snd
            . reverse -- the hope is less specific matches will end up last, but it's not guaranteed
            . filter (liftA2 (&&) (elem '*') (`wildcardMatch` moduleName) . fst)
            $ Map.toList mp
    in exact <> sconcat (sequence wildcard)

This is going to sconcat the two wildcard matches together. I think a proper fix is to instead return a [RestrictItem] instead of Maybe RestrictItem, and run the rule for each RestrictItem instead of combining them together.