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.

Sign up for GitHub

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

Jump to bottom

Use the Ghc monad to emit warnings #969

Closed

peddie wants to merge 4 commits into haskell:ghc-head from peddie:ghc-warning-output

peddie commented Nov 13, 2018

Previously Haddock warnings were emitted directly to stdout via
putStrLn. This commit changes the output to go via the Ghc monad
as a new type of Haddock warning. To take advantage of using Ghc in
this way, source locations are now tracked and emitted as part of most
warning messages.

This is an attempt to address #927.

The corresponding GHC commits are here and here.

peddie mentioned this pull request

add option to turn warnings into errors #927

Open

harpocrates requested changes

View reviewed changes

Collaborator

harpocrates left a comment

Looks great, especially for a first time contribution! I've left a bunch of inline comments, but almost all of them are just two main points repeated over

threading moduleLocation through to as the location of more warnings messages
updating warning messages to not start with "Warning:" and the location - GHC will add that on for you

Also, this PR is against haskell:wip/shnajd-TTG-SrcLocs - mind re-opening it against ghc-8.6 or ghc-head?

I understand you are working with a limited time budget, so do let me know if you run out of hours - I can help finish off what is left.

Thanks!

haddock-api/src/Haddock/Interface.hs Outdated

+              haddockWarningToGhcWarning :: DynFlags -> ErrMsg -> WarnMsg
+              haddockWarningToGhcWarning dflags (L loc doc) =
+                makeIntoWarning (Reason Opt_WarnMalformedHaddock) $

Collaborator

harpocrates Nov 13, 2018

Convincing the GHC folks to accept a new Opt_WarnMalformedHaddock flag just for Haddock just for this seems like a bit of a stretch. Could we switch this to NoReason for now? You'll still be able to get Haddock to error out on warnings with -Werror.

My plan is to add Opt_WarnMalformedHaddock in https://phabricator.haskell.org/D5057 (where there is a legitimate GHC-side use case) then sneakily reuse it here. I doubt I'll finish that before 8.8 though.

haddock-api/src/Haddock/Interface.hs Outdated

                                           , isTcOcc (nameOccName name)   -- Types and classes only
                                           , unQualOK gre ]               -- In scope unqualified
-                  liftIO $ mapM_ putStrLn (nub msgs)
+                  -- liftIO $ mapM_ putStrLn (nub msgs)

Collaborator

harpocrates Nov 13, 2018

Let's get rid of this comment.

haddock-api/src/Haddock/Interface/Create.hs Outdated

@@ @@ -99,18 +100,18 @@ createInterface tm flags modMap instIfaceMap = do @@
                 -- Cabal can be trusted to pass the right flags, so this warning should be
                 -- mostly encountered when running Haddock outside of Cabal.
                 when (isNothing pkgName) $
-                  liftErrMsg $ tell [ "Warning: Package name is not available." ]
+                  liftErrMsg $ tell [L moduleLocation "Warning: Package name is not available." ]

Collaborator

harpocrates Nov 13, 2018

The "Warning: " is superfluous. You end up displaying messages that look like

Example.hs:1:1: warning: Warning: Package name is not available.
  |
1 | module Example where
  | ^

haddock-api/src/Haddock/Interface/Create.hs Outdated

                 -- The renamed source should always be available to us, but it's best
                 -- to be on the safe side.
                 (group_, imports, mayExports, mayDocHeader) <-
                   case renamedSource tm of
                     Nothing -> do
-                      liftErrMsg $ tell [ "Warning: Renamed source is not available." ]
+                      liftErrMsg $ tell [L moduleLocation "Warning: Renamed source is not available." ]

Collaborator

harpocrates Nov 13, 2018

As before, the "Warning: " is superfluous.

haddock-api/src/Haddock/Interface/Create.hs Outdated

                 opts <- case mbOpts of
                   Just opts -> case words $ replace ',' ' ' opts of
-                    [] -> tell ["No option supplied to DOC_OPTION/doc_option"] >> return []
+                    [] -> tell [L loc "No option supplied to DOC_OPTION/doc_option"] >> return []

Collaborator

harpocrates Nov 13, 2018

I know this isn't your doing, but that error message needs to be revised (how old is it!?). The pragma is called OPTIONS_HADDOCK and the GHC option is called -haddock-opts.

Author

peddie Nov 14, 2018

Would you prefer that I make this change in a separate commit so the commits remain atomic / minimal?

Collaborator

harpocrates Nov 14, 2018

I intend to squash before merging, so it shouldn't make a difference.

haddock-api/src/Haddock/Interface/Create.hs Outdated

@@ @@ -964,7 +965,8 @@ moduleExport thisMod dflags ifaceMap instIfaceMap expMod = @@
                         Just iface -> return [ ExportModule (instMod iface) ]
                         Nothing -> do
                           liftErrMsg $
-                            tell ["Warning: " ++ pretty dflags thisMod ++ ": Could not find " ++
+                            tell [dislocatedErrMsg $
+                                  "Warning: " ++ pretty dflags thisMod ++ ": Could not find " ++

Collaborator

harpocrates Nov 13, 2018

The "Warning: " ++ pretty dflags thisMod ++ is superfluous. Let's also pass the moduleLocation here.

haddock-api/src/Haddock/Interface/Create.hs Outdated

@@ @@ -1200,14 +1202,15 @@ mkMaybeTokenizedSrc dflags flags tm @@
                           tokens <- liftGhcToErrMsgGhc (liftIO (mkTokenizedSrc dflags summary src))
                           return $ Just tokens
                       Nothing -> do
-                          liftErrMsg . tell . pure $ concat
+                          liftErrMsg . tell . pure . L moduleLocation $ concat
                               [ "Warning: Cannot hyperlink module \""

Collaborator

harpocrates Nov 13, 2018

As before, lets get rid of the "Warning: " in the message.

haddock-api/src/Haddock/Interface/Create.hs Outdated

               findNamedDoc name = search
                 where
                   search [] = do
-                    tell ["Cannot find documentation for: $" ++ name]
+                    tell [dislocatedErrMsg $ "Cannot find documentation for: $" ++ name]

Collaborator

harpocrates Nov 13, 2018

Let's also pass the moduleLocation here.

haddock-api/src/Haddock/Interface/LexParseRn.hs Outdated

-                          "    If you qualify the identifier, haddock can try to link it\n" ++
-                          "    it anyway."]
+                    tell $ fmap (rdrNameErrMsg x)
+                      ["Warning: '" ++ showPpr dflags a ++ "' is out of scope.\n" ++

Collaborator

harpocrates Nov 13, 2018

I'd get rid of rdrNameErrMsg here - it is always going to noLoc (as the comment on the Exact case indicates).

As before, the "Warning: prefix in the error message is redundant.

haddock-api/src/Haddock/Interface/LexParseRn.hs Outdated

@@ @@ -192,7 +193,7 @@ ambiguous dflags x gres = do @@
                 -- of the same name, but not the only constructor.
                 -- For example, for @data D = C | D@, someone may want to reference the @D@
                 -- constructor.
-                when (length noChildren > 1) $ tell [msg]
+                when (length noChildren > 1) $ tell $ [rdrNameErrMsg x msg]

Collaborator

harpocrates Nov 13, 2018

As before, the "Warning: " prefix in msg is redundant.

Matt Peddie and others added 2 commits

November 14, 2018 10:00


          Use the Ghc monad to emit warnings

66efc5f

Previously Haddock warnings were emitted directly to `stdout` via
`putStrLn`.  This commit changes the output to go via the `Ghc` monad
as a new type of Haddock warning.  To take advantage of using `Ghc` in
this way, source locations are now tracked and emitted as part of most
warning messages.


          Part 1 of PR fixups

249e6d3

peddie changed the base branch from wip/shnajd-TTG-SrcLocs to ghc-head

November 14, 2018 12:59

peddie force-pushed the ghc-warning-output branch from af5b36a to 92d9469 Compare

November 14, 2018 13:00

peddie added 2 commits

November 14, 2018 23:06


          Part 2 of PR fixups

7a6cbea


          Plumb module locations to out-of-scope and ambiguous warnings

b94262a

peddie force-pushed the ghc-warning-output branch from 92d9469 to b94262a Compare

November 14, 2018 13:37

Author

peddie commented Nov 14, 2018 •

edited

Sorry for the delay. I think I've addressed all the requested changes. In testing the changes, I realised that the out-of-scope and ambiguous identifier warnings changed from

Warning: 'mapAccumR' is out of scope.
    If you qualify the identifier, haddock can try to link it anyway.

to

<no location info>: warning:
    'mapAccumR' is out of scope.
    If you qualify the identifier, haddock can try to link it anyway.

If I were a user, I'd be annoyed to receive the latter ("thanks for pointing out that neither of us knows where this problem is, Haddock"), so I added the last commit in the series, which plumbs through module location info. Now the messages look like:

libraries/containers/Data/IntMap/Internal.hs:1:1: warning:
    'mapAccumR' is out of scope.
    If you qualify the identifier, haddock can try to link it anyway.
  |
1 | {-# LANGUAGE CPP #-}
  | ^

Unfortunately, as you can see, this always grabs the source span at the beginning of the file in question, which usually has nothing to do with the link destinations. Doing better seems like it would require the additional location info I gave up on earlier. What do you think?

Author

peddie commented Nov 14, 2018

I've made my patch against the ghc-head branch, but it looks like the tests are failing due to lack of a field introduced to GHC a few weeks ago. Is it possible the test suite is testing against 8.6 or an outdated HEAD branch of the compiler?

Collaborator

harpocrates commented Nov 15, 2018

I've made my patch against the ghc-head branch, but it looks like the tests are failing due to lack of a field introduced to GHC a few weeks ago. Is it possible the test suite is testing against 8.6 or an outdated HEAD branch of the compiler?

Don't worry about CI on ghc-head; it hardly ever works. I'll test locally when I review (which should be in the next couple of days - I did notice that you addressed my comments, I've just not found time to give the final thorough review it deserves).

Collaborator

harpocrates commented Nov 20, 2018

This looks great!

I'm not going to merge just yet because I'd like to wait for the next GHC release cycle in order to dogfood on HEAD for a while (the new 8.8 release just got cut). I'm a bit concerned that we might end up breaking the workflows of people who are using -Werror. That is especially a problem if we don't have a specific flag with which to disable just Haddock warnings or if there are some warnings in Haddock which cannot be avoided (I think there might be some nasty edge cases... for instance missing links to methods which are mentioned in MINIMAL pragmas but are otherwise not exported).

Author

peddie commented Nov 20, 2018

Thanks for the review and thanks again for your guidance while working on this feature. Is there anything else you need from me now or in the future to ensure that this goes in smoothly?

I assume once Opt_WarnMalformedHaddock makes it into DynFlags.hs, it's possible to disable only Haddock warnings or prevent only Haddock warnings from being errors?

Collaborator

harpocrates commented Nov 20, 2018 via email

I don’t think so. It might be necessary to rebase at some point (depending on how long it takes me to finish the motivating patch for `Opt_WarnMalformedHaddock`). I’ll ping you if so. Yes, once `Opt_WarnMalformedHaddock` makes it into DynFlags, it’ll be possible to disable Haddock warnings, or prevent just Haddock warnings from being errors. I think it should even be possible to do this on a module-by-module basis with `OPTIONS_GHC`. Thanks again for all your hard work!

On Nov 20, 2018, at 2:41 AM, Matt Peddie ***@***.***> wrote: Thanks for the review and thanks again for your guidance while working on this feature. Is there anything else you need from me now or in the future to ensure that this goes in smoothly? I assume once Opt_WarnMalformedHaddock makes it into DynFlags.hs, it's possible to disable only Haddock warnings or prevent only Haddock warnings from being errors? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#969 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKRHAf3a21bXHodqACQePruZkp-zCIKtks5uw9xVgaJpZM4YbCs3>.

harpocrates mentioned this pull request

--show-interface outputs to stderr instead of stdout #864

Closed

guibou mentioned this pull request

Ghc more detailled missing symbols #1118

Merged

Contributor

guibou commented Nov 27, 2019

Hello.

Any news about this?

I'm regularly rebasing these changes on top of my current GHC and that's a painful process. What can I do to help getting this merged?

Contributor

simonmichael commented Mar 7, 2020

Ping ? Any movement on haddock's logging would be great. Perhaps this would also fix the -w/--no-warnings flag, which I think haddock currently ignores ?

Collaborator

harpocrates commented Mar 23, 2020

We are actually very close to this! There is a GHC patch that will introduce a Haddock related warning flag to GHC's flags, and it is close to being merged: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/2377.

@guibou I apologize for the delay here - the blocker was that without a flag under which to register these warnings, it won't be possible to turn them on or off directly. This would be very annoying in the presence of -Werror. Also, don't worry about rebasing changes - I can help with that part. Thanks again for putting this together.

harpocrates self-requested a review

March 27, 2020 21:36

Contributor

guibou commented Jul 2, 2020 •

edited

@harpocrates what is the status of this pull request?

At work, I'm rebasing this on each GHC release. I'm starting to think about another approach (using a post processing of the haddock cli output) to detect error and I'm wondering where my motivation will be best used.

So what is missing for that PR to be merged? You discussed the need of a flag, what is the current status of this flag? Are you interested by a rebase on top of haddock master? If not, are we closing this PR as "won't fix"?

Member

Kleidukos commented Feb 7, 2021

@peddie Does this PR affect non-GHC consumers? If no, could you please re-open it targeting the ghc-9.0 branch so that all may benefit from it?

Author

peddie commented Feb 7, 2021

@Kleidukos I think this PR could affect anything that's used to the current format of Haddock errors -- is that what you're asking?

I made this patch over 2 years ago, and unfortunately I do not currently have the time to continue working on it. It's difficult for me to prioritize tasks that have already been "accepted" and then left untouched by the maintainers for 2 years and several GHC releases. I hope someone with more time and/or the ability to directly merge the patch when it's ready can find the time to update it and land it so that all may benefit, as you say.

Member

Kleidukos commented Feb 7, 2021

@Kleidukos I think this PR could affect anything that's used to the current format of Haddock errors -- is that what you're asking?

Yes. The ghc-head branch is reserved for the GHC-specific patches, like glue code. Patches to the documentation generator should go toward the current GHC release branch, which is currently ghc-9.0.

I made this patch over 2 years ago, and unfortunately I do not currently have the time to continue working on it. It's difficult for me to prioritize tasks that have already been "accepted" and then left untouched by the maintainers for 2 years and several GHC releases. I hope someone with more time and/or the ability to directly merge the patch when it's ready can find the time to update it and land it so that all may benefit, as you say.

That's understandable. Thanks for your contribution, I am certain it will live under one shape or another in the codebase one day.

Have a nice day!

Kleidukos closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment