mirror of https://github.com/astral-sh/ruff
[red-knot] Fix playground crashes when diagnostics are stale (#17165)
## Summary Fixes a crash in the playground where it crashed with an "index out of bounds" error in the `Diagnostic::to_range` call after deleting content at the end of the file. The root cause was that the playground uses `useDeferred` to avoid too frequent `checkFile` calls (to get a smoother UX). However, this has the problem that the rendered `diagnostics` can be stable (from before the last change). Rendering the diagnostics can then fail because the `toRange` call queries the latest content and not the content from when the diagnostics were created. The fix is "easy" in the sense that we now eagerly perform the `toRange` calls. This way, it doesn't matter when the diagnostics are stale for a few ms. This problem can only be observed on examples where Red Knot is "slow" (takes more than ~16ms to check) because only then does `useDeferred` "debounce" the `check` calls.
This commit is contained in:
Micha Reiser
GitHub
parent
177afabe18
commit
66355a6185
|
|
@ -13,7 +13,7 @@ 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";
|
||||
|
|
@ -21,7 +21,7 @@ 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";
|
||||
|
|
@ -168,7 +168,7 @@ export default function Chrome({
|
|||
workspace={workspace}
|
||||
onMount={handleEditorMount}
|
||||
onChange={(content) => onFileChanged(workspace, content)}
|
||||
onOpenFile={onFileSelected}
|
||||
onFileOpened={onFileSelected}
|
||||
/>
|
||||
{checkResult.error ? (
|
||||
<div
|
||||
|
|
@ -192,7 +192,6 @@ export default function Chrome({
|
|||
>
|
||||
<Diagnostics
|
||||
diagnostics={checkResult.diagnostics}
|
||||
workspace={workspace}
|
||||
onGoTo={handleGoTo}
|
||||
theme={theme}
|
||||
/>
|
||||
|
|
@ -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,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,11 +1,10 @@
|
|||
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;
|
||||
|
|
@ -13,14 +12,13 @@ interface Props {
|
|||
|
||||
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;
|
||||
|
|
@ -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>
|
||||
);
|
||||
|
|
@ -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) {
|
||||
|
|
@ -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;
|
||||
|
|
@ -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>
|
||||
|
|
@ -97,3 +89,11 @@ function Items({
|
|||
</ul>
|
||||
);
|
||||
}
|
||||
|
||||
export interface Diagnostic {
|
||||
id: string;
|
||||
message: string;
|
||||
severity: Severity;
|
||||
range: Range | null;
|
||||
textRange: TextRange | null;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17,9 +17,8 @@ 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";
|
||||
|
|
@ -27,6 +26,7 @@ import {
|
|||
import IStandaloneCodeEditor = editor.IStandaloneCodeEditor;
|
||||
import { FileId, ReadonlyFiles } from "../Playground";
|
||||
import { isPythonFile } from "./Files";
|
||||
import { Diagnostic } from "./Diagnostics";
|
||||
|
||||
type Props = {
|
||||
visible: boolean;
|
||||
|
|
@ -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({
|
||||
|
|
@ -51,7 +51,7 @@ export default function Editor({
|
|||
workspace,
|
||||
onChange,
|
||||
onMount,
|
||||
onOpenFile,
|
||||
onFileOpened,
|
||||
}: Props) {
|
||||
const disposable = useRef<{
|
||||
typeDefinition: IDisposable;
|
||||
|
|
@ -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.
|
||||
|
|
@ -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) => {
|
||||
|
|
@ -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 =
|
||||
|
|
@ -116,7 +116,7 @@ export default function Editor({
|
|||
onMount(editor, instance);
|
||||
},
|
||||
|
||||
[onMount, workspace, diagnostics],
|
||||
[onMount, diagnostics],
|
||||
);
|
||||
|
||||
return (
|
||||
|
|
@ -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];
|
||||
|
||||
|
|
@ -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: [],
|
||||
};
|
||||
}),
|
||||
|
|
@ -191,7 +187,7 @@ interface PlaygroundServerProps {
|
|||
workspace: Workspace;
|
||||
files: ReadonlyFiles;
|
||||
|
||||
onOpenFile: (file: FileId) => void;
|
||||
onFileOpened: (file: FileId) => void;
|
||||
}
|
||||
|
||||
class PlaygroundServer
|
||||
|
|
@ -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);
|
||||
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);
|
||||
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 {
|
||||
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(
|
||||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue