Raise proper location errors from the PPX, and improve what the error…

…s actually say a lot.
zth · Sep 29, 2019 · a015726 · a015726
1 parent 02e53e7
commit a015726
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 72 deletions.
diff --git a/packages/reason-relay/reason-relay-ppx/esy.lock/index.json b/packages/reason-relay/reason-relay-ppx/esy.lock/index.json
@@ -1,5 +1,5 @@
 {
-  "checksum": "028f20c3b3cff1b6d727786c596bd0f8",
+  "checksum": "1cc329b62ee5ac84eb9b82a2d1fb2122",
   "root": "reason-relay-ppx@link-dev:./package.json",
   "node": {
     "refmterr@3.2.2@d41d8cd9": {
@@ -35,7 +35,7 @@
       "dependencies": [
         "refmterr@3.2.2@d41d8cd9", "ocaml@4.6.1000@d41d8cd9",
         "@reason-native/rely@3.1.0@d41d8cd9",
-        "@opam/ppxlib@opam:0.6.0@844d9e68",
+        "@opam/ppxlib@opam:0.9.0@bfabe269",
         "@opam/ppx_tools_versioned@opam:5.2.3@4994ec80",
         "@opam/ocaml-migrate-parsetree@opam:1.4.0@0c4ec62d",
         "@opam/graphql_parser@opam:0.12.2@881db779",
@@ -446,20 +446,20 @@
         "ocaml@4.6.1000@d41d8cd9", "@opam/result@opam:1.4@dc720aef"
       ]
     },
-    "@opam/ppxlib@opam:0.6.0@844d9e68": {
-      "id": "@opam/ppxlib@opam:0.6.0@844d9e68",
+    "@opam/ppxlib@opam:0.9.0@bfabe269": {
+      "id": "@opam/ppxlib@opam:0.9.0@bfabe269",
       "name": "@opam/ppxlib",
-      "version": "opam:0.6.0",
+      "version": "opam:0.9.0",
       "source": {
         "type": "install",
         "source": [
-          "archive:https://opam.ocaml.org/cache/md5/e2/e2d129139891c135acc6d52a3fa9f731#md5:e2d129139891c135acc6d52a3fa9f731",
-          "archive:https://github.com/ocaml-ppx/ppxlib/releases/download/0.6.0/ppxlib-0.6.0.tbz#md5:e2d129139891c135acc6d52a3fa9f731"
+          "archive:https://opam.ocaml.org/cache/sha256/d2/d269f882a31ff75095a80793082f7481884ca75ef867c8c409f26ad36f9a0a54#sha256:d269f882a31ff75095a80793082f7481884ca75ef867c8c409f26ad36f9a0a54",
+          "archive:https://github.com/ocaml-ppx/ppxlib/archive/0.9.0.tar.gz#sha256:d269f882a31ff75095a80793082f7481884ca75ef867c8c409f26ad36f9a0a54"
         ],
         "opam": {
           "name": "ppxlib",
-          "version": "0.6.0",
-          "path": "esy.lock/opam/ppxlib.0.6.0"
+          "version": "0.9.0",
+          "path": "esy.lock/opam/ppxlib.0.9.0"
         }
       },
       "overrides": [],

diff --git a/...relay-ppx/esy.lock/opam/ppxlib.0.6.0/opam → ...relay-ppx/esy.lock/opam/ppxlib.0.9.0/opam b/...relay-ppx/esy.lock/opam/ppxlib.0.6.0/opam → ...relay-ppx/esy.lock/opam/ppxlib.0.9.0/opam
@@ -11,16 +11,16 @@ build: [
   ["dune" "build" "-p" name "-j" jobs]
 ]
 run-test: [
-  ["dune" "runtest" "-p" name "-j" jobs] { ocaml >= "4.06" }
+  ["dune" "runtest" "-p" name "-j" jobs] { ocaml:version >= "4.06" & ocaml:version < "4.09" }
 ]
 depends: [
-  "ocaml"                   {>= "4.04.1" & < "4.08.0"}
-  "base"                    {>= "v0.11.0" & < "v0.13"}
+  "ocaml"                   {>= "4.04.1"}
+  "base"                    {>= "v0.11.0"}
   "dune"
   "ocaml-compiler-libs"     {>= "v0.11.0"}
-  "ocaml-migrate-parsetree" {>= "1.0.9"}
+  "ocaml-migrate-parsetree" {>= "1.3.1"}
   "ppx_derivers"            {>= "1.0"}
-  "stdio"                   {>= "v0.11.0" & < "v0.13"}
+  "stdio"                   {>= "v0.11.0"}
   "ocamlfind"               {with-test}
 ]
 synopsis: "Base library and tools for ppx rewriters"
@@ -37,6 +37,9 @@ A comprehensive toolbox for ppx development. It features:
 """
 url {
   src:
-    "https://github.com/ocaml-ppx/ppxlib/releases/download/0.6.0/ppxlib-0.6.0.tbz"
-  checksum: "md5=e2d129139891c135acc6d52a3fa9f731"
+    "https://github.com/ocaml-ppx/ppxlib/archive/0.9.0.tar.gz"
+  checksum: [
+    "sha256=d269f882a31ff75095a80793082f7481884ca75ef867c8c409f26ad36f9a0a54"
+    "sha512=b903b0386739e17107f89fb9d7c861e95232c0deebbceeacad2d54f1ef771eb942a63e6057d2e3828f4c299c896c6488b6fd351bc150fd4e49d5a48bef84d8a8"
+  ]
 }
diff --git a/packages/reason-relay/reason-relay-ppx/library/ReasonRelayPpxLibrary.re b/packages/reason-relay/reason-relay-ppx/library/ReasonRelayPpxLibrary.re
@@ -12,11 +12,14 @@ let queryExtension =
     "relay.query",
     Extension.Context.module_expr,
     Ast_pattern.__,
-    (~loc, ~path as _, expr) =>
-    makeQuery(
-      ~moduleName=extractOperationStr(~loc, ~expr) |> extractTheQueryName,
-      ~loc,
-    )
+    (~loc, ~path as _, expr) => {
+      let (operationStr, operationStrLoc) = extractOperationStr(~loc, ~expr);
+
+      makeQuery(
+        ~moduleName=operationStr |> extractTheQueryName(~loc=operationStrLoc),
+        ~loc=operationStrLoc,
+      );
+    },
   );
 
 // Same as queryExtension but for [%relay.fragment]
@@ -26,22 +29,25 @@ let fragmentExtension =
     Extension.Context.module_expr,
     Ast_pattern.__,
     (~loc, ~path as _, expr) => {
-      let operationStr = extractOperationStr(~loc, ~expr);
+      let (operationStr, operationStrLoc) = extractOperationStr(~loc, ~expr);
       let refetchableQueryName =
-        operationStr |> extractFragmentRefetchableQueryName;
+        operationStr
+        |> extractFragmentRefetchableQueryName(~loc=operationStrLoc);
 
       makeFragment(
-        ~moduleName=operationStr |> extractTheFragmentName,
+        ~moduleName=
+          operationStr |> extractTheFragmentName(~loc=operationStrLoc),
         ~refetchableQueryName,
         ~hasConnection=
           switch (
             refetchableQueryName,
-            operationStr |> fragmentHasConnectionNotation,
+            operationStr
+            |> fragmentHasConnectionNotation(~loc=operationStrLoc),
           ) {
           | (Some(_), true) => true
           | _ => false
           },
-        ~loc,
+        ~loc=operationStrLoc,
       );
     },
   );
@@ -52,11 +58,14 @@ let mutationExtension =
     "relay.mutation",
     Extension.Context.module_expr,
     Ast_pattern.__,
-    (~loc, ~path as _, expr) =>
-    makeMutation(
-      ~moduleName=extractOperationStr(~loc, ~expr) |> extractTheMutationName,
-      ~loc,
-    )
+    (~loc, ~path as _, expr) => {
+      let (operationStr, operationStrLoc) = extractOperationStr(~loc, ~expr);
+      makeMutation(
+        ~moduleName=
+          operationStr |> extractTheMutationName(~loc=operationStrLoc),
+        ~loc=operationStrLoc,
+      );
+    },
   );
 
 // Same as queryExtension but for [%relay.subscription]
@@ -65,12 +74,14 @@ let subscriptionExtension =
     "relay.subscription",
     Extension.Context.module_expr,
     Ast_pattern.__,
-    (~loc, ~path as _, expr) =>
-    makeSubscription(
-      ~moduleName=
-        extractOperationStr(~loc, ~expr) |> extractTheSubscriptionName,
-      ~loc,
-    )
+    (~loc, ~path as _, expr) => {
+      let (operationStr, operationStrLoc) = extractOperationStr(~loc, ~expr);
+      makeSubscription(
+        ~moduleName=
+          operationStr |> extractTheSubscriptionName(~loc=operationStrLoc),
+        ~loc=operationStrLoc,
+      );
+    },
   );
 
 // This registers all defined extension points to the "reason-relay" ppx.

diff --git a/packages/reason-relay/reason-relay-ppx/library/Util.re b/packages/reason-relay/reason-relay-ppx/library/Util.re
@@ -4,62 +4,90 @@
 // Ppxlib provides a number of helpers for writing and registering PPXs that life is very difficult without.
 open Ppxlib;
 
-exception Could_not_extract_operation_name;
-exception Could_not_extract_operation;
-
 /**
  * This function takes a GraphQL document as a string (typically extracted from the [%relay.<operation>] nodes),
  * uses Graphql_parser to parse the string into a list of GraphQL definitions, and then extracts the _first_ operation
  * of the document only. This is because Relay disallows multiple operations in the same definition.
  */
-let extractGraphQLOperation = str =>
+let extractGraphQLOperation = (~loc, str) =>
   switch (str |> Graphql_parser.parse) {
   | Ok(definitions) =>
     switch (definitions) {
-    | [op, ..._] => op
-    | _ => raise(Could_not_extract_operation)
+    | [op] => op
+    | _ =>
+      Location.raise_errorf(
+        ~loc,
+        "Only one GraphQL operation per [%%relay]-node is allowed.",
+      )
     }
-  | Error(_) => raise(Could_not_extract_operation)
+  | Error(_) =>
+    Location.raise_errorf(
+      ~loc,
+      "[%%relay]-nodes must define a single, valid GraphQL operation.",
+    )
   };
 
 /**
  * Takes a raw GraphQL document as a string and extracts the query name. Raises an error if it's not a query
  * or the query has no name.
  */
-let extractTheQueryName = str =>
-  switch (str |> extractGraphQLOperation) {
+let extractTheQueryName = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Operation({optype: Query, name: Some(name)}) => name
-  | _ => raise(Could_not_extract_operation_name)
+  | Operation({optype: Query, name: None}) =>
+    Location.raise_errorf(~loc, "GraphQL query must be named.")
+  | _ =>
+    Location.raise_errorf(
+      ~loc,
+      "[%%relay.query] must contain a query definition, and nothing else.",
+    )
   };
 
 /**
  * Takes a raw GraphQL document as a string and extracts the mutation name. Raises an error if it's not a mutation
  * or the mutation has no name.
  */
-let extractTheMutationName = str =>
-  switch (str |> extractGraphQLOperation) {
+let extractTheMutationName = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Operation({optype: Mutation, name: Some(name)}) => name
-  | _ => raise(Could_not_extract_operation_name)
+  | Operation({optype: Mutation, name: None}) =>
+    Location.raise_errorf(~loc, "GraphQL mutation must be named.")
+  | _ =>
+    Location.raise_errorf(
+      ~loc,
+      "[%%relay.mutation] must contain a mutation definition, and nothing else.",
+    )
   };
 
 /**
  * Takes a raw GraphQL document as a string and extracts the fragment name. Raises an error if it's not a fragment
  * or the fragment has no name.
  */
-let extractTheFragmentName = str =>
-  switch (str |> extractGraphQLOperation) {
+let extractTheFragmentName = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Fragment({name}) => name
-  | _ => raise(Could_not_extract_operation_name)
+  | _ =>
+    Location.raise_errorf(
+      ~loc,
+      "[%%relay.fragment] must contain a fragment definition with a name, and nothing else.",
+    )
   };
 
 /**
  * Takes a raw GraphQL document as a string and extracts the subscription name. Raises an error if it's not a subscription
  * or the subscription has no name.
  */
-let extractTheSubscriptionName = str =>
-  switch (str |> extractGraphQLOperation) {
+let extractTheSubscriptionName = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Operation({optype: Subscription, name: Some(name)}) => name
-  | _ => raise(Could_not_extract_operation_name)
+  | Operation({optype: Subscription, name: None}) =>
+    Location.raise_errorf(~loc, "GraphQL subscription must be named.")
+
+  | _ =>
+    Location.raise_errorf(
+      ~loc,
+      "[%%relay.subscription] must contain a subscription definition, and nothing else.",
+    )
   };
 
 /**
@@ -73,8 +101,8 @@ let extractTheSubscriptionName = str =>
  * So, this functions makes sure that @refetchable is defined and the queryName arg exists, and if so, extracts and
  * returns "SomeFragmentRefetchQuery" as an option string.
  */
-let extractFragmentRefetchableQueryName = str =>
-  switch (str |> extractGraphQLOperation) {
+let extractFragmentRefetchableQueryName = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Fragment({name: _, directives}) =>
     let refetchableQueryName = ref(None);
 
@@ -85,7 +113,7 @@ let extractFragmentRefetchableQueryName = str =>
              name: "refetchable",
              arguments: [("queryName", `String(queryName))],
            } =>
-           refetchableQueryName := Some(queryName);
+           refetchableQueryName := Some(queryName)
          | _ => ()
          }
        );
@@ -124,8 +152,8 @@ let rec selectionSetHasConnection = selections =>
   };
 
 // Returns whether a fragment has a @connection annotation or not
-let fragmentHasConnectionNotation = str =>
-  switch (str |> extractGraphQLOperation) {
+let fragmentHasConnectionNotation = (~loc, str) =>
+  switch (extractGraphQLOperation(~loc, str)) {
   | Fragment({name: _, selection_set}) =>
     selectionSetHasConnection(selection_set)
   | _ => false
@@ -137,6 +165,9 @@ let getGraphQLModuleName = opName => opName ++ "_graphql";
  * This is some AST voodoo to extract the provided string from [%relay.<operation> {| ...string here... |}].
  * It basically just matches on the correct AST structure for having an extension node with a string, and
  * returns that string.
+ *
+ * It also returns loc, which keeps track of *where* in the code the string is located, so editors can highlight
+ * the actual operation string as a whole when it errors, rather than just the module keyword.
  */
 let extractOperationStr = (~loc, ~expr) =>
   switch (expr) {
@@ -157,16 +188,14 @@ let extractOperationStr = (~loc, ~expr) =>
           ),
         _,
       },
-    ]) => operationStr
+    ]) => (
+      operationStr,
+      loc,
+    )
   | _ =>
-    raise(
-      Location.Error(
-        Obj.magic(),
-        /*Location.Error.createf(
-            ~loc,
-            "All [%relay] operations must be provided a string, like [%relay.query {| { query SomeQuery { id } |}]",
-          ),*/
-      ),
+    Location.raise_errorf(
+      ~loc,
+      "All [%%relay] operations must be provided a string, like [%%relay.query {| { query SomeQuery { id } |}]",
     )
   };
 

diff --git a/packages/reason-relay/reason-relay-ppx/package.json b/packages/reason-relay/reason-relay-ppx/package.json
@@ -62,7 +62,7 @@
     "@opam/graphql_parser": "0.12.2",
     "@opam/ocaml-migrate-parsetree": "1.4.0",
     "@opam/ppx_tools_versioned": "5.2.3",
-    "@opam/ppxlib": "0.6.0",
+    "@opam/ppxlib": "*",
     "@reason-native/rely": "*",
     "ocaml": "~4.6.1",
     "refmterr": "*"