Capture parse and source errors and make them redirect comments (#60)

* Capture parse and source errors and make them redirect comments

* Fix shared comment names

Author: SleeplessByte

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## 0.6.0
+
+- Fix various `shared` comments which had `generic` instead of `general` in their name.
+- Fix a bug with the parser that caused some solutions to blow up
+- Add `makeParseErrorOutput` and `makeNoSourceOutput` to redirect to a mentor with helpful information
+
 ## 0.5.3
 
 - Fix `resistor-color` constant lookup, when name doesn't match "probably"

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@exercism/javascript-analyzer",
-  "version": "0.5.3",
+  "version": "0.6.0",
   "description": "Exercism analyzer for javascript",
   "repository": "https://github.com/exercism/javascript-analyzer",
   "author": "Derk-Jan Karrenbeld <[email protected]>",

src/analyzers/IsolatedAnalyzerImpl.ts

@@ -1,6 +1,9 @@
-import { getProcessLogger as getLogger, Logger } from '~src/utils/logger'
-
-import { IsolatedAnalyzerOutput, EarlyFinalization } from '~src/output/IsolatedAnalyzerOutput';
+import { NoSourceError } from '~src/errors/NoSourceError';
+import { ParserError } from '~src/errors/ParserError';
+import { EarlyFinalization, IsolatedAnalyzerOutput } from '~src/output/IsolatedAnalyzerOutput';
+import { getProcessLogger as getLogger, Logger } from '~src/utils/logger';
+import { makeNoSourceOutput } from '~src/output/makeNoSourceOutput';
+import { makeParseErrorOutput } from '~src/output/makeParseErrorOutput';
 
 export abstract class IsolatedAnalyzerImpl implements Analyzer {
   protected readonly logger: Logger
@@ -23,9 +26,31 @@ export abstract class IsolatedAnalyzerImpl implements Analyzer {
    * @memberof BaseAnalyzer
    */
   public async run(input: Input): Promise<Output> {
+    return this.run_(input)
+      .catch((err: Error) => {
+
+        // Here we handle errors that blew up the analyzer but we don't want to
+        // report as blown up. This converts these errors to the commentary.
+        if (err instanceof NoSourceError) {
+          return makeNoSourceOutput(err)
+        } else if (err instanceof ParserError) {
+          return makeParseErrorOutput(err)
+        }
+
+        // Unhandled issue
+        return Promise.reject(err)
+      })
+  }
+
+  private async run_(input: Input): Promise<Output> {
     const output = new IsolatedAnalyzerOutput()
+
+    // Block and execute
     await this.execute(input, output)
       .catch((err): void | never => {
+
+        // The isolated analyzer output can use exceptions as control flow.
+        // This block here explicitely accepts this.
         if (err instanceof EarlyFinalization) {
           this.logger.log(`=> early finialization (${output.status})`)
         } else {

src/analyzers/SourceImpl.ts

@@ -13,21 +13,33 @@ class SourceImpl implements Source {
   private readonly lines: string[]
 
   constructor(source: string) {
-    this.lines = source.split("\n")
+    this.lines = source.split(/\r?\n/)
   }
 
-  public get(node: NodeWithLocation): string {
-    const start = this.lines[node.loc.start.line - 1]
-    const end = this.lines[node.loc.end.line - 1]
-    if (start === end) {
-      return start.substring(node.loc.start.column, node.loc.end.column)
+  public getLines(node: NodeWithLocation): string[] {
+    const startLineLoc = Math.max(0, node.loc.start.line - 1)
+    const start = this.lines[startLineLoc]
+    const endLineLoc = Math.min(this.lines.length - 1, node.loc.end.line - 1)
+
+    if (startLineLoc === endLineLoc) {
+      return [start.substring(node.loc.start.column, node.loc.end.column)]
     }
 
+    const end = this.lines[endLineLoc]
+
     return [
       start.substring(node.loc.start.column),
-      ...this.lines.slice(node.loc.start.line, node.loc.end.line - 2),
-      end.substring(0, node.loc.end.column)
-    ].join("\n")
+      ...(
+        startLineLoc + 1 <= endLineLoc - 1
+          ? this.lines.slice(startLineLoc + 1, endLineLoc - 1)
+          : []
+      ),
+      end.substring(0, node.loc.end.column < 0 ? undefined : node.loc.end.column)
+    ]
+  }
+
+  public get(node: NodeWithLocation): string {
+    return this.getLines(node).join("\n")
   }
 
   public getOuter(node: NodeWithLocation): string {

src/analyzers/two-fer/index.ts

@@ -20,6 +20,8 @@ import { NO_METHOD, NO_NAMED_EXPORT, NO_PARAMETER, PREFER_STRICT_EQUALITY, PREFE
 import { AstParser, ParsedSource } from "~src/parsers/AstParser";
 import { NoSourceError } from "~src/errors/NoSourceError";
 import { ParserError } from "~src/errors/ParserError";
+import { makeParseErrorOutput } from "~src/output/makeParseErrorOutput";
+import { makeNoSourceOutput } from "~src/output/makeNoSourceOutput";
 
 /**
  * The factories here SHOULD be kept in sync with exercism/website-copy. Under
@@ -132,15 +134,17 @@ export class TwoFerAnalyzer extends AnalyzerImpl {
     try {
       return await Parser.parse(input)
     } catch (err) {
+
+      // Here we handle errors that blew up the analyzer but we don't want to
+      // report as blown up. This converts these errors to the commentary.
       if (err instanceof NoSourceError) {
-        this.logger.error(`=> [NoSourceError] ${err.message}`)
+        const output = makeNoSourceOutput(err)
+        output.comments.forEach((comment) => this.comment(comment))
+        this.redirect()
+      } else if (err instanceof ParserError) {
+        const output = makeParseErrorOutput(err)
+        output.comments.forEach((comment) => this.comment(comment))
         this.redirect()
-      }
-
-      if (err instanceof ParserError) {
-        this.logger.error(`=> [ParserError] ${err.message}`)
-        const { message, ...details } = err.original
-        this.disapprove(PARSE_ERROR({ error: message, details: JSON.stringify(details) }))
       }
 
       throw err

src/comments/shared.ts

@@ -74,7 +74,7 @@ Instead, in Javascript, prefixing a parameter with an underscore will stop
 most IDEs from highlighting that parameter if it's unused, which is actually a
 tool you probably want to keep in this case. Remove the underscore \`_\` from
 ${'parameter.name'} in order to fix this.
-`('javascript.generic.prefer_unprefixed_underscore_parameters')
+`('javascript.general.prefer_unprefixed_underscore_parameters')
 
 export const PARSE_ERROR = factory<'error' | 'details'>`
 There is something wrong with your submission, most likely a Syntax Error:
@@ -84,14 +84,14 @@ Message: "${'error'}"
 \`\`\`
 ${'details'}
 \`\`\`
-`('javascript.generic.parse_error')
+`('javascript.general.parse_error')
 
 export const PREFER_CONST_OVER_LET_AND_VAR = factory<'kind' | 'name'>`
 Instead of \`${'kind'} ${'name'}\`, consider using \`const\`.
 
 \`const\` is a signal that the identifier won't be reassigned, which SHOULD be
 true for this top-level constant. (Not to be confused with _immutable values_).
-`('javascript.generic.prefer_const_over_let_and_var')
+`('javascript.general.prefer_const_over_let_and_var')
 
 export const BETA_COMMENTARY_PREFIX = factory`
 🧪 This solution's output contains a new format of comments that is currently
@@ -107,4 +107,8 @@ please open an issue [here](https://github.com/exercism/javascript-analyzer/issu
 this to the student directly. Use it to determine what you want to say.
 - If there is no icon, the commentary has not been updated to the latest
 standard. Proceed with caution.
-`('javascript.generic.beta_disapprove_commentary_prefix')
+`('javascript.general.beta_disapprove_commentary_prefix')
+
+export const ERROR_CAPTURED_NO_SOURCE = factory<'expected' | 'available'>`
+Expected source file "${'expected'}", found: ${'available'}.
+`('javascript.general.error_captured_no_source')

src/errors/NoSourceError.ts

@@ -2,9 +2,19 @@ import { SOURCE_MISSING_ERROR } from "./codes";
 
 export class NoSourceError extends Error {
   public readonly code: typeof SOURCE_MISSING_ERROR;
+  public readonly expected: string;
+  public readonly available: string[];
+
+  constructor(expected?: string, available?: string[]) {
+    super(
+      expected
+        ? `Expected source file "${expected}", found: ${JSON.stringify(available)}`
+        : 'No source file(s) found'
+    )
+
+    this.expected = expected || '<unknown>'
+    this.available = available || []
 
-  constructor() {
-    super('No source file(s) found')
     Error.captureStackTrace(this, this.constructor)
 
     this.code = SOURCE_MISSING_ERROR

src/errors/ParserError.ts

@@ -3,7 +3,7 @@ import { SOURCE_PARSE_ERROR } from "./codes";
 export class ParserError extends Error {
   public readonly code: typeof SOURCE_PARSE_ERROR;
 
-  constructor(public readonly original: Error) {
+  constructor(public readonly original: Error & { lineNumber: number; column: number; index: number }, public readonly source?: string) {
     super(`
 Could not parse the source; most likely due to a syntax error.
 
@@ -15,3 +15,4 @@ ${original.message}
     this.code = SOURCE_PARSE_ERROR
   }
 }
+

src/input/DirectoryInput.ts

@@ -2,6 +2,7 @@ import { readDir } from "~src/utils/fs";
 import { FileInput } from "./FileInput";
 
 import nodePath from 'path'
+import { NoSourceError } from "~src/errors/NoSourceError";
 
 const EXTENSIONS = /\.(jsx?|tsx?|mjs)$/
 const TEST_FILES = /\.spec|test\./
@@ -12,8 +13,8 @@ export class DirectoryInput implements Input {
 
   public async read(n = 1): Promise<string[]> {
     const files = await readDir(this.path)
-
     const candidates = findCandidates(files, n, `${this.exerciseSlug}.js`)
+
     const fileSources = await Promise.all(
       candidates.map((candidate): Promise<string> => {
         return new FileInput(nodePath.join(this.path, candidate))
@@ -24,6 +25,13 @@ export class DirectoryInput implements Input {
 
     return fileSources
   }
+
+  public async informativeBail(): Promise<never> {
+    const expected = `${this.exerciseSlug}.js`
+    const available = await readDir(this.path)
+
+    return Promise.reject(new NoSourceError(expected, available))
+  }
 }
 
 /**

src/input/FileInput.ts

@@ -7,4 +7,8 @@ export class FileInput implements Input {
     const buffer = await readFile(this.path)
     return [buffer.toString("utf8")]
   }
+
+  public async informativeBail(): Promise<never> {
+    return Promise.reject(new Error(`Could not read file "${this.path}"`))
+  }
 }