Interaction of import list and hiding lists

[fendor]

Tries to implement the suggestion in #19 as I have understood it.

There are some rough edges regarding importing Data Constructors. Tries to reuse as much code as possible.

Examples:

-- Main.hs
import Data.Text

> hsimport --hiding -m Data.Text -s isInfixOf Main.hs

-- Main.hs
import Data.Text hiding (isInfixOf)

> hsimport -m Data.Text -s isInfixOf Main.hs

-- Main.hs
import Data.Text

---

import Data.Text hiding (isPrefixOf, isInfixOf)

> hsimport -m Data.Text -s isInfixOf Main.hs

import Data.Text hiding (isPrefixOf)

---

import Data.Text (isPrefixOf, isInfixOf)

> hsimport --hiding -m Data.Text -s isInfixOf Main.hs

import Data.Text (isPrefixOf)

[dan-t]

if we hide all constructors from a data type, just remove the whole import. If there’s ‘import Foo (Bar(A))’ and ‘A’ gets hidden, then the result ‘import Foo (Bar)’ also seems to be sensible.

if we want to hide only a subset of the constructors, hide only those.

Ok.

if the currently imported Constructors only have an overlap with the constructors to hide, hide the constructors that intersect with the imports. I don’t quite understand this one, can you please give me an example.

Greetings, Daniel

[fendor]

It is very similar to the second point, what I had in mind:

import Foo(Bar(A, B, C))

> hsimport --hiding -m Foo -s Bar --with B C D # note, D is not currently imported

import Foo(Bar(A))

---

One more complicated case that I currently dont know a nice solution for:

import Foo(Bar(..))

> hsimport --hiding -m Foo -s Bar --with B 

We dont know the constructors, so we cant do the right thing.

[dan-t]

Thinking about the whole hiding of constructors I don‘t quite see a real use case for it. Normally you just want to extend the import list and not reduce it. A correct reduction anyway can only be done with compiler support. So I‘ve to question a bit the usefulness of ´—hiding´ in conjunction with ´—with´ and if we should just forbid it.

About the hiding of ´B´ on ´import Foo (Bar(..))´, like you‘re saying, it can‘t be correctly handled and should at least result into an error.

But adding this kind of error handling with the increased complexity in the code only makes sense if the functionality adds real value. But I‘ve my doubts if that’s the case for the constructor hiding.

[fendor]

A use case I can think of is hiding a certain constructor if it causes a name clash. This can be utilised by an ide, e.g. Haskell-IDE-Engine, to provide code actions to hide a constructor. Granted, it does not seem like a common case, but it has a use-case in my opinion.

Does hsimport ever throw an error? I dont think so, and I would prefer, if it stayed that way. Possible solutions may be:

• Throw an error in that very specific case.
• Forbid --with in combination with --hiding.
• Dont do anything in that case.
• Hide all constructors, thus forcing the user to import exactly the constructors they want.
• Give up and add a second import line, similar to the previous behaviour.

I am leaning to the fourth possibility, but mainly because of my personal use-case, Haskell-IDE-Engine, would profit from it the most. However, I dont think that this would be the most intuitive behaviour for HsImport.

[dan-t]

Ok, if you see a use case for it then I‘m fine with it.

Then I would prefer if „importChange“ returns an „Either Error [ImportChange]“, with „Error“ just being an alias for „String“, so that the „String“ can contain a descriptive error message.

The higher level functions should then return an „IO (Maybe Error)“, like „hsimportWithArgs“ already does.

[fendor]

Now the smarter Constructor implementation works. However, the case One more complicated case that I currently dont know a nice solution for:

import Foo(Bar(..))

> hsimport --hiding -m Foo -s Bar --with B 

Is not fixed. The fix would be to change the signature of

removeSpecList :: SymbolImport -> HS.ImportSpecList Annotation -> Maybe (HS.ImportSpecList Annotation)

to something like:

removeSpecList :: SymbolImport -> HS.ImportSpecList Annotation -> Either Error (Maybe (HS.ImportSpecList Annotation))

but this actually complicates the code quite a lot. I was thinking about using error and catching it somewhere to make it less convoluted, but this is just like a goto, so I am hesitant. Is this increase of complexity tolerable or is the goto more acceptable?

I will push the changes for Either Error ... for comparison.

EDIT: Here the changes that are required to use EIther https://github.com/fendor/hsimport/compare/hsimport-interaction...fendor:hsimport-error?expand=1

[fendor]

Now also integrates the following case:

-- Main.hs
import Data.Text (isPrefixOf)

> hsimport -m Data.Text Main.hs

-- Main.hs
import Data.Text

[dan-t]

Thanks for your changes!

I'm not absolutely happy with the Either Error [ImportChange] return type. ImportChange already contains the constructor NoImportChange so it seems somehow more natual to extend it with another Error constructor.

So instead of Either Error [ImportChange] having:

data ImportChange
   = ReplaceImportAt SrcSpan ImportDecl
   | AddImportAfter SrcLine ImportDecl
   | AddImportAtEnd ImportDecl
   | FindImportPos ImportDecl
   | NoImportChange
   | Error String
   deriving (Show)

Seems like the overall nicer and more flexible solution.

[fendor]

I agree that it seems more natural, but I am not sure how to handle errors in that case. The result would be now [ImportChange], if any of these is Error String (or maybe ImportError which seems to be a nicer name to me), then one of the following may happen:

• Perform ImportChanges that are not Errors and print the Error.
• Do not execute any ImportChange and print the error.

Current behaviour is the latter. Also, what happens in case of multiple errors? Just concatenate them and then print them?

[dan-t]

(or maybe ImportError which seems to be a nicer name to me)

Yes, ImportError is a fine name.

• Do not execute any ImportChange and print the error.

I think I prefer this behavior.

Also, what happens in case of multiple errors? Just concatenate them and then print them?

Concatenate them and return them from hsimportWithSpec by changing its return type to IO (Maybe Error).

[fendor]

Indeed, that is easier to read! I also extended the error message to give an example why the execution we try to perform can not succeed.

[dan-t]

Do you want to apply further changes or can I merge it?

[fendor]

I consider the feature done. If the code quality and documentation is to your satisfaction, you can merge it!

[dan-t]

The '--hiding' option is now released with 'hsimport 0.11.0'.

Thanks for your work!

Greetings, Daniel