Skip to content
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

Region-Prioritized Error Checking for Editors #57393

Closed
DanielRosenwasser opened this issue Feb 13, 2024 · 3 comments · Fixed by #57842 or microsoft/vscode#208713
Closed

Region-Prioritized Error Checking for Editors #57393

DanielRosenwasser opened this issue Feb 13, 2024 · 3 comments · Fixed by #57842 or microsoft/vscode#208713
Assignees
Labels
Domain: Error Messages The issue relates to error messaging Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

One of the difficulties in working in larger files is that error updates have per-file granularity. This means that even if a viewport is focused on a region of 60 lines of code, the entire file must be type-checked. For a file that's more than 50,000 lines like TypeScript's checker.ts, it can be painfully slow to wait for errors to go away after typing something.

One idea might be to enrich TypeScript's existing errors requests with regions of interest. By specifying the viewport of the editor as a reigon, TypeScript can first prioritize gathering errors across statements that are visible to a user, and inform an editor of any diagnostic/error spans discovered in that region. Any prior diagnostics in that region can be considered invalidated, and a subsequent response with all the diagnostics can be returned afterwards.

Implementation

Right now TypeScript has a GeterrRequestArgs that looks like this:

/**
 * Arguments for geterr messages.
 */
export interface GeterrRequestArgs {
    /**
     * List of file names for which to compute compiler errors.
     * The files will be checked in list order.
     */
    files: string[];

    /**
     * Delay in milliseconds to wait before starting to compute
     * errors for the files in the file list
     */
    delay: number;
}

Instead of files being string[], we could switch over to each element being string | FileSpan or something similar.

When TypeScript receives a span, it can find a more appropriate span to start/end checking on. Errors are typically emitted as events that are defined through these types:

export interface DiagnosticEventBody {
    /**
     * The file for which diagnostic information is reported.
     */
    file: string;

    /**
     * An array of diagnostic information items.
     */
    diagnostics: Diagnostic[];
}

export type DiagnosticEventKind = "semanticDiag" | "syntaxDiag" | "suggestionDiag";

/**
 * Event message for DiagnosticEventKind event types.
 * These events provide syntactic and semantic errors for a file.
 */
export interface DiagnosticEvent extends Event {
    body?: DiagnosticEventBody;
    event: DiagnosticEventKind;
}

and these events types can be adjusted to indicate the following information:

  • whether the event is a region-based diagnostic event, and
  • what the adjusted region for a diagnostic request was

This information MUST be provided so that editors

  • do not clear out all the diagnostics
  • know what to adjust in case the user scrolls, or TypeScript has adjusted the span for whatever reason

Risks

There are some risks associated with this approach.

Checking Inconsistencies

First, out-of-order type-checking is already something that can cause inconsistencies between tsc and the editor. This could get worse. However, we removed the separate diagnostics-producing type-checker from the one used by the rest of the language service 2 years ago (#36747), so completions, quick info, etc. can already cause this. Since then, the problems have mostly smoothed out over time.

The other concern is that diagnostics in a region are not always caused by the code within that same region. Diagnostics can be placed wherever they choose. The risk here is that if type-checking the code within a viewport does not create the currently-visible errors, then a user will end up with flashing diagnostics as they type. Basically:

  1. A user types some code
  2. TypeScript comes back with diagnostics for the viewport
  3. There are no diagnostics due to the current viewport, so the editor clears out all the diagnostics
  4. TypeScript then comes back with diagnostics for the entire file, and the errors re-appear

This could get aggravating.

For the most part, we do not issue errors in such a non-local fashion, and strive to keep most errors close to the relevant code. The exception to this might be conflicting declarations that are duplicates. Otherwise, most non-local diagnostics are related spans. If there are any other errors that are not co-located with their code, we should probably try to relocate them to improve the experience.

Chattiness

In some cases, it may be unnecessary to check only a specific region. For some files, we might be better off just checking the entire file. This is why TypeScript should reserve the right to decide on an appropriate span, or to even report diagnostics only for a specific span.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Domain: Error Messages The issue relates to error messaging labels Feb 13, 2024
@DanielRosenwasser DanielRosenwasser changed the title Region-Prioritized Error Checking Region-Prioritized Error Checking for Editors Feb 13, 2024
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.5.0 milestone Feb 13, 2024
@DanielRosenwasser
Copy link
Member Author

@sandersn points out that maybe this behavior should be triggered based on past behavior - for example, if the last update took a while, that's a hint that you want a partial update in the future.

@DanielRosenwasser
Copy link
Member Author

@MariaSolOs points out that this experience could be a bit jarring if errors moved in and out of the error list.

@DanielRosenwasser
Copy link
Member Author

@RyanCavanaugh points out that if TSServer had no information, it would probably form a heuristic around current errors in the file - though by the time TypeScript has gotten a request for errors on a given file, it's lost the errors from the last check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Error Messages The issue relates to error messaging Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
3 participants