Detect module imports from transitive dependencies

[colinwahl]

This intends to close #598

Description of the change

Adds a step to build to check if a source file imports a module that is part of a transitive dependency.
In that case, we fail the build and report those packages to the user.

Also, adds a step to build which warns on unused dependencies.

Checklist:

• Added the change to the "Unreleased" section of the changelog
• Added some example of the new feature to the README
• Added a test for the contribution (if applicable)

[colinwahl]

Notably left out of this is the check for unused dependencies.

I was talking about that with @thomashoneyman and he mentioned that we may not want to fail the build on detection of unused dependencies - for instance, someone may have debug as a dependency, and they probably don't want to add/remove it from their dependency list when they want to debug something.

[colinwahl]

Right now I've only included the import check as part of build - is there anywhere else we'd like to include it? (such as repl)

[f-f]

Wooow, great work here @colinwahl! 👏

The implementation is not yet covering all the cases, so I left some comments to detail the missing parts, but we're definitely on the right path.

Another minor note: as I specified in #723 (which after this will become a piece of cake!), it's important that we skip this check if the purs version is lower than 0.13.8.
I'd store the version (note that we already have code to get it) inside the PursCmd - in this way we could figure it out at startup, here

[colinwahl]

I got quite busy and haven't had a chance to update this branch yet - I plan on having some more time this week to keep moving on this.

[colinwahl]

@f-f I was running this on my work codebase and also creating a test when I realized that purs graph seems to be including the transitive closure of the module imports for a given module in the depends entry, instead of only listing the modules that are actually imported from that module.

I'm not sure if purs graph was meant to do that - I did some looking in purescript/purescript#3781 and this is the behavior of sortModules because of https://github.com/purescript/purescript/blob/master/src/Language/PureScript/ModuleDependencies.hs#L47

That being said, I don't think that we can implement this check with the current implementation of purs graph - I can't distinguish between a module importing a module or one of a modules imported modules importing a module.

Was this behavior of purs graph intended? If not, I can log an issue for https://github.com/purescript/purescript that we can look in to.

Edit: I just read the release notes https://github.com/purescript/purescript/releases/tag/v0.13.8 and it looks like this is not intended - I logged purescript/purescript#3991

[colinwahl]

With the release of PureScript 0.14, I can continue work on this. I hope to pick it back up soon!

[f-f]

@colinwahl is there something I can do to help with this?

[colinwahl]

@f-f To be honest I had lost where I was on this!

I think I was mostly stuck on the tests before: I was having trouble getting the fixture file to match the output, even though the output was what I wanted. I've fixed that now!

Another thought I had: spago init immediately drops the user in to a project that will no longer build with this change! That is because src/Main.purs imports Prelude, but prelude isn't specified as a dependency (it is a transitive dependency because of effect). Should I update the default dependencies?

[f-f]

Should I update the default dependencies?

@colinwahl yes please! It shows that this change is needed so badly since we got the dependencies wrong even in the default project 😄😄

[colinwahl]

Ah, the tests are failing because the purs version we use in tests is 0.13.8 - this code doesn't actually run on that version.

Should I bump the version in the tests? I don't know if there's a plan to test against multiple versions of purs or anything. Maybe that'd be worth it - and I could edit the test to do the version check before expecting failures

[colinwahl]

Okay, I did some investigation into the build failures, and it's caused by how I'm handling UsePsa - looking into it also brought up another confusion that I wanted to clarify:

My confusion is with the implementation of Spago.Purs.compile - it uses the pursCmd, in this case that's psa. Then it calls psa compile ... - psa already is a wrapper around purs compile, so I think that will always complain about purs compile: No files found using pattern: compile

Should it handle when psa is available by removing the compile portion of the command?

The failures in the appveyor are caused because pursCmd is set to psa, and then it is trying to run psa graph - which isn't a command.

I think that I'll modify PursCmd to be

{ purs :: Text
, psa :: Maybe Text
, compilerVersion :: Version.SemVer
} deriving (Generic)

And if UsePsa is enabled, I'll properly handle calling psa for Spago.Purs.compile, else I'll use purs. Does that sound reasonable?

[thomashoneyman]

That explains the issue with purs compile: No files found using pattern: compile that's been cropping up. I do think that should be fixed as you described. I can't speak for @f-f, but what you've suggested seems reasonable to me.

[f-f]

Great call on refactoring the PursCmd type, I think this is exactly what we need!

I had a last round of review and I left a bunch of minor comments, I think we're good to merge once we take care of them. Great work! 🙂

[colinwahl]

@f-f I've addressed your review comments and updated the changelog to reflect everything that got done!

[f-f]

@colinwahl thank you for the great work, this is perfect now! 👏 👏

I added you as a collaborator, so you can merge yourself once CI is all green 😊

[colinwahl]

Thanks @f-f !

I'm not seeing permission to merge, not sure what that's about - but I'll merge once it shows up! We usually squash-and-merge, right?

[f-f]

@colinwahl

• we only do squash-merge
• check your email, you should find an invitation from GitHub to join the repo

[colinwahl]

Oh! Duh, didn't think to check my email :D

[colinwahl]

Thanks @f-f and @thomashoneyman for the help on this!