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

Handle TypeScript Diagnostics like V8 Syntax errors in Rust #1738

Closed
kitsonk opened this issue Feb 11, 2019 · 5 comments · Fixed by #2445
Closed

Handle TypeScript Diagnostics like V8 Syntax errors in Rust #1738

kitsonk opened this issue Feb 11, 2019 · 5 comments · Fixed by #2445

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Feb 11, 2019

Opening this for tracking purposes, but I will be working on it. In a chat with Ry, he pointed out that TypeScript Diagnostics are very similar to the structure of V8 Syntax errors. It makes sense to further abstract the compiler from the rest of Deno, that instead of the compiler.ts logging out the Diagnostics returned when compiling a module and exiting, that the diagnostics are passed back to Rust and Rust figures out what to do (which would be log them out and exit).

This means that the compiler would return something like this:

interface CompileResponse {
  outputCode: string;
  sourceMap?: string;
  diagnostics?: Diagnostic[];
}

This would make it easier in the longer term to provide a cleaner compiler API that would allow easy implementation in the runtime of other compilers.

@ry
Copy link
Member

ry commented Feb 19, 2019

Note that #1793 matches colors with JSError and TypeScript errors. This will be unnecessary to conform to if we can shoehorn TS diagnostics into the JSError format.

@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 25, 2019

While they are related, a JSError and a TypeScript Diagnostic are not compatible.

The interface for JSError would be:

interface JSError {
  message: string;
  sourceLine: string;
  scriptResourceName: string;
  lineNumber: number;
  startPosition: number;
  endPosition: number;
  errorLevel: number;
  startColumn: number;
  endColumn: number;
  frames: JSFrame[];
}

interface JSFrame {
  line: number;
  column: number;
  functionName: string;
  scriptName: string;
  isEval: boolean;
  isConstructor: boolean;
  isWasm: boolean;
}

While a Diagnostic is a bit different and would look something like this:

interface Diagnostic {
  category: DiagnosticCategory;
  code: number;
  line: number;
  scriptName: string | undefined;
  startPosition: number | undefined;
  endPosition: number | undefined;
  message: string | DiagnosticMessageChain;
}

interface DiagnosticMessageChain {
  message: string;
  category: DiagnosticCategory;
  code: number;
  next?: DiagnosticMessageChain;
}

enum DiagnosticCategory {
  Warning = 0,
  Error = 1,
  Suggestion = 2,
  Message = 3
}

@ry
Copy link
Member

ry commented Feb 25, 2019

So the main difference here is that JSError has Frames and Diagnostics are a list of errors.

  1. Some errors from V8 do not include stack frames - like syntax errors. So we can safely ignore JSFrame.

  2. It seems we could map JSError[] <--> DiagnosticMessageChain

  3. There is also a "category" for JSErrors which I haven't exposed (known as ErrorLevel in V8) https://github.com/v8/v8/blob/b152bb75f880366cc0a015de30d08a84a52de904/include/v8.h#L1784-L1787

@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 26, 2019

I assume that the ErrorLevel value is reflected in this enum? It sort of aligns but is still different. In particular there is the Suggestion diagnostic, which never actually manifests in a console output, as it is a class of diagnostics used with the language service to allow things like code refactoring suggestions.

Based on everything I have merged together something that I think everything can conform to:

interface DenoDiagnostic {
  /** A string message or an ordered array of further diagnostics */
  message: string | DenoDiagnostic[];

  /** The text of the source line related to the diagnostic */
  sourceLine?: string;

  /** The line number that is related to the diagnostic */
  lineNumber?: number;

  /** The name of the script resource related to the diagnostic */
  scriptResourceName?: string;

  /** The start position related to the diagnostic */
  startPosition?: number;

  /** The end position related to the diagnostic */
  endPosition?: number;

  /** The category of the diagnostic */
  category: DenoDiagnosticCategory;

  /** A number identifier */
  code?: number;

  /** The origin of the diagnostic */
  source?: DenoDiagnosticSources;

  /** The the start column of the sourceLine related to the diagnostic */
  startColumn?: number;

  /** The end column of the sourceLine related to the diagnositc */
  endColumn?: number;

  /** Any frames of a stack trace related to the diagnostic */
  frames?: DenoDiagnosticFrame[];
}

enum DenoDiagnosticCategory {
  Log = 0,
  Debug = 1,
  Info = 2,
  Error = 3,
  Warning = 4,
  Suggestion = 5
}

enum DenoDiagnosticSources {
  V8 = 0,
  Rust = 1,
  TypeScript = 2,
  Runtime = 3
}

interface DenoDiagnosticFrame {
  line: number;
  column: number;
  functionName: string;
  scriptName: string;
  isEval: boolean;
  isConstructor: boolean;
  isWasm: boolean;
}

I've added a source, which while not used at the moment could be important information, especially if we have a user land compiler APIs.

@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 26, 2019

I suspect the ErrorLevel is a value based on this enum? If that is the case it sort of aligns, but is a bit different.

Based on all of this, I think this is an interface we can live with and conform everything to:

interface DenoDiagnostic {
  /** A string message or an ordered array of further diagnostics */
  message: string | DenoDiagnostic[];

  /** The text of the source line related to the diagnostic */
  sourceLine?: string;

  /** The line number that is related to the diagnostic */
  lineNumber?: number;

  /** The name of the script resource related to the diagnostic */
  scriptResourceName?: string;

  /** The start position related to the diagnostic */
  startPosition?: number;

  /** The end position related to the diagnostic */
  endPosition?: number;

  /** The category of the diagnostic */
  category: DenoDiagnosticCategory;

  /** A number identifier */
  code?: number;

  /** The origin of the diagnostic */
  source?: DenoDiagnosticSources;

  /** The the start column of the sourceLine related to the diagnostic */
  startColumn?: number;

  /** The end column of the sourceLine related to the diagnositc */
  endColumn?: number;

  /** Any frames of a stack trace related to the diagnostic */
  frames?: DenoDiagnosticFrame[];
}

enum DenoDiagnosticCategory {
  Log = 0,
  Debug = 1,
  Info = 2,
  Error = 3,
  Warning = 4,
  Suggestion = 5
}

enum DenoDiagnosticSources {
  V8 = 0,
  Rust = 1,
  TypeScript = 2,
  Runtime = 3
}

interface DenoDiagnosticFrame {
  line: number;
  column: number;
  functionName: string;
  scriptName: string;
  isEval: boolean;
  isConstructor: boolean;
  isWasm: boolean;
}

In particular it has the source of the error which will be important long term, especially if we have a user land compiler API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants