srid / ema

Change-aware static site generator for Haskell programmers
https://ema.srid.ca
GNU Affero General Public License v3.0
117 stars 9 forks source link

Simplify complex compile errors when using generic deriving #116

Closed srid closed 2 years ago

srid commented 2 years ago

Part of this is due to the use of generics-sop. And it happens when writing route types and deriving IsRoute generically (via GenericRoute). Accidental typos or mistyping can produce really gnarly compile errors.

For example, with this change:

--- a/src/Ema/Example/Ex03_Store.hs
+++ b/src/Ema/Example/Ex03_Store.hs
@@ -97,7 +97,7 @@ deriveGeneric ''ProductRoute
 deriveIsRoute
   ''ProductRoute
   [t|
-    '[ WithModel (Map Slug Product)
+    '[ WithModel ()
      , WithSubRoutes
         '[ FileRoute "index.html"
          , StringRoute Product Slug

we get this complex error from ghc:

image

Or with this change:

diff --git a/src/Ema/Example/Ex03_Store.hs b/src/Ema/Example/Ex03_Store.hs
index 236fa72..80a0e8a 100644
--- a/src/Ema/Example/Ex03_Store.hs
+++ b/src/Ema/Example/Ex03_Store.hs
@@ -100,7 +100,6 @@ deriveIsRoute
     '[ WithModel (Map Slug Product)
      , WithSubRoutes
         '[ FileRoute "index.html"
-         , StringRoute Product Slug
          ]
      ]
     |]

we see ghc throwing:

image

These can really scare newcomers off.

RiugaBachi commented 2 years ago

So the major scenarios that I could think of to cover were:

1) ADT + WithSubRoutes shape mismatch 2) Incompatible constructor field + corresponding WithSubRoutes list index 3) ADT + WithSubModels shape mismatch 4) Incompatible constructor field + corresponding WithSubModels index 5) WithModel, after factoring into account WithSubModels specification if it exists, is incompatible with the root model

Can you think of any other cases worth checking for?

Everything is pretty trivial except for case 5. Typechecking essentially fails on HasAny -> { HasPosition / HasField / HasType } -> { all of their unique Context constraints }. Even just taking the case of HasType, its Context internally fails on GLens (HasTotalTypePSym ty) _ _ _ _, so we're looking at duplicating a lot of logic that already exists for all 3 selector styles in order to cough up friendly errors in a comprehensive way.

Ideally I want to just catch and rewrap any type errors thrown by HasAny in GSubModels but we're looking at reimplementing HasAny until -XDependentHaskell is finally a thing. Alternatively we can ignore case 5 since the type error isn't terribly bad I guess? Probably still confusing to newcomers but a lot less obscure than the other 4 cases and their coerce errors:

    • Could not deduce (generic-lens-core-2.2.1.0:Data.Generics.Product.Internal.GLens.GLens
                          (generic-lens-core-2.2.1.0:Data.Generics.Product.Internal.Typed.HasTotalTypePSym
                             (Map Slug Category))
                          (GHC.Generics.U1 @Type)
                          (GHC.Generics.U1 @Type)
                          (Map Slug Category)
                          (Map Slug Category))
        arising from the 'deriving' clause of a data type declaration
      from the context: (TypeError ...)
        bound by the deriving clause for ‘HasSubModels CategoryRoute’
        at src/Ema/Example/Ex03_Store.hs:68:20-31
      Possible fix:
        use a standalone 'deriving instance' declaration,
          so you can specify the instance context yourself
    • When deriving the instance for (HasSubModels CategoryRoute)
srid commented 2 years ago

Alternatively we can ignore case 5 since the type error isn't terribly bad I guess?

Without all the context in mind - intuitively, this sounds good to me. We can always revisit this later. But we can document case 5 in docs/guide/route/generic/errors.md (title: "Dealing with complex errors in generic deriving") for users?

Can you think of any other cases worth checking for?

At the moment, no. If we come across any in the future, I think we can add it to errors.md as first step, and then see if it is possible to provide a friendly type error for it (thus removing its entry from errors.md).

srid commented 2 years ago

That PR is a great improvement. In the fpindia-site I just noticed that for the following:

image

I get:

image

Which is not particular super useful. If the error message also showed the route constructor name, it would be more helpful to the user.

RiugaBachi commented 2 years ago

Currently working on the testing plan document.

Which is not particular super useful. If the error message also showed the route constructor name, it would be more helpful to the user.

It's hard to generalize this, I think. Looking at the current verification logic, it would print HtmlRoute_Resources as the missing constructor case, even though the offending constructor is HtmlRoute_UpcomingEvents.

srid commented 2 years ago

@RiugaBachi Looks like I overlooked this. But am I correct in understanding that the downside of doing all of this is that when you derive IsRoute generically on the type itself (ie., not as standalone deriving) the error messages become even less useful?

Specifically, I see:

image
srid commented 2 years ago

The above kind of error happens for the following type:

data CategoryRoute
  = CategoryRoute_Index
  | CategoryRoute_Category Slug
  deriving stock (Show, Eq, Ord, Generic)
  deriving
    (IsRoute)
    via (GenericRoute CategoryRoute '[])

And if I add back the generic-sop deriving, ie.:

data CategoryRoute
  = CategoryRoute_Index
  | CategoryRoute_Category Slug
  deriving stock (Show, Eq, Ord, Generic)
  deriving anyclass (SOP.Generic, SOP.HasDatatypeInfo)
  deriving
    (IsRoute)
    via (GenericRoute CategoryRoute '[])

then I see:

image
RiugaBachi commented 2 years ago

That's one of the regressions I fixed IIRC. Let me get around to pushing my fixes for #122 and then I'll get that PR up once it gets merged.

Basically everything is fine unless the user can make the verification type families stuck like in this example, which is a matter of proper matching / totality.

srid commented 2 years ago

@RiugaBachi Is there anything left to do for this? I'm close to releasing 0.8.

RiugaBachi commented 2 years ago

Yea i'll get around to making that regression fix PR.

srid commented 2 years ago

Another case,

https://github.com/EmaApps/ema/issues/131#issuecomment-1217370610