Skip to content
This repository has been archived by the owner on Nov 7, 2020. It is now read-only.

Reuse more logic from GHC regarding detecting of unused imports #68

Open
chshersh opened this issue Sep 10, 2019 · 4 comments
Open

Reuse more logic from GHC regarding detecting of unused imports #68

chshersh opened this issue Sep 10, 2019 · 4 comments
Labels
purify Clean import section by removing unused imports refactoring

Comments

@chshersh
Copy link
Contributor

See here:

@chshersh chshersh added purify Clean import section by removing unused imports refactoring labels Sep 10, 2019
@jrp2014
Copy link

jrp2014 commented Mar 29, 2020

There is a function printMinimalImports that seems to be designed for this purpose (see, eg, https://gitlab.haskell.org/ghc/ghc/issues/15439) that produces a TcRnTypes.RnM (). It may be trivial to wire it up to smuggler, but I haven't figured out how. Pointers welcome.

@chshersh
Copy link
Contributor Author

chshersh commented Apr 1, 2020

@jrp2014 This particular issue is actually about a different thing. @vrom911 refactored the printMinimalImports function in GHC, so we could use getMinimalImports to implement #42.

This issue is about refactoring another GHC function. Currently our implementation of unused imports has some duplicate code with GHC. Our code:

unusedLocs :: ImportDeclUsage -> [(Int, Int)]
unusedLocs (L (UnhelpfulSpan _) _, _, _) = []
unusedLocs (L (RealSrcSpan loc) decl, used, unused)
-- Do not remove `import M ()`
| Just (False, L _ []) <- ideclHiding decl
= []
-- Note [Do not warn about Prelude hiding]
-- TODO: add ability to support custom prelude
| Just (True, L _ hides) <- ideclHiding decl
, not (null hides)
, pRELUDE_NAME == unLoc (ideclName decl)
= []
-- Nothing used; drop entire decl
| null used = [ (lineNum, colNum)
| lineNum <- [srcSpanStartLine loc .. srcSpanEndLine loc]
, colNum <- [srcSpanStartCol loc .. getEndColMax unused]
]
-- Everything imported is used; drop nothing
| null unused = []
-- only part of non-hiding import is used
| Just (False, L _ lies) <- ideclHiding decl
= unusedLies lies
-- TODO: unused hidings
| otherwise = []

Most work is done by the compiler, we only pattern-match on imports to decide whether the import is redudant or not. Here is the same code in GHC:

So first we need to refactor the warnUnusedImport function in GHC to be able to reuse more logic from the compiler. This is also less trivial, because in smuggler we would like to implement the removal of unused hidings, which is not supported by GHC at the moment, but can be easily added in here, since we have all required information. But GHC refactoring should be performed with these extension capabilities in mind.

@jrp2014
Copy link

jrp2014 commented Apr 1, 2020

Thanks. Yes. Sorry, I should have pointed t getMinimalImports. That seems to do the trick, although it retains stubs for instance only imports, which is OK, at least at this stage.

The point at which I am stuck is grafting the result back into the ast and printing out the result (if there was a change). If I've understood smuggler correctly, it reparses the source to get an ast, replaces the imports, and exactPrints the result back out. It's a pity that that extra work is necessary, but I assume that the results of the parsing phase of the compiler are not available directly to a typechecker plug-in. I haven't dug too deep into the compiler but any pointers would, as I say, be welcome.

In summary, can we get away without using exactprint? (At a later stage it would be good to move to using ghc-lib.)

As things stand, with GHC 8.8.3 smuggler doesn't pass its unit tests.

@jrp2014
Copy link

jrp2014 commented Apr 10, 2020

I've now had a bit more of a look at this.

Regrettably, the getMinimalImports function generates a GhcRn minimal import list, which means that it can't just be grafted back into the GhcPs ast and ann that ghc-exactprint provides.

I haven't completely figured out how the ast and anns work together. Unless I've missed something, smuggler seems to be able to delete unused imports by deleting their anns but leaving the original ast, leaving the exactPrint function to resolve any discrepancies.

I wondered whether the exactprint transform machinery might provide a way of manipulating the ast and anns together.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
purify Clean import section by removing unused imports refactoring
Projects
None yet
Development

No branches or pull requests

2 participants