Skip to content
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.

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

split scenario construction into separate sublibrary #1719

Merged
merged 6 commits into from
Jan 24, 2024

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Jan 6, 2024

Towards #1715 and #1043

Creates a new swarm-scenario sublibrary intended for scenario data that is independent of game state.

Planned follow-ups

This PR is already pretty large, but there is still more that can be done regarding sublibrary reorganization/splitting.

  • May want to pare-down a sublibrary exclusively for world-generation without all the other baggage of scenarios.
  • Swarm.Game.ScenarioInfo, Swarm.Game.Tick, and Swarm.Game.Scenario.Status could probably be moved from swarm-scenario to swarm-engine.

@kostmo kostmo requested a review from byorgey January 6, 2024 07:00
@kostmo kostmo marked this pull request as ready for review January 6, 2024 07:00
@kostmo kostmo changed the title split world construction into separate sublibrary split scenario construction into separate sublibrary Jan 6, 2024
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other sublibraries so far made sense to me, but I confess being confused about this one.

  • It is named swarm-world, but the title of this PR says "scenario construction"
  • There is some stuff in here that seems like it should belong in swarm-engine, the biggest ones being Swarm.Game.CESK and Swarm.Game.Robot. But also Swarm.Game.Entity perhaps, and Swarm.Game.Exception...

swarm.cabal Outdated Show resolved Hide resolved
@kostmo kostmo force-pushed the refactor/world-sublibrary branch from 0bdeba6 to 699e42e Compare January 7, 2024 01:02
@kostmo
Copy link
Member Author

kostmo commented Jan 7, 2024

It is named swarm-world, but the title of this PR says "scenario construction"

I've now renamed it to swarm-scenario. At some future point, I may split this further to extract a narrower swarm-world sublibrary.

There is some stuff in here that seems like it should belong in swarm-engine, the biggest ones being Swarm.Game.CESK and Swarm.Game.Robot. But also Swarm.Game.Entity perhaps, and Swarm.Game.Exception...

The issue is that Swarm.Game.Scenario imports TRobot from Swarm.Game.Robot, which depends on Swarm.Game.CESK and by extension Swarm.Game.Exception. But I think I may be able to solve this by adding more type family members to RobotR in Swarm.Game.Robot that depend on RobotPhase, which should let me split out TRobot to a module that does not reference CESK.

@kostmo
Copy link
Member Author

kostmo commented Jan 7, 2024

adding more type family members to RobotR

On that note, would it be possible to meet our needs by using just simple type parameters in the RobotR type to differentiate Robot and TRobot? @byorgey could you summarize the advantage of using type families here?

@kostmo kostmo force-pushed the refactor/world-sublibrary branch 2 times, most recently from 445ad9c to 5bd4345 Compare January 7, 2024 03:08
@byorgey
Copy link
Member

byorgey commented Jan 8, 2024

The issue is that Swarm.Game.Scenario imports TRobot from Swarm.Game.Robot, which depends on Swarm.Game.CESK and by extension Swarm.Game.Exception.

Right, but if we let modules get slurped into libraries they don't logically belong in just because of dependency issues, that kind of defeats the whole purpose of splitting up the codebase into sublibraries. (To be clear, I see the purposes of splitting into sublibraries as (1) enforcing abstraction boundaries, so we can't just import any module into any other module, and (2) making it easier to navigate the codebase.)

On that note, would it be possible to meet our needs by using just simple type parameters in the RobotR type to differentiate Robot and TRobot? @byorgey could you summarize the advantage of using type families here?

First of all, just to make sure I understand what you mean by "using just simple type parameters", do you have in mind something like this?

data RobotR loc rid = RobotR
  { ...
  , _robotLocation :: loc
  ...
  , _robotID :: rid
  ...
  }

This would certainly work. But the benefits of the current code with type families as opposed to the above are as follows:

  1. Using type families means only precisely the things we want to allow are allowed. The only things that should be possible are (a) a RobotR record with no location and no ID, or (b) a record with both. With the type parameters version as above, it becomes possible to write down nonsensical/undesirable types such as RobotR Bool Int or RobotR Location () etc.
  2. It more precisely captures our intentions and produces better error messages. For example with the type family version, if you try to use Robot somewhere TRobot is expected, you will get an error message saying that TemplateRobot and ConcreteRobot don't match, which is telling you exactly that you tried to use a robot template where a concrete robot was expected or vice versa. With the type parameters version, you will just get an error saying that e.g. RID does not match ().
  3. It is much more easily extensible. In the type family version, we can easily add either more phases, or more fields that vary by phase. (In fact I think I may need to do both those things in order to get import working.)

@byorgey
Copy link
Member

byorgey commented Jan 8, 2024

adding more type family members to RobotR in Swarm.Game.Robot that depend on RobotPhase, which should let me split out TRobot to a module that does not reference CESK.

This actually makes a lot of sense to me --- a template robot should not really have a CESK machine.

@kostmo
Copy link
Member Author

kostmo commented Jan 8, 2024

This would certainly work. But the benefits of the current code with type families as opposed to the above are as follows:

  1. Using type families means only precisely the things we want to allow are allowed. The only things that should be possible are (a) a RobotR record with no location and no ID, or (b) a record with both. With the type parameters version as above, it becomes possible to write down nonsensical/undesirable types such as RobotR Bool Int or RobotR Location () etc.
  2. It more precisely captures our intentions and produces better error messages. For example with the type family version, if you try to use Robot somewhere TRobot is expected, you will get an error message saying that TemplateRobot and ConcreteRobot don't match, which is telling you exactly that you tried to use a robot template where a concrete robot was expected or vice versa. With the type parameters version, you will just get an error saying that e.g. RID does not match ().
  3. It is much more easily extensible. In the type family version, we can easily add either more phases, or more fields that vary by phase. (In fact I think I may need to do both those things in order to get import working.)

For the sake of argument, what if we bundled all of the differentiated fields into a single field with parameterized type, such that we have a toplevel parameterized type RobotR a along with two different records that we could plug in for a:

data ConcreteRobotParams = ConcreteRobotParams {
    robotLocation :: Cosmic Location
  , robotID :: RID
}

and

-- Does not provide an ID field, since it would just be `()` anyway
data TemplateRobotParams = TemplateRobotParams {
    templateLocation :: Maybe (Cosmic Location)
  , someOtherField :: Foo  -- Extensible; just add more fields
}

@byorgey
Copy link
Member

byorgey commented Jan 9, 2024

what if we bundled all of the differentiated fields into a single field with parameterized type

That would be annoying since we would have to either edit every place we use one of those fields to add an extra lens to get down through the nested record, or else create some custom lenses for referring to the nested fields. Well, I guess some custom lenses wouldn't be that bad. But I still don't see what the benefit would be. What's wrong with the type families?

@kostmo
Copy link
Member Author

kostmo commented Jan 13, 2024

What's wrong with the type families?

Practically

The first issue is practical. I'm not actually sure how we could make use of type families to split what currently constitutes the Swarm.Game.Robot module across two sublibraries: swarm-scenario, which would define TRobot; and swarm-engine, which would define Robot.

Currently the RobotR record has this field:

  , _machine :: CESK

We want to purge the swarm-scenario sublibrary of all references to CESK, so let's say we make it into a type family:

  , _machine :: RobotMachine phase

where

type family RobotMachine (phase :: RobotPhase) :: Data.Kind.Type where
  RobotID 'TemplateRobot = ()
  RobotID 'ConcreteRobot = CESK

But we're stuck in the same predicament; we still have a reference to CESK in the same sublibrary as the TRobot definition.

Are type families "open" for extension, such that we can split it into two non-exhaustive definition sites?
As an experiment I tried this:

type family RobotID (phase :: RobotPhase) :: Data.Kind.Type where
  RobotID 'TemplateRobot = ()

type family RobotID (phase :: RobotPhase) :: Data.Kind.Type where
  RobotID 'ConcreteRobot = RID

which yielded an error:

Multiple declarations of ‘RobotID’

Philosophically

The second issue is philosophical and possibly moot.

I sympathize with ambition of "Boring Haskell". We would like to maximize accessibility of the codebase to contributors, which IMO means limiting use of less-common language extensions.

I recognize type families are used in a couple other modules the project, but we can still try to minimize the surface area for which knowledge of these advanced language features is required. A given language extension may offer some advantage over plain Haskell2010, but IMO the advantage has to be more than "slight" to justify it's introduction. If some goal can be achieved by "boring" Haskell with just "minor" inconvenience, we should do it that way to shield potential contributors from intimidating extensions.

@byorgey
Copy link
Member

byorgey commented Jan 14, 2024

Are type families "open" for extension, such that we can split it into two non-exhaustive definition sites?

Yes, they are, if you avoid the where syntax (which explicitly introduces a closed type family). In fact, originally there were only open type families, and the where syntax was introduced because sometimes it's useful to be able to declare them closed.

type family RobotID (phase :: RobotPhase) :: Data.Kind.Type
type instance RobotID 'TemplateRobot = ()

... (in a different module) ...

type instance RobotID 'ConcreteRobot = CESK

@byorgey
Copy link
Member

byorgey commented Jan 14, 2024

By the way, as far as "Boring Haskell" goes, I am sympathetic to that idea in general, but my primary goal with Swarm has always been to have fun writing fun code to make a fun game; maxmizing accessibility of the codebase is only a secondary goal.

mergify bot pushed a commit that referenced this pull request Jan 14, 2024
Towards #1715 and #1043.

This refactoring step is a prerequisite for #1719 to extricate references to the `CESK` module from the base `RobotR` definition.

In this PR:
* `Swarm.Game.CESK` is imported qualified to more easily track usages
* A new `RobotMachine` type family is added to parameterize the `_machine` field.
* `CESK` is a new parameter to `addTRobot`
@kostmo kostmo force-pushed the refactor/world-sublibrary branch from 5bd4345 to 66c6047 Compare January 15, 2024 04:27
@kostmo kostmo marked this pull request as draft January 15, 2024 04:27
@kostmo kostmo force-pushed the refactor/world-sublibrary branch from 66c6047 to ae59077 Compare January 15, 2024 05:41
mergify bot pushed a commit that referenced this pull request Jan 20, 2024
Continuation of #1731.

Towards #1715 and #1043.

This refactoring step is a prerequisite for #1719 to extricate references to the `CESK` module from the base `RobotR` definition.

# In this PR:

* Extracts the `RobotContext` type to a new module
* Introduces a type family for the `RobotContext` field
@kostmo kostmo force-pushed the refactor/world-sublibrary branch 3 times, most recently from daccd61 to b9fc20e Compare January 21, 2024 03:12
@kostmo kostmo force-pushed the refactor/world-sublibrary branch from 1f3d8a3 to 44a2bbd Compare January 21, 2024 05:27
@kostmo kostmo marked this pull request as ready for review January 21, 2024 05:40
@kostmo kostmo force-pushed the refactor/world-sublibrary branch from 44a2bbd to c9b658b Compare January 24, 2024 17:41
@kostmo kostmo requested a review from byorgey January 24, 2024 18:22
src/swarm-scenario/Swarm/Game/Tick.hs Show resolved Hide resolved
swarm.cabal Show resolved Hide resolved
swarm.cabal Show resolved Hide resolved
@@ -12,6 +12,13 @@
-- A data type to represent robots.
module Swarm.Game.Robot (
-- * Robots data
RobotContextMember,
_robotID,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than exporting these explicit record fields, can we just use the lenses instead (which we are already exporting)? e.g. instead of _robotID r we can write r ^. robotID.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _robotID and _robotLocation field exports are pretty much only needed for the instantiateRobot function in Swarm.Game.Robot.Concrete. But it's unclear how lenses could substitute in that case.

Furthermore, yes, the robotID lens for the _robotID field is defined in swarm-scenario, but the robotContext and machine lenses are necessarily defined in swarm-engine because the Store and CESK types live there.

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Jan 24, 2024
@mergify mergify bot merged commit 05613df into main Jan 24, 2024
9 checks passed
@mergify mergify bot deleted the refactor/world-sublibrary branch January 24, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants