Overlapping instances for HashMap Text Value

[RichardWarfield]

There seems to be an overlap with microlens-platform:

    • Overlapping instances for Lens.Micro.Internal.Ixed
                                  (HM.HashMap Text Value)
        arising from a use of ‘ix’
      Matching instances:
        instance Lens.Micro.Internal.Ixed (HM.HashMap Text Value)
          -- Defined in ‘microlens-aeson-2.3.0.4:Lens.Micro.Aeson.Internal’
        instance (Eq k, Hashable k) =>
                 Lens.Micro.Internal.Ixed (HM.HashMap k a)
          -- Defined in ‘Lens.Micro.Platform’

[fosskers]

Thanks for the report.

[fosskers]

Bumping this to remind myself.

[fosskers]

Now that I look, how is that happening? That instance is defined in Lens.Micro.Aeson.Internal, which isn't intended to be imported by users. Neither is it reexported by the main module.

[fosskers]

Is this still an issue?

[RichardWarfield]

Sorry I haven't looked at this for a while. But I think what you said above about this being only in an "Internal" module isn't quite possible unless I've misunderstood your comment. Haskell modules can't control which instances they import/export - see e.g. section 5.4 of the Haskell 2010 report.

[fosskers]

Do you by chance still have the code around that triggered the original compilation failure?

[RichardWarfield]

Unfortunately I don't remember what code I was writing when I opened the issue. But looking at the two instances, I'm confident that any module that imports both Lens.Micro.Aeson and Lens.Micro.Platform (and uses the Ixed instance of HashMap Text Value) is going to run into this. Typeclass instances always "infect" all downstream modules and there isn't any way to prevent this IIRC. I think an easy fix would be to add an {-# OVERLAPS #-} pragma to the instance, which is anyway just the same implementation as in Lens.Micro.Platform.

[sjshuck]

I just ran into this.

Noticing that ix only seems to be used internally in two places:

• https://github.com/fosskers/microlens-aeson/blob/521b6b70142b5fbed076fd98d63f2bc81f8de0a5/src/Lens/Micro/Aeson.hs#L333
• https://github.com/fosskers/microlens-aeson/blob/521b6b70142b5fbed076fd98d63f2bc81f8de0a5/src/Lens/Micro/Aeson.hs#L358

Is this the only reason for creating these orphan instances? Do you think they can be replaced by some myHashMapIx and myVectorIx that aren't a class method?

Alternately, can microlens-platform just be a dependency of this package? My gut feeling is most people using microlens-aeson will already want it anyway.

[fosskers]

Can you elaborate on the exact error you saw?

[sjshuck]

This isn't with latest releases of aeson or microlens-aeson. Let me know if you have reason to believe this would make a difference.

>>> "{\"foo\": \"bar\"}" ^. _Object . ix "foo" . _String

<interactive>:7:35: error:
    • Overlapping instances for Lens.Micro.Internal.Ixed
                                  (Data.HashMap.Internal.HashMap
                                     Data.Text.Internal.Text
                                     aeson-1.5.6.0:Data.Aeson.Types.Internal.Value)
        arising from a use of ‘ix’
      Matching instances:
        two instances involving out-of-scope types
          instance (Eq k, hashable-1.3.0.0:Data.Hashable.Class.Hashable k) =>
                   Lens.Micro.Internal.Ixed (Data.HashMap.Internal.HashMap k a)
            -- Defined in ‘Lens.Micro.Platform’
          instance Lens.Micro.Internal.Ixed
                     (Data.HashMap.Internal.HashMap
                        Data.Text.Internal.Text
                        aeson-1.5.6.0:Data.Aeson.Types.Internal.Value)
            -- Defined in ‘microlens-aeson-2.3.1:Lens.Micro.Aeson.Internal’
    • In the first argument of ‘(.)’, namely ‘ix "foo"’
      In the second argument of ‘(.)’, namely ‘ix "foo" . _String’
      In the second argument of ‘(^.)’, namely
        ‘_Object . ix "foo" . _String’

[fosskers]

This compiles without issue:

{-# LANGUAGE OverloadedStrings #-}

module Main where

import qualified Data.Text as T
import           Lens.Micro
import           Lens.Micro.Aeson

main :: IO ()
main = do
  putStrLn "Done"
  where
    foo :: T.Text
    foo = ("{\"foo\": \"bar\"}" :: T.Text) ^. _Object . ix "foo" . _String

but if I change the microlens import to Lens.Micro.Platform I see the same error as you.

[fosskers]

Ah, although it occurs to me that key is always better than _Object . ix:

foo :: T.Text
foo = ("{\"foo\": \"bar\"}" :: T.Text) ^. key "foo" . _String

[sjshuck]

key is always better than _Object . ix

By itself, yes. But sometimes I want to view a JSON object, and then view multiple keys within it. If I use key for each key, it pattern matches the Value variant multiple times. Can't waste those nanoseconds! 🙂

This compiles without issue

Indeed, the aeson part of my example was irrelevant; it was just being the HashMap values. However, the issue remains that microlens-aeson is defining orphan instances of Ixed for HashMap and Vector, which IMHO is the culprit of this issue. This code does not belong in this library: https://github.com/fosskers/microlens-aeson/blob/ec484a2859feb8f0857a273be87390214bcaae79/src/Lens/Micro/Aeson/Internal.hs#L45-L65

As I see it, there are two solutions:

1. Have microlens-platform as a dependency and import those instances. I can understand why you might not want to do that; the author recommends microlens-platform for apps and microlens for libs, though this might be a special case;
2. Re-implement key and nth using the code you wrote for the instances directly, and delete the instances altogether.

What do you think?

[fosskers]

(2) seems better to me.

[sjshuck]

In working on a PR and looking at the aeson 2 changes, you were right about key vs _Object . ix i. Or rather, I'm accidentally right because of a performance bug, which exists both here and in lens-aeson: Currently key is implemented as _Object . ix i, which does an unnecessary conversion to a HashMap rather than delegate to the generic KeyMap interface. Regarding this behavior of _Object, see this issue.

I'll hold off on fixing key (and members) here until they get fixed in lens-aeson, but the fix is straightforward. The sooner the better because these conversions are already likely happening in the wild.