Improve error handling in Generator - come up with monad transformer stack for Generator

[Martinsos]

Right now we just call error which is very crude, instead we should use a monad transformer stack that has ExceptT in it. Probably we will also want to combine it with ReaderT in order to pass wasp around. So this really comes down to coming up with nice monad transformer stack for Generator.

[shayneczyzewski]

@Martinsos Per my chat with Edsko, we could consider something like this:

newtype CompileStack a = WrapCompileStack
  { unwrapCompileStack :: ReaderT Wasp (ExceptT CompileError Identity) a
  }
  deriving
    ( Functor,
      Applicative,
      Monad,
      MonadReader Wasp,
      MonadError CompileError
    )

The one question was about how many downstream functions use what is in the reader. In this case, I think all Generators use Wasp, but only some use CompileOptions, so using Wasp here like this makes sense to me 👍🏻

[Martinsos]

Looks good! Once we add Analyzer there will be no more CompileOptions passed to Generator, it will be only AppSpec, so this makes sense.

One interesting thing might be parametrizing this type regarding what is stored in the Reader, so that we can send a subset of data to certain functions -> that way it is clearer what they are operating on. This would need some investigating, to ensure this is a reasonable practice, but I think I saw a similar approach somewhere.

Only the name I think is a bit unusual -> what I think would be more typical is to call it Generator:

newtype Generator a = Generator { execGenerator :: ReaderT Wasp (ExceptT GeneratorError Identity) a }

[shayneczyzewski]

Cool makes sense! Yeah, that name was my quick substitute for his placeholder AppStack, I guess to convey Monad stack? But I think Generator works too, esp if convention/how it is used with Generators in our code 👍🏻

Unsure on if/how we can parameterize on what is in the Reader, and what the implications are if so.

[Martinsos]

It is often that specific monad or monad stack is called by the part of the system it is supporting, so for example it could be named Parser, or Generator, or Analyzer, or Cli, or typical example for ReaderT: App, or smth like that.

So, for parametrizing, I was imagining smth like this:

newtype Generator env a = Generator
  { execGenerator :: ReaderT env (ExceptT GeneratorError Identity) a }

And then we would have Generator AppSpec as a common thing, but we might also somewhere go with smth more restricted like Generator [Route] or Generator Page or smth like that. That said, I would first ask around if this is a typical approach or not, just to be ensure it doesn't have some bad sides that we are not aware of. Cool thing about this would be that if you see a function like this:

generatePage :: Generator Page

you know it depends only on a single Page as an input, and doesn't use anything else from Wasp. That said, maybe that is too limiting? Ok, this is certainly very fuzzy for me also, I don't have a good feeling for pros/cons here.

[shayneczyzewski]

Ok, sounds good! I will ask around and tinker when I take a look at this. I will also do some preparatory research/prototyping to start feeling more comfortable with these stacked transformers. 😅

[shayneczyzewski]

Did some simple experimenting locally and made it runnable here just in case anyone wants to get a feel for it all: https://replit.com/@smcnc/ReaderT-ExceptT-playground#Main.hs

Note: You can just check out the Main.hs file if you want. It won't Run out of the box (replit quirk it seems), but if you fork and follow README directions you can get it running pretty easily.

[shayneczyzewski]

Think about how to handle warnings, so we may be able to do other things like show "Need to migrate" in browser when running wasp start and Prisma schema changed. This could be handled with different error severity levels.

[shayneczyzewski]

Chatted through this a bit more with Edsko today @Martinsos, and we had some interesting discussions (both about ReaderT/ExceptT Monad transformer stacks and how they can be refactored for even better maintainability/abstraction in other ways). He noted while it is objectively a valid use of ReaderT for passing around a Wasp, he still felt (possibly due to my mediocre explanations :D) that his personal preference is for ReaderT to be used for singular static things vs different values of some larger type (i.e. only needing some parts of larger entity/type like waspElements :: [WaspElement]). Also, if it is going to be used everywhere down the call stack anyways, and is important to understanding the function's purpose, it lends itself to being included explicitly in the function signature as additional documentation about what it needs to operate on (takes a Wasp and returns blah). ReaderT to him is more often used for flatter items/mockables/things we don't require all the time at each level. Much of the gains lost when every function will just have to do wasp <- ask as first line (vs explicit param). This will also have implications on testing (passing args vs constructing contexts). WARNING: I'm 99% sure I'm losing some things in translation. Certainly we can still use as such, and I can see many pros/cons of both sides of the coin (and have read dozens of conflicting opinions online too), but just wanted to raise as one data point for us to consider/chew on before committing to ReaderT with Wasp. Possibly worth a quick meeting sync on it sometime as a final 👍🏻 Thanks!

[Martinsos]

@shayneczyzewski that makes perfect sense to me! It was also the reason why I was considering that weird idea with polymorphic monad transformer stack where we can specify which value it carries, so we can sometimes make it carry some sub-vale of Wasp. But that might be the worst of both world then hm. Or maybe the best :D?

Ok, so I think the question is: is it better to pass AppSpec (let's use AppSpec from now on instead of Wasp) as we are now, in the signature of functions, or to have it be contained in the ReaderT.

Now, it is important to notice that generator in general is very tree-like -> we have a top function which calls other generator functions which call more generator functions and so on. And we can expect this tree to become both wider and deeper as time goes. Also, it is not super easy to predict which part of generator will need which data -> it might happen that suddenly one part needs something from the very different subsystem of AppSpec.

So on one hand, we would like to be able to easily access AppSpec at any place in Generator logic, since we are aware that might be needed. On the other hand, we do like keeping explicit the information about which generator sub-system depends on what -> if it doesn't depend on whole AppSpec but just small part of it, we would like to capture that in types. But if it turns out it needs more of AppSpec, we would like to have that easily addable.

Propagating AppSpec everywhere via function signatures is not super nice, which was clear in the amount of refactoring I had to do to replace Wasp with AppSpec. And due to tree-like nature of generator, there is a lot of propagating.

So, how do we make it easy to change what specific sub-systems of generator depends on, while still being explicit about what they depend on, I guess that is the problem, right?

And from what I get, Edsko suggests that it might be better just being explicit about it, in the function signature, as we are now, than making AppSpec available everywhere, since then we will lose this explicitness?

I wonder if we can have both. We could put AppSpec in our monad stack, and have it everywhere in that way, but then we could make sure to extract functions that operate only on the subpart of it, and they would not use ReaderT, instead would have explicit signatures, same like generator functions now. So it would become similar to how we deal with IO and pure functions -> there is an IO shell, and then we extract as much as we can into pure functions. So our monad transformer stack, that has AppSpec in ReaderT, let's say it is called Generator -> we could have that to play analogous role to IO, and then specific generator functions like generatePage :: Page -> [FileDraft] would be analogous to pure functions?

[Martinsos]

But yeah, I do agree that just passing around AppSpec via ReaderT and having no dependencies defined in function signatures would be bad, that would really make it hard to figure out what the function does and negate the purpose of types.

[shayneczyzewski]

Hey @Martinsos - thanks for the thoughts! Yeah, I'm not sure on the surface just how the polymorphic monad transformer stack would change things for the better or worse.

I think you captured the tension between propagating AppSpec via explicit function params vs. implicit ReaderT context well. I don't have a great answer myself right now, although explicit feels slightly better to me personally atm 😄 Interesting idea to kinda treat ReaderT as IO shell, will have to think more on that as well!

While possibly not directly applicable to this case, just something that came up in our chat worth mentioning. It blends the ReaderT context and explicit dependencies by using another layer of abstraction like so:

-- [1] Our current approach exposes our use of MonadReader to clients, but we may wish to abstract that for both ease of refactoring and testing. Existing:
newtype AppMonad a = WrapAppMonad { unwrapAppMonad :: ReaderT .. }
  deriving ( MonadReader ... )

-- [2] So we could expose a different way to get what our ReaderT encapsulates via a function instead. Here it still leverages our MonadReader under the covers, but the client is unaware/unable to:
getOptions :: AppMonad Options
getOptions = WrapAppMonad $ ask

-- The above gives us a step toward easier refactoring if we wanted something else than MonadReader, or multiple environments, etc. Same principles apply to ExceptT.

-- [3] Taking it a step further, we could even make use of Type Classes/Types to create (possibly more restricted and) explicit function type signature constraints:
class HasOptions m where
  getOptions :: m Options

instance HasOptions AppMonad where
  getOptions = WrapAppMonad $ ask

foo :: HasOptions m => ... -> m ... -- instead of foo :: ... -> AppMonad ...

-- The above gets us a step toward easier testing too (so we don't have to setup a full environment just to call each function which may only need a subset, and we can more easily mock now), plus more explicit dependencies about what in the environment it needs.

If we go all the way to [3] we could do things like HasXYZ for different parts of the AppSpec (if that made sense). Some tradeoffs include:

• Cons: every single function will list one or more (often many) constraints
• Pro: much more testable
• Pro: functions don't need the full context
• Pro: function signatures explain what the function needs and does not need
• Pro: forces you to be disciplined about separation of concerns (in a case of things like db handles, web service dependencies, etc)

I'll think more on this and see what I can come up with. Thanks for the brainstorming help! 👍🏻

[Martinsos]

@shayneczyzewski thanks for this, this is very cool!

Regarding additional level of interface, so we don't deal directly with ask but instead we have functions on top of it -> yes, I like that very much, was considering proposing that at some point actually. I love looking at a monad like this as a type and bunch of functions on it, and while ask is a function on it, it is a bit too close to the implementation. So I think this is a no-brainer and great approach.

As for Has... approach -> makes sense! I think you explained pros/cons in great way. The only thing I fear is that we introduce more complexity than we take away, which is not easy to judge at the moment. But it is certainly attractive, and the idea of specifying granular dependencies on type level is really attractive. This does though mean we have to define a type class for each of those. But that might be ok. Interesting interesting!

Hm it seems we are slowly coming up with a gradual plan to attack this problem. It is obviously not yet completely clear, but it is shaping well for sure! Thanks for the effort, looking forward to see what you suggest next!

[shayneczyzewski]

Ok, after giving it some thought @Martinsos, it sounds like we are in general agreement that some form of explicitness for AppSpec dependencies into the various generator functions is good (so not using a vanilla ReaderT), but whether that is an explicit function param, or possibly a Has... type constraint, or maybe the polymorphic option for the transformer stack, or the IO shell idea, are all up in the air at this point. It probably requires more thought and research to consider all pros/cons and what we are trying to solve for before we rip the bandaid off (if ever, even).

In the meantime, we still do want better error reporting now. My thinking is we focus this first round on just improving the error reporting, and circle back on how best to abstract the AppSpec dependency (leaving as explicit function param for now). I think this gives us the option to iterate in the future but still get a win now.

My plan is to modify all the top level gen function signatures from ... -> [FileDraft] to ... -> Either GeneratorError [FileDraft]. For the ones that they call, if they can error out I will update them to ... -> Either GeneratorError FileDraft, if not they can remain -> FileDraft.

Therefore, the main generation function would then look something like this:

writeWebAppCode :: AppSpec -> Path' Abs (Dir ProjectRootDir) -> IO (Either GeneratorError ())
writeWebAppCode spec dstDir = runExceptT $ do
  let app = [generateWebApp spec, genServer spec, genDb spec, genDockerFiles spec]
  fds <- except $ do concat <$> sequence app

  liftIO $ do
    ServerGenerator.preCleanup spec dstDir
    DbGenerator.preCleanup spec dstDir
    writeFileDrafts dstDir fds
    writeDotWaspInfo dstDir

and can report errors up to Lib.compile. Does this sound like a reasonable first step? It is possible as I update all these functions of the form gen* :: AppSpec -> FileDraft, it may help inform some of the unknowns to how to handle the AppSpec input dependency as well. Thanks!

[Martinsos]

You are right that we should probably give up on trying to pass AppSpec everywhere for now and just focus on error reporting!

I am thinking however that we should make it easier to refactor in the future -> I wouldn't want us to again have to refactor all of these type signatures.

For example, we might want to soon add the capability to report warnings -> in that case we don't want to crash the Generator, we want to record one or multiple warnings and still generate the code. For that Either would not be enough, and I can imagine us wanting to do this very soon.

See how I did it for Parser in this practice project I was doing: https://github.com/Martinsos/lox-haskell/blob/master/src/TokenParser.hs -> here I collect errors via State monad.

So, what I am thinking is that we could create a monad called Generator, and we go with it and modify it in the future as we want. We can start with it having just ExceptT inside, and for example if we want to collect more errors, we can modify it to use State and to collect warnings in there, while not having to refactor all the signatures. And then we can extend it with AppSpec maybe in the future and so on. What do you think about that, maybe that is a good direction to go in?

[shayneczyzewski]

Hey @Martinsos thanks for the example. I think planning ahead if we think we will need it, which it does sound like here, makes sense. I'm good with creating a Generator monad that is first just ExceptT! 👍🏻 Will start on it, thanks.

[shayneczyzewski]

Hey there, @Martinsos! I did some early work on the new Generator Monad today: https://github.com/wasp-lang/wasp/commit/2b6b43c53f54b76b94c9c4989f8c1e2872a79889 I'm not ready for a full PR review, as it is still a very rough day-old WIP, but I did have a few immediate questions that hopefully won't take more than a bit of your time.

The questions I ran into are:

• Do you think it makes sense to only convert those functions that have the form ... -> FileDraft that can error over to ... -> Generator FileDraft, or just do it for all of those called by the main top level gen functions for consistency down for the whole call hierarchy. An example can be found here: https://github.com/wasp-lang/wasp/commit/2b6b43c53f54b76b94c9c4989f8c1e2872a79889#diff-e4fd2bdd8554b06320cc11ecf47ae97d2bc31abeb10b6d9a23a608eca838e7c2R56-R70 In this case, I only converted the one that had error but should they all go over? The pro to all is the consistency in the calling function (genServer), as well as gaining any other future enhancements by the Monad we may not need now. The con is we are adding something we are not using right now and adding a new context (aka complexity, to me at least) to functions that do not benefit atm. This is one I do not have the experience in Haskell to see a recommended way.
• Should writeWebAppCode handle the display of warnings/non-fatal errors, or should it pass that along to Lib (and thus to ultimate callers like CLI, possibly as IO (Either GeneratorError GeneratorState)). Ref: https://github.com/wasp-lang/wasp/commit/2b6b43c53f54b76b94c9c4989f8c1e2872a79889#diff-dbc5f9920ee899b04949bdac19fcacd740bc22739eb7e7848df254e922a049dcR34-R46 Pro to showing here is we have it all in one place. However, the con is it feels like CLI is "more in charge" of displaying content to the user. It is probably easier to display in writeWebAppCode and only pass along any fatal errors that caused compile to fail, but wanted a sanity check.

Thanks!!

[Martinsos]

1. Generator or not -> I understand your reasoning and I don't have a very good answer. I would say, if we are not sure, go with Generator. Yes, it gives us some capabilities that we don't need, but that is not really an issue. On the other hand, if there is a function that you really feel can be more standalone for whatever reasons, and you don't see it as a generator function, then keep it "pure" (of Generator). As long as we use Generator for all top lvl stuff, it should never be far away and easy to pull in if we need it, so I don't think we can miss a lot. As for that specific example -> ha not sure, I might even say yes go with Generator. If you look at how the libraries like Parsec and similar are used, they also have Parser monad and normally you do stuff in it, not worrying if you are not using something from it, but of course if there is something that can be easily separated as a helper / utility / pure function, then you do that. So I would say: do as your gut feeling tells you, if not sure then maybe prefer using Generator a bit, and we can also review it together in PR and try together to make some judgement.

2. I would say:

   • Have writeWebAppCode return GeneratorError, similar like Analyzer returns AnalyzeError.
   • Have Lib turn those errors into Strings (as it works right now -> Lib turns AnalyzeError intro String)
   • Have CLI print those errors with possible additional formatting / styling.

   We can always make Lib returned more structured errors if we figure out we need more information in the CLI, but for now I would keep it simple if we don't need more info. You will probably need to discern between errors and warnings hm. So maybe you could make data CompilerError that can be Error String or Warning String for now, and we can make it more elaborate in the future? Or you could make Lib return two arrays, one of errors and one of warnings. Something in that direction sounds good to me! And as I said, we can always make CompilerError smarter if needed. In theory we could even make CLI deal directly with AnalyzeError and GeneratorError, but I like the idea of having this bit of de-coupling for now -> but I am not sure why hm, I guess because I want to hide the implementation a bit -> don't expose what is not needed if it is not needed.

[shayneczyzewski]

Great, thanks @Martinsos for your quick take! This is super helpful and will unblock me to hopefully to get closer towards a Draft PR today (and hopefully ready for review tomorrow). I really appreciate the thoughts! 👍🏻 🚀