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

Improve lint import warnings #3685

Open
wants to merge 3 commits into
base: master
from

Conversation

@matthew-hilty
Copy link
Contributor

commented Jun 20, 2019

Two main changes:

  1. Add an import declaration's optional qualifier as a parameter to UnusedImport.
    • Previously, warnings didn't distinguish between import declarations from the same module.
      Code like the following
      import A.B (x) -- `x` is used.
      import A.B (y) as C -- `y` is not used.
      would induce a warning like The import of module A.B is redundant even though only the qualified import declaration C is actually redundant.
    • The warning now would be The import of module A.B (qualified as C) is redundant.
  2. Include kind imports among all imports considered by the linter for determining UnusedImport and UnusedExplicitImport warnings.
    • Previously, kind imports were ignored.
      The linter wouldn't emit any warnings for code like the following.
      import A.B (kind K) -- `kind K` is not used.
    • And the linter, disregarding kind K, would emit an UnusedImport instead of an UnusedExplicitImport for code like the following.
      import A.B (x, kind K) -- `x` is not used, but `kind K` is.

Issues #1647, #2339, #1897, #1647, #1842, #1670 are similar to this issue, but they relate to warnings/errors about types or exports rather than about imports.

Improve linter import warnings
- Include qualifier in UnusedImport warning so to distinguish between
  multiple import declarations for the same module.
- Include kind imports with other imports for relevance checking.
@hdgarrood
Copy link
Member

left a comment

This looks great, thanks! Sorry it took us a while to get to it. I just have a couple of minor questions and then I'm 👍 for merging. Also would you mind adding yourself to CONTRIBUTORS.md in this PR? (I know you have a few open but this one is probably going to be the first to get merged.)

go _ result = result
go (ImportDeclaration (pos, _) mn typ qual) =
M.alter (return . ((pos, typ, qual) :) . fromMaybe []) mn
go _ = id

This comment has been minimized.

Copy link
@hdgarrood

hdgarrood Jul 29, 2019

Member

Am I correct in thinking the change in this file is just a refactor which shouldn't change behaviour?

This comment has been minimized.

Copy link
@matthew-hilty

matthew-hilty Jul 29, 2019

Author Contributor

Yeah. I think Gary's advice from a different pull request was still in mind when I made this change. It's meant to be just a simple refactor like you say, but maybe I should undo this patch so everything here is related directly to linting. I don't mind either way.

This comment has been minimized.

Copy link
@hdgarrood

hdgarrood Jul 29, 2019

Member

Happy to keep this in here - I was just wondering.

mark = markCode . runModuleName
qual' q = " (qualified as " <> mark q <> ")"
qual = maybe "" qual'
in line $ "The import of module " <> mark mn <> qual qualifier <> " is redundant"

This comment has been minimized.

Copy link
@hdgarrood

hdgarrood Jul 29, 2019

Member

Bit of a nit but may I suggest copying GHC and saying "The qualified import of [module] as [qualified-name] is redundant"?

This comment has been minimized.

Copy link
@matthew-hilty

matthew-hilty Jul 29, 2019

Author Contributor

Sure, that seem likes a good idea. I'll change the message, and I'll probably be able to push up the changes by tonight.

(By the way, please, pick as many nits as you want. I appreciate any tips/advice you guys have for me.)

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@matthew-hilty looks like your rebase has gone a bit wrong. Would you mind trying again?

@matthew-hilty matthew-hilty force-pushed the matthew-hilty:improve-lint-import-warnings branch from 6326e3f to 965eaf2 Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.