Merge pull request prettier#817 from prettier/better-errors

Fix for prettier#814

Author: kddnewton

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
 
 - [#812](https://github.com/prettier/plugin-ruby/issues/812) - valscion, kddeisz - Splats and blocks within args that have comments attached should place their respective operators in the right place.
 - [#816](https://github.com/prettier/plugin-ruby/pull/816) - valscion - Document RuboCop's Style/Lambda should be disabled
+- [#814](https://github.com/prettier/plugin-ruby/issues/814) - jscheid, kddeisz - Provide better errors when the source location of the error is known.
 
 ## [1.5.2] - 2021-02-03
 

src/haml/parser.rb

@@ -131,8 +131,6 @@ module Prettier
   class HAMLParser
     def self.parse(source)
       Haml::Parser.new({}).call(source).as_json
-    rescue StandardError
-      false
     end
   end
 end

src/parser/parseSync.js

@@ -5,26 +5,21 @@ const requestParse = require("./requestParse");
 // synchronous and Node.js does not offer a mechanism for synchronous socket
 // requests.
 function parseSync(parser, source) {
-  const response = requestParse(parser, source);
+  const { stdout, stderr, status } = requestParse(parser, source);
 
-  if (
-    response.stdout.length === 0 ||
-    (response.status !== null && response.status !== 0)
-  ) {
-    console.error("Could not parse response from server");
-    console.error(response);
-
-    throw new Error(response.stderr || "An unknown error occurred");
+  if (stdout.length === 0 || (status !== null && status !== 0)) {
+    throw new Error(stderr || "An unknown error occurred");
   }
 
-  const parsed = JSON.parse(response.stdout);
+  const parsed = JSON.parse(stdout);
 
   if (parsed.error) {
-    throw new Error(
-      `@prettier/plugin-ruby encountered an error when attempting to parse \
-the ruby source. This usually means there was a syntax error in the \
-file in question. You can verify by running \`ruby -i [path/to/file]\`.`
-    );
+    const error = new Error(parsed.error);
+    if (parsed.loc) {
+      error.loc = parsed.loc;
+    }
+
+    throw error;
   }
 
   return parsed;

src/parser/server.rb

@@ -48,6 +48,11 @@
     else
       socket.write('{ "error": true }')
     end
+  rescue Prettier::Parser::ParserError => error
+    loc = { start: { line: error.lineno, column: error.column } }
+    socket.write(JSON.fast_generate(error: error.message, loc: loc))
+  rescue StandardError => error
+    socket.write(JSON.fast_generate(error: error.message))
   ensure
     socket.close
   end

src/rbs/parser.rb

@@ -87,8 +87,6 @@ module Prettier
   class RBSParser
     def self.parse(text)
       { declarations: RBS::Parser.parse_signature(text) }
-    rescue StandardError
-      false
     end
   end
 end

src/ruby/parser.rb

@@ -1729,6 +1729,28 @@ def on_paren(contents)
     )
   end
 
+  # A special parser error so that we can get nice syntax displays on the error
+  # message when prettier prints out the results.
+  class ParserError < StandardError
+    attr_reader :lineno, :column
+
+    def initialize(error, lineno, column)
+      super(error)
+      @lineno = lineno
+      @column = column
+    end
+  end
+
+  # If we encounter a parse error, just immediately bail out so that our runner
+  # can catch it.
+  def on_parse_error(error, *)
+    raise ParserError.new(error, lineno, column)
+  end
+  alias on_alias_error on_parse_error
+  alias on_assign_error on_parse_error
+  alias on_class_name_error on_parse_error
+  alias on_param_error on_parse_error
+
   # The program node is the very top of the AST. Here we'll attach all of
   # the comments that we've gathered up over the course of parsing the
   # source string. We'll also attach on the __END__ content if there was

test/js/files.test.js

@@ -1,10 +1,20 @@
-describe("files", () => {
-  test("handles full files that match", () =>
-    expect("files/Gemfile").toInferParser());
+const path = require("path");
+const { getFileInfo } = require("prettier");
+
+const plugin = require("../../src/plugin");
+
+function getInferredParser(filename) {
+  const filepath = path.join(__dirname, filename);
 
-  test("handles shebangs that match", () =>
-    expect("files/shebang").toInferParser());
+  return getFileInfo(filepath, { plugins: [plugin] }).then(
+    ({ inferredParser }) => inferredParser
+  );
+}
+
+describe("files", () => {
+  const cases = ["files/Gemfile", "files/shebang", "files/test.rake"];
 
-  test("handles extensions that match", () =>
-    expect("files/test.rake").toInferParser());
+  test.each(cases)("infers Ruby parser from %s", (filename) =>
+    expect(getInferredParser(filename)).resolves.toBe("ruby")
+  );
 });

test/js/ruby/errors.test.js

@@ -1,7 +1,25 @@
+const prettier = require("prettier");
 const { print } = require("../../../src/ruby/printer");
 
 describe("errors", () => {
-  test("invalid ruby", () => expect("<>").toFailFormat());
+  const cases = [
+    "alias $a $1",
+    "self = 1",
+    "$` = 1",
+    "class foo; end",
+    "def foo(A) end",
+    "def foo($a) end",
+    "def foo(@a) end",
+    "def foo(@@a) end",
+    "<>"
+  ];
+
+  test.each(cases)("fails for %s", (content) => {
+    const format = () =>
+      prettier.format(content, { parser: "ruby", plugins: ["."] });
+
+    expect(format).toThrow();
+  });
 
   test("when encountering an unsupported node type", () => {
     const path = { getValue: () => ({ type: "unsupported", body: {} }) };

test/js/setupTests.js

@@ -1,5 +1,4 @@
 const net = require("net");
-const path = require("path");
 const prettier = require("prettier");
 
 // eslint-disable-next-line no-underscore-dangle
@@ -76,36 +75,5 @@ expect.extend({
   },
   toMatchFormat(before, config = {}) {
     return checkFormat(before, before.code || before, config);
-  },
-  toFailFormat(before, message) {
-    let pass = false;
-    let error = null;
-
-    try {
-      prettier.format(before, { parser: "ruby", plugins: ["."] });
-    } catch (caught) {
-      error = caught;
-      pass = caught.message === message;
-    }
-
-    return {
-      pass,
-      message: () => `
-        Expected format to throw an error for ${before} with ${message},
-        but got ${error.message} instead
-      `
-    };
-  },
-  toInferParser(filename) {
-    const filepath = path.join(__dirname, filename);
-    const plugin = path.join(__dirname, "..", "..", "src", "plugin");
-
-    return prettier
-      .getFileInfo(filepath, { plugins: [plugin] })
-      .then((props) => ({
-        pass: props.inferredParser === "ruby",
-        message: () =>
-          `Expected prettier to infer the ruby parser for ${filename}, but got ${props.inferredParser} instead`
-      }));
   }
 });