Skip to content

[red-knot] Fix playground crashes when diagnostics are stale #17165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions playground/knot/src/Editor/Chrome.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ import {
Theme,
VerticalResizeHandle,
} from "shared";
import type { Diagnostic, Workspace } from "red_knot_wasm";
import type { Workspace } from "red_knot_wasm";
import { Panel, PanelGroup } from "react-resizable-panels";
import { Files, isPythonFile } from "./Files";
import SecondarySideBar from "./SecondarySideBar";
import SecondaryPanel, {
SecondaryPanelResult,
SecondaryTool,
} from "./SecondaryPanel";
import Diagnostics from "./Diagnostics";
import Diagnostics, { Diagnostic } from "./Diagnostics";
import { FileId, ReadonlyFiles } from "../Playground";
import type { editor } from "monaco-editor";
import type { Monaco } from "@monaco-editor/react";
Expand Down Expand Up @@ -168,7 +168,7 @@ export default function Chrome({
workspace={workspace}
onMount={handleEditorMount}
onChange={(content) => onFileChanged(workspace, content)}
onOpenFile={onFileSelected}
onFileOpened={onFileSelected}
/>
{checkResult.error ? (
<div
Expand All @@ -192,7 +192,6 @@ export default function Chrome({
>
<Diagnostics
diagnostics={checkResult.diagnostics}
workspace={workspace}
onGoTo={handleGoTo}
theme={theme}
/>
Expand Down Expand Up @@ -290,8 +289,18 @@ function useCheckResult(
};
}

// Eagerly convert the diagnostic to avoid out of bound errors
// when the diagnostics are "deferred".
const serializedDiagnostics = diagnostics.map((diagnostic) => ({
id: diagnostic.id(),
message: diagnostic.message(),
severity: diagnostic.severity(),
range: diagnostic.toRange(workspace) ?? null,
textRange: diagnostic.textRange() ?? null,
}));

return {
diagnostics,
diagnostics: serializedDiagnostics,
error: null,
secondary,
};
Expand Down
28 changes: 14 additions & 14 deletions playground/knot/src/Editor/Diagnostics.tsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
import { Diagnostic, Workspace } from "red_knot_wasm";
import type { Severity, Range, TextRange } from "red_knot_wasm";
import classNames from "classnames";
import { Theme } from "shared";
import { useMemo } from "react";

interface Props {
diagnostics: Diagnostic[];
workspace: Workspace;
theme: Theme;

onGoTo(line: number, column: number): void;
}

export default function Diagnostics({
diagnostics: unsorted,
workspace,
theme,
onGoTo,
}: Props) {
const diagnostics = useMemo(() => {
const sorted = [...unsorted];
sorted.sort((a, b) => {
return (a.textRange()?.start ?? 0) - (b.textRange()?.start ?? 0);
return (a.textRange?.start ?? 0) - (b.textRange?.start ?? 0);
});

return sorted;
Expand All @@ -43,11 +41,7 @@ export default function Diagnostics({
</div>

<div className="flex grow p-2 overflow-hidden">
<Items
diagnostics={diagnostics}
onGoTo={onGoTo}
workspace={workspace}
/>
<Items diagnostics={diagnostics} onGoTo={onGoTo} />
</div>
</div>
);
Expand All @@ -56,10 +50,8 @@ export default function Diagnostics({
function Items({
diagnostics,
onGoTo,
workspace,
}: {
diagnostics: Array<Diagnostic>;
workspace: Workspace;
onGoTo(line: number, column: number): void;
}) {
if (diagnostics.length === 0) {
Expand All @@ -73,9 +65,9 @@ function Items({
return (
<ul className="space-y-0.5 grow overflow-y-scroll">
{diagnostics.map((diagnostic, index) => {
const position = diagnostic.toRange(workspace);
const position = diagnostic.range;
const start = position?.start;
const id = diagnostic.id();
const id = diagnostic.id;

const startLine = start?.line ?? 1;
const startColumn = start?.column ?? 1;
Expand All @@ -86,7 +78,7 @@ function Items({
onClick={() => onGoTo(startLine, startColumn)}
className="w-full text-start cursor-pointer select-text"
>
{diagnostic.message()}
{diagnostic.message}
<span className="text-gray-500">
{id != null && ` (${id})`} [Ln {startLine}, Col {startColumn}]
</span>
Expand All @@ -97,3 +89,11 @@ function Items({
</ul>
);
}

export interface Diagnostic {
id: string;
message: string;
severity: Severity;
range: Range | null;
textRange: TextRange | null;
}
81 changes: 40 additions & 41 deletions playground/knot/src/Editor/Editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ import {
import { RefObject, useCallback, useEffect, useRef } from "react";
import { Theme } from "shared";
import {
Diagnostic,
Severity,
Workspace,
type Workspace,
Position as KnotPosition,
type Range as KnotRange,
} from "red_knot_wasm";

import IStandaloneCodeEditor = editor.IStandaloneCodeEditor;
import { FileId, ReadonlyFiles } from "../Playground";
import { isPythonFile } from "./Files";
import { Diagnostic } from "./Diagnostics";

type Props = {
visible: boolean;
Expand All @@ -38,7 +38,7 @@ type Props = {
workspace: Workspace;
onChange(content: string): void;
onMount(editor: IStandaloneCodeEditor, monaco: Monaco): void;
onOpenFile(file: FileId): void;
onFileOpened(file: FileId): void;
};

export default function Editor({
Expand All @@ -51,7 +51,7 @@ export default function Editor({
workspace,
onChange,
onMount,
onOpenFile,
onFileOpened,
}: Props) {
const disposable = useRef<{
typeDefinition: IDisposable;
Expand All @@ -61,14 +61,14 @@ export default function Editor({
monaco: null,
files,
workspace,
onOpenFile,
onFileOpened,
});

playgroundState.current = {
monaco: playgroundState.current.monaco,
files,
workspace,
onOpenFile,
onFileOpened,
};

// Update the diagnostics in the editor.
Expand All @@ -79,8 +79,8 @@ export default function Editor({
return;
}

updateMarkers(monaco, workspace, diagnostics);
}, [workspace, diagnostics]);
updateMarkers(monaco, diagnostics);
}, [diagnostics]);

const handleChange = useCallback(
(value: string | undefined) => {
Expand All @@ -98,7 +98,7 @@ export default function Editor({

const handleMount: OnMount = useCallback(
(editor, instance) => {
updateMarkers(instance, workspace, diagnostics);
updateMarkers(instance, diagnostics);

const server = new PlaygroundServer(playgroundState);
const typeDefinitionDisposable =
Expand All @@ -116,7 +116,7 @@ export default function Editor({
onMount(editor, instance);
},

[onMount, workspace, diagnostics],
[onMount, diagnostics],
);

return (
Expand All @@ -141,11 +141,7 @@ export default function Editor({
);
}

function updateMarkers(
monaco: Monaco,
workspace: Workspace,
diagnostics: Array<Diagnostic>,
) {
function updateMarkers(monaco: Monaco, diagnostics: Array<Diagnostic>) {
const editor = monaco.editor;
const model = editor?.getModels()[0];

Expand All @@ -170,16 +166,16 @@ function updateMarkers(
}
};

const range = diagnostic.toRange(workspace);
const range = diagnostic.range;

return {
code: diagnostic.id(),
code: diagnostic.id,
startLineNumber: range?.start?.line ?? 0,
startColumn: range?.start?.column ?? 0,
endLineNumber: range?.end?.line ?? 0,
endColumn: range?.end?.column ?? 0,
message: diagnostic.message(),
severity: mapSeverity(diagnostic.severity()),
message: diagnostic.message,
severity: mapSeverity(diagnostic.severity),
tags: [],
};
}),
Expand All @@ -191,7 +187,7 @@ interface PlaygroundServerProps {
workspace: Workspace;
files: ReadonlyFiles;

onOpenFile: (file: FileId) => void;
onFileOpened: (file: FileId) => void;
}

class PlaygroundServer
Expand Down Expand Up @@ -223,26 +219,29 @@ class PlaygroundServer
new KnotPosition(position.lineNumber, position.column),
);

const locations = links.map((link) => {
const targetSelection =
link.selection_range == null
? undefined
: knotRangeToIRange(link.selection_range);

const originSelection =
link.origin_selection_range == null
? undefined
: knotRangeToIRange(link.origin_selection_range);

return {
uri: Uri.parse(link.path),
range: knotRangeToIRange(link.full_range),
targetSelectionRange: targetSelection,
originSelectionRange: originSelection,
} as languages.LocationLink;
});

return locations;
return (
links
.map((link) => {
const targetSelection =
link.selection_range == null
? undefined
: knotRangeToIRange(link.selection_range);

const originSelection =
link.origin_selection_range == null
? undefined
: knotRangeToIRange(link.origin_selection_range);

return {
uri: Uri.parse(link.path),
range: knotRangeToIRange(link.full_range),
targetSelectionRange: targetSelection,
originSelectionRange: originSelection,
} as languages.LocationLink;
})
// Filter out vendored files because they aren't open in the editor.
.filter((link) => link.uri.scheme !== "vendored")
);
}

openCodeEditor(
Expand Down Expand Up @@ -284,7 +283,7 @@ class PlaygroundServer
if (files.selected !== fileId) {
source.setModel(model);

this.props.current.onOpenFile(fileId);
this.props.current.onFileOpened(fileId);
}

if (selectionOrPosition != null) {
Expand Down
Loading