Warn about unused packages
Needs ReviewPublic

Authored by Yuras on Oct 30 2018, 10:51 AM.

Details

Reviewers
bgamari
simonpj
Trac Issues
#15838
Yuras created this revision.Oct 30 2018, 10:51 AM

This probably requires tests yet.

Does this work when a module is not recompiled because it has not changed? In other words, what is the result of running ghc with this flag enabled twice in a row?

Yuras updated this revision to Diff 18533.Oct 30 2018, 11:21 AM
  • Fix the check

@mpickering To my surprice, it seems to work when run the second time. I'm not sure why. Anyway, the idea is to use this flag on a clean code base.

hvr awarded a token.Oct 30 2018, 11:31 AM
simonpj accepted this revision.Oct 31 2018, 5:08 PM
simonpj added a subscriber: simonpj.

Needs a test case or two.

Broadly looks useful to me. Thanks

compiler/main/GhcMake.hs
281

I wonder if this message is self-explanatory enough. I think the warning is that you have passed too many -package flags to GHC. But that's not apparent. Reword? Perhaps "The following -package flags are redundant:" or something like that.

This revision is now accepted and ready to land.Oct 31 2018, 5:08 PM
Yuras updated this revision to Diff 18553.Nov 1 2018, 7:09 AM
  • Change message, add test
Yuras added a comment.EditedNov 1 2018, 7:21 AM

@simonpj I added a test and reworded the message:

The following packages were specified via -package or -package-id flags, but were not needed for compilation:

I still have false positivities for wired-in packages, like base, when specified via -package-id. E.g. -package=base and -package=base-4.12.0.0 work, but -package-id=base-4.12.0.0 doesn't. I guess the matching function from Packages module is buggy. Or am I using wrong function to do comparision?

Yuras planned changes to this revision.Nov 1 2018, 8:34 AM
Yuras marked an inline comment as done.

I wasn't able to quickly find the reason for the false positivities, so lets change status to prevent accidental merge.

Yuras updated this revision to Diff 18557.Nov 1 2018, 11:45 AM
  • Fix wired-in package handling
This revision is now accepted and ready to land.Nov 1 2018, 11:45 AM
Yuras added a comment.Nov 1 2018, 11:51 AM

Ok, I fixed the issue with wired-in packages. I inlined the matching function from Packages and modified it. Not sure whether the original function should be fixed instead.

Also it probably worth noting that this warning won't report indirect dependencies because they (packages, not flags) are *actually* required for compilation.

bgamari accepted this revision.Nov 1 2018, 5:24 PM

This is fantastic! Thank you for doing this, @Yuras. pack-unused always seemed like a unsatisfactory solution for something that GHC should do on its own. Moreover, I am pleasantly surprised by how simple this was to implement.

bgamari requested changes to this revision.Nov 1 2018, 5:26 PM

I think more tests are needed, however. In particular, we should have a test verifying that this works in response to various recompilation conditions (e.g. a module not being changed, as @mpickering points out).

This revision now requires changes to proceed.Nov 1 2018, 5:26 PM

Also, can you add a note about this to the 8.8 release notes? This is a great feature; it would be a shame if it went unnoticed.

Yuras updated this revision to Diff 18605.Nov 6 2018, 12:02 PM
  • Add release note
Yuras added a comment.Nov 6 2018, 12:09 PM

@bgamari I added a release note.

Regarding test cases with recompilation, I have no idea how to add them. It probably requires multimodule test, that will be executed twice, touching one module between the runs to force relinking. I'm not familiar with GHC's test framework enough to implement that. Sorry.

And, probably more important, I still don't understand why it actually works on partial recompilation. There could be corner cases when it doesn't work. I'd prefer not to advertise this at all and assume it works only on a clean code base.

You could make the flag imply -fforce-recomp but if we don't understand how the patch works for the partial recompilation case then I would suggest that we don't understand what the code is doing well enough.

Yuras added a comment.Nov 7 2018, 2:07 PM

@mpickering I understand your concern, but IMO it is completely clear what the patch does in case of a clean build. It is not clear why GHC loads all packages when doing partial recompilation, and I need someone's help to figure it out. Do you have any idea? My best guess is that packages are necessary for linking executable/library, but I'm not familiar with this part of GHC. As you probably know, I contribute only occasionally, and my knowledges are sparse.

I don't have an opition regarding -fforce-recomp. I'll try to make -Wunused-packages imply -fforce-recomp if you think it is necessary.

Actually I expected conserns regarding indirect dependencies support, but looks like everybody is OK with it (and so I am). Or did my comment go unnoticed? I mean this one: https://phabricator.haskell.org/D5285#145838

Yuras added a comment.Nov 8 2018, 5:25 AM

@mpickering On other hand, why should we force user to recompile all the code if she can get results immediately, even if less occurate? So I'm -0.5 for enabling -fforce-recomp by default, I think it is enough to suggest it in the user guide. @bgamari, what do you think about that?

Of cource I'll enable -fforce-recomp if everyone prepers it.

hvr added a subscriber: hvr.EditedWed, Nov 21, 3:19 AM
In D5285#146323, @Yuras wrote:

So I'm -0.5 for enabling -fforce-recomp by default, I think it is enough to suggest it in the user guide.

I'm -1 for enabling -fforce-recomp by default as it makes it harder to enable this warning by default in your pkg.cabal or cabal.project or even the global cabal user config.

Moreover, I'd think it'd set a bad precedent that a -W flag triggers recompilation; warning flags ought to passive flags which merely expose additional hints (unless -Werror is involved) but don't cause "active" effects. Also, occasionally warnings are inaccurate -- that's not too surprising either.

Long story short: Let's just add a warning (pun intended) to the user-guide about the potential gotchas/limitations of this warning flag, maybe even declare it an "experimental" feature for now. I'd really like to see this in GHC 8.8 so we can gather real-world feedback.

PS: If we're worried about inaccurate warnings, we could prefix warnings emitted from -Wunused-packages by a disclaimer along the lines of partial recompilation detected; this warning might not be accurate for partial recompilations; consider using '-fforce-recomp'

Sorry for the delay, @Yuras.

In D5285#147665, @hvr wrote:

I'm -1 for enabling -fforce-recomp by default as it makes it harder to enable this warning by default in your pkg.cabal or cabal.project or even the global cabal user config.

Moreover, I'd think it'd set a bad precedent that a -W flag triggers recompilation; warning flags ought to passive flags which merely expose additional hints (unless -Werror is involved) but don't cause "active" effects. Also, occasionally warnings are inaccurate -- that's not too surprising either.

I absolutely agree with this. Warning flags should not change compiler behavior.

Long story short: Let's just add a warning (pun intended) to the user-guide about the potential gotchas/limitations of this warning flag, maybe even declare it an "experimental" feature for now. I'd really like to see this in GHC 8.8 so we can gather real-world feedback.

I am sympathetic to this line of thinking. My only concern is that there is currently no plan to resolve these limitations but I agree that something is better than nothing and consequently we should just merge it.

PS: If we're worried about inaccurate warnings, we could prefix warnings emitted from -Wunused-packages by a disclaimer along the lines of partial recompilation detected; this warning might not be accurate for partial recompilations; consider using '-fforce-recomp'

How would we know whether the recompilation is partial? AFAICT the best we can do is test for the use of -fforce-recomp and suppress the disclaimer if it is used.

Yuras added a comment.Mon, Dec 3, 9:14 AM

Oops, I missed notifications for this Diff somehow, sorry.
@bgamari so what actions should I perform to get it merged? Suggest -fforce-recomp in user guide? Anything else?

In D5285#148953, @Yuras wrote:

Oops, I missed notifications for this Diff somehow, sorry.
@bgamari so what actions should I perform to get it merged? Suggest -fforce-recomp in user guide? Anything else?

I would do the following:

  • Mention in the users guide that this feature needs to be used with -fforce-recomp, and
  • Throw a warning if -Wunused-packages is used without -fforce-recomp.
Yuras updated this revision to Diff 19075.Mon, Dec 10, 11:24 AM
  • Mention -fforce-recomp in the user guide

Throw a warning if -Wunused-packages is used without -fforce-recomp.

@bgamari I don't think it is a good idea because often the warning will be enabled on CI. So user is doing a clean build anyway, and the warning on -fforce-recomp will be completely wrong and annoying.

In D5285#149745, @Yuras wrote:

Throw a warning if -Wunused-packages is used without -fforce-recomp.

@bgamari I don't think it is a good idea because often the warning will be enabled on CI. So user is doing a clean build anyway, and the warning on -fforce-recomp will be completely wrong and annoying.

Perhaps, but I think it's much worse to mislead the user into thinking that their code is clean when a full recompilation will reveal otherwise. In general compilers should be pure functions from source code to warnings and compilation results. Any departure from this deserves to be documented loudly. However, I'm willing to defer to others if I am outnumbered on this matter.