From 3da69c9b5da949f00fd05804624bdff1e1e3b254 Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Fri, 28 Jun 2024 18:42:05 +0000 Subject: [PATCH 1/5] Move linting code to under Stanc, tests --- gui/src/app/FileEditor/StanFileEditor.tsx | 97 +----------------- gui/src/app/FileEditor/TextEditor.tsx | 11 +- gui/src/app/Stanc/Linting.ts | 117 ++++++++++++++++++++++ gui/test/app/Stanc/Linting.test.ts | 112 +++++++++++++++++++++ 4 files changed, 232 insertions(+), 105 deletions(-) create mode 100644 gui/src/app/Stanc/Linting.ts create mode 100644 gui/test/app/Stanc/Linting.test.ts diff --git a/gui/src/app/FileEditor/StanFileEditor.tsx b/gui/src/app/FileEditor/StanFileEditor.tsx index 782235c7..f6bfa2db 100644 --- a/gui/src/app/FileEditor/StanFileEditor.tsx +++ b/gui/src/app/FileEditor/StanFileEditor.tsx @@ -9,9 +9,9 @@ import { } from "react"; import StanCompileResultWindow from "./StanCompileResultWindow"; import useStanc from "../Stanc/useStanc"; -import TextEditor, { CodeMarker, ToolbarItem } from "./TextEditor"; +import TextEditor, { ToolbarItem } from "./TextEditor"; import compileStanProgram from "../compileStanProgram/compileStanProgram"; -import { StancErrors } from "../Stanc/Types"; +import { stancErrorsToCodeMarkers } from "../Stanc/Linting"; type Props = { fileName: string; @@ -269,97 +269,4 @@ const stringChecksum = (str: string) => { return hash; }; -const stancErrorsToCodeMarkers = (stancErrors: StancErrors) => { - const codeMarkers: CodeMarker[] = []; - - for (const x of stancErrors.errors || []) { - const marker = stancErrorStringToMarker(x, "error"); - if (marker) codeMarkers.push(marker); - } - for (const x of stancErrors.warnings || []) { - const marker = stancErrorStringToMarker(x, "warning"); - if (marker) codeMarkers.push(marker); - } - - return codeMarkers; -}; - -const stancErrorStringToMarker = ( - x: string, - severity: "error" | "warning", -): CodeMarker | undefined => { - if (!x) return undefined; - - // Example: Syntax error in 'main.stan', line 1, column 0 to column 1, parsing error: - - let lineNumber: number | undefined = undefined; - let startColumn: number | undefined = undefined; - let endColumn: number | undefined = undefined; - - const sections = x.split(",").map((x) => x.trim()); - for (const section of sections) { - if (section.startsWith("line ") && lineNumber === undefined) { - lineNumber = parseInt(section.slice("line ".length)); - } else if (section.startsWith("column ") && startColumn === undefined) { - const cols = section.slice("column ".length).split(" to "); - startColumn = parseInt(cols[0]); - endColumn = - cols.length > 1 - ? parseInt(cols[1].slice("column ".length)) - : startColumn + 1; - } - } - - if ( - lineNumber !== undefined && - startColumn !== undefined && - endColumn !== undefined - ) { - return { - startLineNumber: lineNumber, - startColumn: startColumn + 1, - endLineNumber: lineNumber, - endColumn: endColumn + 1, - message: - severity === "warning" ? getWarningMessage(x) : getErrorMessage(x), - severity, - }; - } else { - return undefined; - } -}; - -//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// -// Adapted from https://github.com/WardBrian/vscode-stan-extension -function getWarningMessage(message: string) { - let warning = message.replace(/Warning.*column \d+: /s, ""); - warning = warning.replace(/\s+/gs, " "); - warning = warning.trim(); - warning = message.includes("included from") - ? "Warning in included file:\n" + warning - : warning; - return warning; -} - -function getErrorMessage(message: string) { - let error = message; - // cut off code snippet for display - if (message.includes("------\n")) { - error = error.split("------\n")[2]; - } - error = error.trim(); - error = message.includes("included from") - ? "Error in included file:\n" + error - : error; - - // only relevant to vscode-stan-extension: - // error = error.includes("given information about") - // ? error + - // "\nConsider updating the includePaths setting of vscode-stan-extension" - // : error; - - return error; -} -//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - export default StanFileEditor; diff --git a/gui/src/app/FileEditor/TextEditor.tsx b/gui/src/app/FileEditor/TextEditor.tsx index f2cf2b5d..c42036d1 100644 --- a/gui/src/app/FileEditor/TextEditor.tsx +++ b/gui/src/app/FileEditor/TextEditor.tsx @@ -12,19 +12,10 @@ import { } from "react"; import { highlightJsData } from "./stanLang"; import { Hyperlink, SmallIconButton } from "@fi-sci/misc"; +import { CodeMarker } from "../Stanc/Linting"; type Monaco = typeof monaco; -// An interface for passing markers (squiggles) to the editor without depending on monaco types -export type CodeMarker = { - startLineNumber: number; - startColumn: number; - endLineNumber: number; - endColumn: number; - message: string; - severity: "error" | "warning" | "hint" | "info"; -}; - type Props = { defaultText?: string; text: string | undefined; diff --git a/gui/src/app/Stanc/Linting.ts b/gui/src/app/Stanc/Linting.ts new file mode 100644 index 00000000..6afba6e5 --- /dev/null +++ b/gui/src/app/Stanc/Linting.ts @@ -0,0 +1,117 @@ +import { StancErrors } from "./Types"; + +type Position = { + startLineNumber: number; + startColumn: number; + endLineNumber: number; + endColumn: number; +}; + +// An interface for passing markers (squiggles) to the editor without depending on monaco types +export type CodeMarker = Position & { + message: string; + severity: "error" | "warning" | "hint" | "info"; +}; + +export const stancErrorsToCodeMarkers = (stancErrors: StancErrors) => { + const codeMarkers: CodeMarker[] = []; + + for (const x of stancErrors.errors || []) { + const marker = stancMessageToMarker(x, "error"); + if (marker) codeMarkers.push(marker); + } + for (const x of stancErrors.warnings || []) { + const marker = stancMessageToMarker(x, "warning"); + if (marker) codeMarkers.push(marker); + } + + return codeMarkers; +}; + +const stancMessageToMarker = ( + message: string, + severity: "error" | "warning", +): CodeMarker | undefined => { + const position = locationFromMessage(message); + if (position === undefined) return undefined; + const { startLineNumber, startColumn, endLineNumber, endColumn } = position; + + return { + startLineNumber, + startColumn, + endLineNumber, + endColumn, + message: + severity === "warning" + ? getWarningMessage(message) + : getErrorMessage(message), + severity, + }; +}; + +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// Adapted from https://github.com/WardBrian/vscode-stan-extension + +const locationFromMessage = (message: string): Position | undefined => { + if (!message) return; + // format is "in 'filename', line (#)), column (#) to (line #,)? column (#)" + const start = message.matchAll(/'.*', line (\d+), column (\d+)( to)?/g); + if (!start) { + return undefined; + } + // there will be multiple in the case of #included files + const lastMatch = Array.from(start).pop(); + if (!lastMatch) { + return undefined; + } + + const startLineNumber = parseInt(lastMatch[1]); + const startColumn = parseInt(lastMatch[2]) + 1; + + let endLineNumber = startLineNumber; + let endColumn = startColumn; + + if (lastMatch[3]) { + // " to" was matched + const end = message.match(/to (line (\d+), )?column (\d+)/); + if (end) { + if (end[1]) { + endLineNumber = parseInt(end[2]); + } + endColumn = parseInt(end[3]) + 1; + } + } + + return { startLineNumber, startColumn, endLineNumber, endColumn }; +}; + +function getWarningMessage(message: string) { + let warning = message.replace(/Warning.*column \d+:/s, ""); + warning = warning.replace(/\s+/gs, " "); + warning = warning.trim(); + warning = message.includes("included from") + ? "Warning in included file:\n" + warning + : warning; + return warning; +} + +function getErrorMessage(message: string) { + let error = message; + // cut off code snippet for display + if (message.includes("------\n")) { + error = error.split("------\n")[2]; + } + error = error.trim(); + error = message.includes("included from") + ? "Error in included file:\n" + error + : error; + + return error; +} +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + +export const exportedForTesting = { + locationFromMessage, + getErrorMessage, + getWarningMessage, +}; diff --git a/gui/test/app/Stanc/Linting.test.ts b/gui/test/app/Stanc/Linting.test.ts new file mode 100644 index 00000000..13109874 --- /dev/null +++ b/gui/test/app/Stanc/Linting.test.ts @@ -0,0 +1,112 @@ +import { assert, describe, expect, test } from "vitest"; +import { exportedForTesting } from "../../../src/app/Stanc/Linting"; + +const { locationFromMessage, getWarningMessage, getErrorMessage } = + exportedForTesting; + +// all messages actually generated by stanc +const jacobianWarning = `Warning in 'jacobian.stan', line 1, column 12: Variable name 'jacobian' will + be a reserved word starting in Stan 2.38.0. Please rename it! +`; + +const multiColumnError = `Semantic error in 'multicol.stan', line 3, column 4 to column 10: + ------------------------------------------------- + 1: parameters { + 2: real y; + 3: int x; + ^ + 4: } + 5: model { + ------------------------------------------------- + +(Transformed) Parameters cannot be integers. +`; + +const multiLineError = `Semantic error in 'multiline.stan', line 2, column 2 to line 4, column 16: + ------------------------------------------------- + 1: generated quantities { + 2: array[3] int x + ^ + 3: = + 4: 10; + ------------------------------------------------- + +Ill-typed arguments supplied to assignment operator =: +The left hand side has type + array[] int +and the right hand side has type + int +`; + +describe("Linting", () => { + describe("position detection", () => { + test("random message returns undefined", () => { + const message = "random message"; + const position = locationFromMessage(message); + expect(position).toBeUndefined(); + }); + + test("should detect the position of a single line, single column message", () => { + const position = locationFromMessage(jacobianWarning); + expect(position).toBeDefined(); + assert(position); + expect(position.startLineNumber).toEqual(position.endLineNumber); + expect(position.startColumn).toEqual(position.endColumn); + + expect(position.startLineNumber).toEqual(1); + expect(position.startColumn).toEqual(13); // 0 vs 1 based indexing + }); + + test("should detect the position of a single line, multi-column error", () => { + const position = locationFromMessage(multiColumnError); + expect(position).toBeDefined(); + assert(position); + expect(position.startLineNumber).toEqual(position.endLineNumber); + expect(position.startColumn).not.toEqual(position.endColumn); + + expect(position.startLineNumber).toEqual(3); + expect(position.startColumn).toEqual(5); + expect(position.endColumn).toEqual(11); + }); + + test("should detect the position of a multi-line, multi-column error", () => { + const position = locationFromMessage(multiLineError); + expect(position).toBeDefined(); + assert(position); + expect(position.startLineNumber).not.toEqual(position.endLineNumber); + expect(position.startColumn).not.toEqual(position.endColumn); + + expect(position.startLineNumber).toEqual(2); + expect(position.startColumn).toEqual(3); + expect(position.endLineNumber).toEqual(4); + expect(position.endColumn).toEqual(17); + }); + }); + + describe("message extraction", () => { + test("should extract warning message without position", () => { + const warningMessage = getWarningMessage(jacobianWarning); + expect(warningMessage).toEqual( + "Variable name 'jacobian' will be a reserved word starting in Stan 2.38.0. Please rename it!", + ); + }); + + test("should extract error message without position or snippet", () => { + const errorMessage = getErrorMessage(multiColumnError); + expect(errorMessage).toEqual( + "(Transformed) Parameters cannot be integers.", + ); + }); + + test("should extract longer error message without position and snippet", () => { + const errorMessage = getErrorMessage(multiLineError); + expect(errorMessage).toEqual( + `Ill-typed arguments supplied to assignment operator =: +The left hand side has type + array[] int +and the right hand side has type + int`, + ); + }); + }); +}); From 9c187cd25c0d8508cee310e53e9cff2c745a64af Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Mon, 1 Jul 2024 16:53:35 +0000 Subject: [PATCH 2/5] Address review comments --- gui/src/app/Stanc/Linting.ts | 42 ++++++++++-------------------- gui/test/app/Stanc/Linting.test.ts | 3 ++- 2 files changed, 16 insertions(+), 29 deletions(-) diff --git a/gui/src/app/Stanc/Linting.ts b/gui/src/app/Stanc/Linting.ts index 6afba6e5..9f6d0ad7 100644 --- a/gui/src/app/Stanc/Linting.ts +++ b/gui/src/app/Stanc/Linting.ts @@ -14,16 +14,14 @@ export type CodeMarker = Position & { }; export const stancErrorsToCodeMarkers = (stancErrors: StancErrors) => { - const codeMarkers: CodeMarker[] = []; - - for (const x of stancErrors.errors || []) { - const marker = stancMessageToMarker(x, "error"); - if (marker) codeMarkers.push(marker); - } - for (const x of stancErrors.warnings || []) { - const marker = stancMessageToMarker(x, "warning"); - if (marker) codeMarkers.push(marker); - } + const codeMarkers = [ + ...(stancErrors.errors || []).map((error) => + stancMessageToMarker(error, "error"), + ), + ...(stancErrors.warnings || []).map((warning) => + stancMessageToMarker(warning, "warning"), + ), + ]; return codeMarkers; }; @@ -49,16 +47,12 @@ const stancMessageToMarker = ( }; }; -//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Adapted from https://github.com/WardBrian/vscode-stan-extension const locationFromMessage = (message: string): Position | undefined => { - if (!message) return; - // format is "in 'filename', line (#)), column (#) to (line #,)? column (#)" + if (!message) return undefined; + // format is "in 'filename', line (#), column (#) to (line #,)? column (#)" const start = message.matchAll(/'.*', line (\d+), column (\d+)( to)?/g); - if (!start) { - return undefined; - } // there will be multiple in the case of #included files const lastMatch = Array.from(start).pop(); if (!lastMatch) { @@ -85,30 +79,22 @@ const locationFromMessage = (message: string): Position | undefined => { return { startLineNumber, startColumn, endLineNumber, endColumn }; }; -function getWarningMessage(message: string) { +const getWarningMessage = (message: string) => { let warning = message.replace(/Warning.*column \d+:/s, ""); warning = warning.replace(/\s+/gs, " "); warning = warning.trim(); - warning = message.includes("included from") - ? "Warning in included file:\n" + warning - : warning; return warning; -} +}; -function getErrorMessage(message: string) { +const getErrorMessage = (message: string) => { let error = message; // cut off code snippet for display if (message.includes("------\n")) { error = error.split("------\n")[2]; } error = error.trim(); - error = message.includes("included from") - ? "Error in included file:\n" + error - : error; - return error; -} -//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +}; export const exportedForTesting = { locationFromMessage, diff --git a/gui/test/app/Stanc/Linting.test.ts b/gui/test/app/Stanc/Linting.test.ts index 13109874..a9e99a52 100644 --- a/gui/test/app/Stanc/Linting.test.ts +++ b/gui/test/app/Stanc/Linting.test.ts @@ -54,7 +54,8 @@ describe("Linting", () => { expect(position.startColumn).toEqual(position.endColumn); expect(position.startLineNumber).toEqual(1); - expect(position.startColumn).toEqual(13); // 0 vs 1 based indexing + // NOTE: stanc emits 0-based column numbers, but Monaco is 1-based + expect(position.startColumn).toEqual(13); }); test("should detect the position of a single line, multi-column error", () => { From 9b4f10576bdcae23d0bc33796c7168aa0ef05f95 Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Mon, 1 Jul 2024 17:02:27 +0000 Subject: [PATCH 3/5] Full test coverage --- gui/src/app/Stanc/Linting.ts | 2 +- gui/test/app/Stanc/Linting.test.ts | 55 +++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/gui/src/app/Stanc/Linting.ts b/gui/src/app/Stanc/Linting.ts index 9f6d0ad7..99710a05 100644 --- a/gui/src/app/Stanc/Linting.ts +++ b/gui/src/app/Stanc/Linting.ts @@ -23,7 +23,7 @@ export const stancErrorsToCodeMarkers = (stancErrors: StancErrors) => { ), ]; - return codeMarkers; + return codeMarkers.filter((marker) => marker !== undefined); }; const stancMessageToMarker = ( diff --git a/gui/test/app/Stanc/Linting.test.ts b/gui/test/app/Stanc/Linting.test.ts index a9e99a52..a5a0f93d 100644 --- a/gui/test/app/Stanc/Linting.test.ts +++ b/gui/test/app/Stanc/Linting.test.ts @@ -1,5 +1,8 @@ import { assert, describe, expect, test } from "vitest"; -import { exportedForTesting } from "../../../src/app/Stanc/Linting"; +import { + exportedForTesting, + stancErrorsToCodeMarkers, +} from "../../../src/app/Stanc/Linting"; const { locationFromMessage, getWarningMessage, getErrorMessage } = exportedForTesting; @@ -40,6 +43,12 @@ and the right hand side has type describe("Linting", () => { describe("position detection", () => { + test("empty message returns undefined", () => { + const message = ""; + const position = locationFromMessage(message); + expect(position).toBeUndefined(); + }); + test("random message returns undefined", () => { const message = "random message"; const position = locationFromMessage(message); @@ -110,4 +119,48 @@ and the right hand side has type ); }); }); + + describe("from Stanc Errors", () => { + test("empty errors returns empty list", () => { + const stancErrors = {}; + const codeMarkers = stancErrorsToCodeMarkers(stancErrors); + expect(codeMarkers).toEqual([]); + }); + + test("bogus errors returns empty list", () => { + const stancErrors = { + errors: ["bogus error"], + warnings: ["bogus warning"], + }; + const codeMarkers = stancErrorsToCodeMarkers(stancErrors); + expect(codeMarkers).toEqual([]); + }); + + test("single warning returns single warning marker", () => { + const stancErrors = { + warnings: [jacobianWarning], + }; + const codeMarkers = stancErrorsToCodeMarkers(stancErrors); + expect(codeMarkers).toHaveLength(1); + expect(codeMarkers[0]?.severity).toEqual("warning"); + }); + + test("single error returns single error marker", () => { + const stancErrors = { + errors: [multiColumnError], + }; + const codeMarkers = stancErrorsToCodeMarkers(stancErrors); + expect(codeMarkers).toHaveLength(1); + expect(codeMarkers[0]?.severity).toEqual("error"); + }); + + test("full stanc errors returns all markers", () => { + const stancErrors = { + errors: [multiColumnError, multiLineError], + warnings: [jacobianWarning, jacobianWarning, jacobianWarning], + }; + const codeMarkers = stancErrorsToCodeMarkers(stancErrors); + expect(codeMarkers).toHaveLength(5); + }); + }); }); From b504982708162935815d01aa6404123f7fcd026f Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Mon, 1 Jul 2024 19:00:22 +0000 Subject: [PATCH 4/5] Type fix --- gui/src/app/Stanc/Linting.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gui/src/app/Stanc/Linting.ts b/gui/src/app/Stanc/Linting.ts index 99710a05..6f3ab939 100644 --- a/gui/src/app/Stanc/Linting.ts +++ b/gui/src/app/Stanc/Linting.ts @@ -23,7 +23,7 @@ export const stancErrorsToCodeMarkers = (stancErrors: StancErrors) => { ), ]; - return codeMarkers.filter((marker) => marker !== undefined); + return codeMarkers.filter((marker) => marker !== undefined) as CodeMarker[]; }; const stancMessageToMarker = ( From d3c67b75cf96cf8ff0cd862240d5d2dbb882fb24 Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Mon, 1 Jul 2024 19:19:28 +0000 Subject: [PATCH 5/5] Clarify situation where undefined position arises --- gui/test/app/Stanc/Linting.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/gui/test/app/Stanc/Linting.test.ts b/gui/test/app/Stanc/Linting.test.ts index a5a0f93d..7b4f77f4 100644 --- a/gui/test/app/Stanc/Linting.test.ts +++ b/gui/test/app/Stanc/Linting.test.ts @@ -12,6 +12,10 @@ const jacobianWarning = `Warning in 'jacobian.stan', line 1, column 12: Variable be a reserved word starting in Stan 2.38.0. Please rename it! `; +// note: stanc can produce warnings like this that do not contain a position! +const emptyModelWarning = `Warning: Empty file 'empty.stan' detected; this is a valid stan model but + likely unintended!`; + const multiColumnError = `Semantic error in 'multicol.stan', line 3, column 4 to column 10: ------------------------------------------------- 1: parameters { @@ -49,9 +53,8 @@ describe("Linting", () => { expect(position).toBeUndefined(); }); - test("random message returns undefined", () => { - const message = "random message"; - const position = locationFromMessage(message); + test("message without position returns undefined", () => { + const position = locationFromMessage(emptyModelWarning); expect(position).toBeUndefined(); });