move a bunch of types into dedicated modules #502

alpmestan · 2018-02-15T09:05:53Z

Alright, it's show time.

This is the first PR extracted out of #445, hopefully addressing the feedback you (@snowleopard) gave in comments around here. Let me know what you think.

snowleopard · 2018-02-15T11:55:11Z

src/Context/Type.hs

+
+import Hadrian.Package.Type
+import Stage.Type
+import Way


Presumably, this can be relaxed to import Way.Type?

Addressed in an upcoming commit.

snowleopard · 2018-02-15T11:56:45Z

src/Expression/Type.hs

@@ -0,0 +1,17 @@
+module Expression.Type where
+
+import Builder


Again, I think Builder.Type is sufficient.

Ah, sorry, ignore the above comment, apparently there is no Builder.Type. Or does it make sense to add it as well, for consistency?

Looking at Stage below, I think we should only add .Type modules if they help resolve a cyclic import. So, let's not add Builder.Type if it's not necessary.

Indeed I didn't add .Type modules that were not in @angerman's PR under src/Types/, and the ones that were there presumably were there to resolve cyclic imports, so I think the PR effectively implements what you converged on in your last comment, doesn't it?

EDIT: looking at the comments below, looks like it doesn't just yet =)

snowleopard · 2018-02-15T12:07:25Z

src/Hadrian/Builder/Type.hs

@@ -0,0 +1,26 @@
+module Hadrian.Builder.Type where


I'm a bit unhappy about this module: do we really need to separate the definition of builder modes and builder arguments? This looks very awkward.

If that's the only way forward, perhaps we can at least give this module a bit clearer name: Hadrian.Builder.Mode.

Addressed in an upcoming commit.

snowleopard · 2018-02-15T12:13:18Z

src/Stage/Type.hs

@@ -0,0 +1,27 @@
+module Stage.Type where


This module doesn't seem to buy us anything. Is it here only for consistency? I think I'd prefer to keep everything simply in Stage, as otherwise the latter looks really silly with a single stageString function.

Got rid of this module and put back everything into src/Stage.hs, in an upcoming commit.

snowleopard · 2018-02-15T12:16:45Z

src/Hadrian/Package/Type.hs

@@ -0,0 +1,45 @@
+module Hadrian.Package.Type where


From a quick look at Hadrian.Package, it doesn't seem that this split is necessary. Or is it?

I think it will be necessary shortly (as in, in a PR or two), and given that we're not really in a comparable situation as with the Stage/Stage.Type case (the .Package module has several functions and the package-related types are likely going to be needed by "base" modules), I would personally suggest that we leave that one as-is, but it's your call of course :)

snowleopard · 2018-02-15T12:18:52Z

src/Way/Type.hs

+import Data.List
+import Data.Maybe
+import Development.Shake.Classes
+import Hadrian.Utilities


Again, it seems that Hadrian.Utilities is the only potential source of cyclic imports. But does it really cause a cyclic import? If not, perhaps we don't need this split?

snowleopard · 2018-02-15T12:22:58Z

@alpmestan Many thanks! I've left a few comments above. Most of the comments arise because it's unclear which .Type submodules are necessary and why.

I would argue that we need to introduce a .Type submodule only if it helps to break a cyclic import. But I don't see how Stage.Type (for example) can possibly break a cycle. It only seems to complicate the code base and slows down Hadrian's build.

snowleopard · 2018-02-15T12:26:18Z

Perhaps, some cyclic imports only become obvious in the reloc branch. I suggest to add a comment to every .Type module, clarifying which particular cyclic import it is meant to resolve, perhaps attaching it exactly to the offending imports. In this way, if these imports are removed later, we would realise this and merge the modules back, simplifying the code base.

snowleopard · 2018-02-15T12:43:16Z

src/Context.hs

@@ -12,25 +12,13 @@ module Context (
    pkgConfFile, objectPath
    ) where

-import GHC.Generics
+import Context.Type


As an example of the kind of comment I'm looking for:

-- The module "Context.Type" is used to break import cycles caused by "Oracles.Setting". import Context.Type

I doubt those comments will be of much use. They will document what the initial cycle that was broken was, but will ultimately bitrot pretty quickly. Any further refactoring or addition of code to hadrian that uses the .Type modules will not see that there could have been a cycle without the .Type module and subsequently not update the comment.

As such I believe the comment while it might be initially correct will be ultimately incorrect.

Yes, I agree, these comments will likely get out-of-date quickly, and we probably don't want to keep changing the structure very often.

Instead it may be more useful to add a general comment somewhere in the overview document that explains the module structure. (We don't have such a document at the moment, so we don't need to do it now.)

I suggest we write such a description once we have integrated all of #445 as things will be in flux until we're done with that.

angerman · 2018-02-15T15:12:37Z

While I would have preferred to completely separate Data Types and logic in the reloc branch, I opted to only separate the ultimately necessary data types out. I don't think it would be a bad idea to follow this practice through though, as that will ultimately prevent some cycles from occuring in the first place. The occurance of cycles and GHCs inability to deal with them in the first place is rather sad though; however, as that is the current state of affairs, I believe that it's a good practice to separate data types from functions.

As this PR tries to provide a rather self contained refactoring, without pulling in the changes that forced the cycle breaking, I wonder if we can try to review this from the point of view that this PR provides a better extension point to hadrian, that reduces the risk of cycles?

snowleopard · 2018-02-15T15:43:22Z

I wonder if we can try to review this from the point of view that this PR provides a better extension point to hadrian, that reduces the risk of cycles?

@angerman Right, at the moment I'm bothered by two specific decisions:

On the one hand, the current solution seems to be going too far with respect to the Stage module. It is highly unlikely that Stage will ever cause import cycles. I would consider it risk-free and leave it as is.
On the other hand, the current solution doesn't go far enough with respect to the Builder module, which is potentially much more likely to cause import cycles due to importing Oracles.*. Why was it left out? It actually has a lot more functions compared to Stage.

snowleopard · 2018-02-15T16:02:25Z

In general I am fine with splitting all modules that we identify as High Risk for Import Cycles. These seem to be: Context, Builder, Expression, Way (I think this will allow us to safely move Oracles.Settings.libsuf to Way), because they are interlinked with various oracles.

These modules on the other hand are Low Risk: Stage, Target, Flavour.

Does this sound reasonable?

alpmestan · 2018-02-16T17:18:49Z

OK, I'll tweak the PR accordingly next week :)

snowleopard · 2018-02-16T17:32:44Z

@alpmestan Thanks! I started to worry I scared you away with my avalanche of comments :-)

alpmestan · 2018-02-16T17:48:48Z

No no, absolutely not. It's just that I have to interleave the hadrian work with some other tasks :) My work on this will resume on monday.

alpmestan · 2018-02-19T17:11:27Z

On the other hand, the current solution doesn't go far enough with respect to the Builder module, which is potentially much more likely to cause import cycles due to importing Oracles.*. Why was it left out? It actually has a lot more functions compared to Stage.

It looks to me like one of the instances for Builder (the H.Builder one) is dependent on a lot of things, so if we were to split Builder into Builder/Builder.Type, we might end up having to make the H.Builder one orphan, otherwise we'd basically be pulling everything into the .Type module which would defeat the point. Unless I'm overlooking something?

@snowleopard I just pushed a commit addressing most of your feedback, let me know if that works for you or if you see other things that should be changed. :)

snowleopard · 2018-02-19T19:45:14Z

@alpmestan Thanks! One last question: You renamed Hadrian.Builder.Type to Hadrian.Builder.Mode, which is better, but I'm still not sure if this module is really necessary. I don't see how moving simple mode definitions like data ArMode = Pack | Unpack deriving (Eq, Generic, Show) to a separate module allows to break import cycles. Can't we define builder modes together with builder arguments?

If Hadrian.Builder.Mode is necessary for subsequent PRs I'm happy to merge this as is.

alpmestan · 2018-02-19T20:09:57Z

@snowleopard Got rid of Hadrian.Builder.Mode for now. I'll reintroduce it when it becomes strictly necessary. I'd rather get this PR merged and move on to the more juicy (but difficult) bits of #445 than spend more time on those few data types =)

snowleopard · 2018-02-19T20:23:49Z

@alpmestan Thanks =) Merged now! Looking forward to more juicy PRs :-)

move a bunch of types into dedicated modules

0bded85

snowleopard reviewed Feb 15, 2018

View reviewed changes

alpmestan mentioned this pull request Feb 15, 2018

Types.X modules become X.Type angerman/hadrian#4

Merged

address review feedback

32422ba

do away with Hadrian.Builder.Mode for now

73e674b

snowleopard merged commit cc8f62c into snowleopard:master Feb 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move a bunch of types into dedicated modules #502

move a bunch of types into dedicated modules #502

alpmestan commented Feb 15, 2018 •

edited

Loading

snowleopard Feb 15, 2018

alpmestan Feb 19, 2018

snowleopard Feb 15, 2018

snowleopard Feb 15, 2018

snowleopard Feb 15, 2018

alpmestan Feb 15, 2018 •

edited

Loading

snowleopard Feb 15, 2018

snowleopard Feb 15, 2018

alpmestan Feb 19, 2018

snowleopard Feb 15, 2018

alpmestan Feb 19, 2018

snowleopard Feb 15, 2018

alpmestan Feb 19, 2018

snowleopard Feb 15, 2018

snowleopard commented Feb 15, 2018

snowleopard commented Feb 15, 2018 •

edited

Loading

snowleopard Feb 15, 2018 •

edited

Loading

angerman Feb 15, 2018

snowleopard Feb 15, 2018

alpmestan Feb 19, 2018

angerman commented Feb 15, 2018

snowleopard commented Feb 15, 2018

snowleopard commented Feb 15, 2018 •

edited

Loading

alpmestan commented Feb 16, 2018

snowleopard commented Feb 16, 2018

alpmestan commented Feb 16, 2018

alpmestan commented Feb 19, 2018

snowleopard commented Feb 19, 2018 •

edited

Loading

alpmestan commented Feb 19, 2018 •

edited

Loading

snowleopard commented Feb 19, 2018

move a bunch of types into dedicated modules #502

move a bunch of types into dedicated modules #502

Conversation

alpmestan commented Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpmestan Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Feb 15, 2018

snowleopard commented Feb 15, 2018 • edited Loading

snowleopard Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angerman commented Feb 15, 2018

snowleopard commented Feb 15, 2018

snowleopard commented Feb 15, 2018 • edited Loading

alpmestan commented Feb 16, 2018

snowleopard commented Feb 16, 2018

alpmestan commented Feb 16, 2018

alpmestan commented Feb 19, 2018

snowleopard commented Feb 19, 2018 • edited Loading

alpmestan commented Feb 19, 2018 • edited Loading

snowleopard commented Feb 19, 2018

alpmestan commented Feb 15, 2018 •

edited

Loading

alpmestan Feb 15, 2018 •

edited

Loading

snowleopard commented Feb 15, 2018 •

edited

Loading

snowleopard Feb 15, 2018 •

edited

Loading

snowleopard commented Feb 15, 2018 •

edited

Loading

snowleopard commented Feb 19, 2018 •

edited

Loading

alpmestan commented Feb 19, 2018 •

edited

Loading