Add flag to show docs of valid hole fits
ClosedPublic

Authored by Tritlo on Jun 14 2018, 6:00 PM.

Details

Summary

One issue with valid hole fits is that the function names can often be opaque for the uninitiated, such as ($). This diff adds a new flag, -fshow-docs-of-hole-fits that adds the documentation of the identifier in question to the message, using the same mechanism as the :doc command.

As an example, with this flag enabled, the valid hole fits for _ :: [Int] -> Int will include:

Valid hole fits include
  head :: forall a. [a] -> a
    {-^ Extract the first element of a list, which must be non-empty.-}
    with head @Int
    (imported from ‘Prelude’ (and originally defined in ‘GHC.List’))

And one of the refinement hole fits, ($) _, will read:

Valid refinement hole fits include
  ...
  ($) (_ :: [Int] -> Int)
      where ($) :: forall a b. (a -> b) -> a -> b
      {-^ Application operator.  This operator is redundant, since ordinary
          application @(f x)@ means the same as @(f '$' x)@. However, '$' has
          low, right-associative binding precedence, so it sometimes allows
          parentheses to be omitted; for example:

          > f $ g $ h x  =  f (g (h x))

          It is also useful in higher-order situations, such as @'map' ('$' 0) xs@,
          or @'Data.List.zipWith' ('$') fs xs@.

          Note that @($)@ is levity-polymorphic in its result type, so that
              foo $ True    where  foo :: Bool -> Int#
          is well-typed-}
      with ($) @'GHC.Types.LiftedRep @[Int] @Int
      (imported from ‘Prelude’ (and originally defined in ‘GHC.Base’))

Another example of where documentation can come in very handy, is when working with the lens library.

When you compile

{-# OPTIONS_GHC -fno-show-provenance-of-hole-fits -fshow-docs-of-hole-fits #-}
module LensDemo where

import Control.Lens
import Control.Monad.State

newtype Test = Test { _value :: Int } deriving (Show)

value :: Lens' Test Int
value f (Test i) = Test <$> f i

updTest :: Test -> Test
updTest t = t &~ do
    _ value (1 :: Int)

You get:

Valid hole fits include
  (#=) :: forall s (m :: * -> *) a b.
          MonadState s m =>
          ALens s s a b -> b -> m ()
    {-^ A version of ('Control.Lens.Setter..=') that works on 'ALens'.-}
    with (#=) @Test @(StateT Test Identity) @Int @Int
  (<#=) :: forall s (m :: * -> *) a b.
           MonadState s m =>
           ALens s s a b -> b -> m b
    {-^ A version of ('Control.Lens.Setter.<.=') that works on 'ALens'.-}
    with (<#=) @Test @(StateT Test Identity) @Int @Int
  (<*=) :: forall s (m :: * -> *) a.
           (MonadState s m, Num a) =>
           LensLike' ((,) a) s a -> a -> m a
    {-^ Multiply the target of a numerically valued 'Lens' into your 'Monad''s
        state and return the result.

        When you do not need the result of the multiplication, ('Control.Lens.Setter.*=') is more
        flexible.

        @
        ('<*=') :: ('MonadState' s m, 'Num' a) => 'Lens'' s a -> a -> m a
        ('<*=') :: ('MonadState' s m, 'Num' a) => 'Control.Lens.Iso.Iso'' s a -> a -> m a
        @-}
    with (<*=) @Test @(StateT Test Identity) @Int
  (<+=) :: forall s (m :: * -> *) a.
           (MonadState s m, Num a) =>
           LensLike' ((,) a) s a -> a -> m a
    {-^ Add to the target of a numerically valued 'Lens' into your 'Monad''s state
        and return the result.

        When you do not need the result of the addition, ('Control.Lens.Setter.+=') is more
        flexible.

        @
        ('<+=') :: ('MonadState' s m, 'Num' a) => 'Lens'' s a -> a -> m a
        ('<+=') :: ('MonadState' s m, 'Num' a) => 'Control.Lens.Iso.Iso'' s a -> a -> m a
        @-}
    with (<+=) @Test @(StateT Test Identity) @Int
  (<-=) :: forall s (m :: * -> *) a.
           (MonadState s m, Num a) =>
           LensLike' ((,) a) s a -> a -> m a
    {-^ Subtract from the target of a numerically valued 'Lens' into your 'Monad''s
        state and return the result.

        When you do not need the result of the subtraction, ('Control.Lens.Setter.-=') is more
        flexible.

        @
        ('<-=') :: ('MonadState' s m, 'Num' a) => 'Lens'' s a -> a -> m a
        ('<-=') :: ('MonadState' s m, 'Num' a) => 'Control.Lens.Iso.Iso'' s a -> a -> m a
        @-}
    with (<-=) @Test @(StateT Test Identity) @Int
  (<<*=) :: forall s (m :: * -> *) a.
            (MonadState s m, Num a) =>
            LensLike' ((,) a) s a -> a -> m a
    {-^ Modify the target of a 'Lens' into your 'Monad''s state by multipling a value
        and return the /old/ value that was replaced.

        When you do not need the result of the operation, ('Control.Lens.Setter.*=') is more flexible.

        @
        ('<<*=') :: ('MonadState' s m, 'Num' a) => 'Lens'' s a -> a -> m a
        ('<<*=') :: ('MonadState' s m, 'Num' a) => 'Iso'' s a -> a -> m a
        @-}
    with (<<*=) @Test @(StateT Test Identity) @Int
  (Some hole fits suppressed; use -fmax-valid-hole-fits=N or -fno-max-valid-hole-fits)

Which allows you to see at a glance what opaque operators like (<<*=) and (<#=) do.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Tritlo created this revision.Jun 14 2018, 6:00 PM
Tritlo retitled this revision from Add flag to show docs of valid hole fits One issue with valid hole fits is that the function names can often be opaque for the uninitiated, such as ($). This diff adds a new flag, `-fshow-docs-of-hole-fits` that adds the documentation of the... to Add flag to show docs of valid hole fits.Jun 14 2018, 6:01 PM
Tritlo edited the summary of this revision. (Show Details)
Tritlo edited the summary of this revision. (Show Details)
Tritlo updated this revision to Diff 16924.Jun 14 2018, 6:05 PM
  • Fix typo in documentation of flag
alexbiehl added a subscriber: alexbiehl.EditedJun 15 2018, 1:36 AM
alexbiehl added a subscriber: sjakobi.

@sjakobi We have the first consumer of :doc here!

Tritlo added a comment.EditedJun 15 2018, 2:34 AM

@sjakobi @alexbiehl do you know what's going on with the test errors? They all seem to be variants of

Bad interface file: T11274.hi
    T11274.hi: openBinaryFile: does not exist (No such file or directory)

or

Compile failed (exit code 1) errors were:
[1 of 2] Compiling ValidHoleFits    ( ValidHoleFits.hs, ValidHoleFits.o )
[2 of 2] Compiling Foo              ( valid_hole_fits.hs, valid_hole_fits.o )
attempting to use module ‘main:Foo’ (valid_hole_fits.hs) which is not loaded

Neither of which seem to be related to how I'm using the getDocs function.

@sjakobi @alexbiehl do you know what's going on with the test errors? They all seem to be variants of

Bad interface file: T11274.hi
    T11274.hi: openBinaryFile: does not exist (No such file or directory)

or

Compile failed (exit code 1) errors were:
[1 of 2] Compiling ValidHoleFits    ( ValidHoleFits.hs, ValidHoleFits.o )
[2 of 2] Compiling Foo              ( valid_hole_fits.hs, valid_hole_fits.o )
attempting to use module ‘main:Foo’ (valid_hole_fits.hs) which is not loaded

Neither of which seem to be related to how I'm using the getDocs function.

I suspect that the errors are related to the following comment on getModuleInterface on which hscGetModuleInterface relies:

-- | ASSUMES that the module is either in the 'HomePackageTable' or is
-- a package module with an interface on disk.  If neither of these is
-- true, then the result will be an error indicating the interface
-- could not be found.

Maybe there's a way to get the module interface that works for your usecases too?

Tritlo added a comment.EditedJun 15 2018, 6:55 AM

Right, @sjakobi I can handle that for my usecase, but shouldn't it generally be a NoDocsInIface error as well?

An alternative method to get the documentation, would be to use extractDocs on the existing TcGblEnv.

Tritlo updated this revision to Diff 16934.Jun 15 2018, 9:05 AM
  • Add IFaceLoadError handling to getDocs and use extractDocs for local fits

I appreciate the changes to getDocs but I have some qualms about the import of InteractiveEval from typecheck/.

compiler/typecheck/TcHoleErrors.hs
41

So far the only module importing InteractiveEval has been GHC, and I think it would be better to keep it this way.

As I do think that this is a valid use of getDocsIO, this raises the question of where getDocs[IO] should be moved? To some place in iface/ maybe?

Tritlo added a comment.EditedJun 15 2018, 9:54 AM

extractDocs would only work for bindings local to the module.

Thanks for letting me modify your implementation @sjakobi! Your the feature owner, so I'll make any changes you like :)

compiler/typecheck/TcHoleErrors.hs
41

I agree. Would it fit in HsDoc? That would seem like a natural place for it to be.

sjakobi added inline comments.Jun 15 2018, 10:13 AM
compiler/typecheck/TcHoleErrors.hs
41

That would introduce circular dependencies involving HscTypes where ModIface is defined.

Tritlo updated this revision to Diff 16990.Jun 17 2018, 4:50 PM
  • Refactor to remove dependency on InteractiveEval and hscGetModInterface

Now the only shared function is the ifaceLookupName function. There is still a ghci-leak
in T8353 that I have yet to debug.

sjakobi requested changes to this revision.Jun 17 2018, 5:27 PM
sjakobi added inline comments.
compiler/iface/TcIface.hs
21 ↗(On Diff #16990)

As ifaceLookupDocs has nothing to do with typechecking, I fear TcIface isn't the best place for it.

Can someone suggest an appropriate place for it please? @bgamari maybe?

If we currently have no fitting module, can we maybe add iface/IfaceDocs.hs or something like that?

1813 ↗(On Diff #16990)

Can you add a comment explaining the return type? When do we get Nothing? What are the tuple fields?

This revision now requires changes to proceed.Jun 17 2018, 5:27 PM
Tritlo updated this revision to Diff 16991.EditedJun 17 2018, 5:57 PM
Tritlo marked 2 inline comments as done.
  • Rollback changes to getDocs

The scope was getting too big on the changes, so I've removed the getDocs changes.

Tritlo marked 3 inline comments as done.Jun 17 2018, 5:57 PM
Tritlo added inline comments.
compiler/iface/TcIface.hs
21 ↗(On Diff #16990)

It would be weird for me to make decisions like that. Removed this change for later patch.

1813 ↗(On Diff #16990)

Removed this function for later patch.

sjakobi accepted this revision.Jun 17 2018, 6:03 PM

LGTM.

This revision is now accepted and ready to land.Jun 17 2018, 6:03 PM
Tritlo added a comment.EditedJun 19 2018, 4:15 AM

@bgamari would it be possible to sneak this in with 8.6? It's very small and hidden behind the -fshow-docs-of-hole-fits flag, so it shouldn't pose a risk.

I am quite happy to see @sjakobi's documentation getting used!

However, I'll admit, I'm a bit nervous about all of these printer-printer flags. That's not to say that this sort of thing is unprecedented; we already have -fexpand-type-synonyms, -fprint-explicit-kinds, etc. However, I am a bit concerned that continuing down this road will mean that there will be pressure to keep shoehorning more and more features into GHC which are likely better implemented elsewhere (e.g. a proper REPL or IDE). Furthermore, it's not entirely clear to me that this particular flag will often be useful. GHC error messages are already quite long; it seems to me like adding documentation into the mix will only exacerbate this in the majority of cases.

However, given that we currently lack the means to expose the sort of information that an IDE would require to show this sort of documentation (short of the IDE parsing it out of the error message), I am okay with letting this patch in if others feel it's useful. Afterall, I've not yet had the pleasure of working on a project with the benefit of your improved hole fits support; perhaps I'll be surprised.

Regardless of what happens here, my hope is that this progress on Trac #8809 will give us better tools to deal with things like this in the future. More structured errors will allow us to both keep these user-interface decisions out of GHC and provide a significantly better user experience to boot.

Tritlo edited the summary of this revision. (Show Details)Jun 20 2018, 8:15 AM
Tritlo edited the summary of this revision. (Show Details)

@bgamari I agree. Is there any other way to pass this information except via flags? I have it behind a flag precisely so that the length of the error message can be controlled by the user, as having the documentation along by default would be way too much. I'll be sure to update the valid hole fit output to utilize the tools that Trac #8809 will hopefully bring.

The example I took was maybe not the best. A better example would be opaque operators such as those of the lens library, like (<<*=). I've updated the description to show this example.

This revision was automatically updated to reflect the committed changes.