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

[Feature request] Provide linting utilities for third party programs #901

Open
Sarrus1 opened this issue Oct 1, 2023 · 2 comments
Open
Labels
compiler Problems with the compiler parsing source code. docgen

Comments

@Sarrus1
Copy link

Sarrus1 commented Oct 1, 2023

Context

In order to edit plugins, a large part of the community uses a Language Server Protocol implementation for Sourcepawn, which provides completions, gotodefinition, syntax highlighting, etc. You can find the project here.

The LSP also provides diagnostics, such as syntax errors, missing includes, and undefined variables.
It also invokes spcomp everytime the user saves a file to run a blank compilation on the project, parses the output for eventual warnings and errors, and adapts them to the LSP format.

Problem

  • For large plugins, compile time can take several seconds, which delays the diagnostics for the user (the --syntax-only flag partially fixes that issue).
  • Diagnostics are only line based for now, having the column of the error as well would allow for more user friendly diagnostics.
  • There is no granularity in how diagnostics are checked. Suppose the user changes something in one file. The whole project has to be compiled to detect errors. Having the option to only check one file, or have incremental diagnostics would be nice.

Requests

  • Having an output that can be parsed easily, maybe a JSON output to stdout/err ?
  • Add column indexes to errors and warnings.
  • Being able to lint only one file instead of the whole project, OR having the ability to tell the compiler which file changed and update the diagnostics from that.
  • Being able to feed spcomp "Virtual files", as in, when the user edits their file in the editor, but does not save, be able to pass the modified file to spcomp without having to write it to disk.

Lots of requests, please let me know if I should clarify some!

@dvander
Copy link
Member

dvander commented Oct 1, 2023

For large plugins - can you send/link/attach an example? We definitely want spcomp to be lightning fast, so benchmarks are important.

Diagnostics:

  • Please propose an error format that would be ideal, and I'll add it in. We have column information now (Add beautiful error messages. #900)!
  • Please propose a JSON format as well. Happy to add this.
  • Need more information on "virtual" files. Is it a ... temporary file? Is it a stream?
  • Incremental would be nice but there are a few tricky issues that need to be resolved first. Eg, if you add a #define to the top of your file, every #include has to be reparsed. No way around this. We'd need to sanitize all #includes to make sure they do not depend on macros.

@peace-maker peace-maker added docgen compiler Problems with the compiler parsing source code. labels Oct 7, 2023
@Sarrus1
Copy link
Author

Sarrus1 commented Oct 9, 2023

Please propose a JSON format as well. Happy to add this.

I have formalized a JSON format as a typescript declaration. Please find it below. I have added a lot of optional information on purpose so that the format is as future-proof as possible.

This might be too much, the important part are the root level attributes of DiagnosticMessage (i.e: message, code, level and ranges).

Need more information on "virtual" files.

I have tried to look into how that could be implemented, but I don't think it would work/be useful, whatever the way it is implemented. The reason is that my vscode extension is aware of the changes the user makes to a file, even if the file has not been saved yet. It would have been nice to be able to tell spcomp "the file at path x has been edited, if you ever encounter this file, do not read it from disk, use this text instead". However, I don't think there is a sane way to do this apart from passing the file's content as a command line argument to spcomp, which does not sound ideal...

every #include has to be reparsed

Yes, that's one of the problems I came across when writing my preprocessor. My solution was to keep track of the includes where the #define was used, and only refresh them. But that entails other problems... I don't think this would be easy to use anyways. We can ignore this idea 👍

type DiagnosticMessage = {
  /** The primary message. */
  message: string;
  /**
   * The diagnostic code.
   * Some messages may set this value to null.
   */
  code: {
    /** A unique string identifying which diagnostic triggered. */
    code: string;
    /** An optional string explaining more detail about the diagnostic code. */
    explanation: string | null;
  } | null;
  /**
   * The severity of the diagnostic.
   * Values may be:
   * - "error": A fatal error that prevents compilation.
   * - "warning": A possible error or concern.
   * - "note": Additional information or context about the diagnostic.
   * - "help": A suggestion on how to resolve the diagnostic.
   * - "failure-note": A note attached to the message for further information.
   * - "error: internal compiler error": Indicates a bug within the compiler.
   */
  level:
    | "error"
    | "warning"
    | "note"
    | "help"
    | "failure-note"
    | "error: internal compiler error";
  /**
   * An array of source code locations to point out specific details about
   * where the diagnostic originates from. This may be empty, for example
   * for some global messages, or child messages attached to a parent.
   *
   * Character offsets are offsets of Unicode Scalar Values.
   */
  ranges: Array<{
    /**
     * The file where the range is located.
     * Note that this path may not exist. For example, if the path
     * points to the standard library, and the rust src is not
     * available in the sysroot, then it may point to a nonexistent
     * file. Beware that this may also point to the source of an
     * external crate.
     */
    file_path: string;
    /** The byte offset where the range starts (0-based, inclusive). */
    byte_start: number;
    /** The byte offset where the range ends (0-based, exclusive). */
    byte_end: number;
    /** The first line number of the range (1-based, inclusive). */
    line_start: number;
    /** The last line number of the range (1-based, inclusive). */
    line_end: number;
    /** The first character offset of the line_start (1-based, inclusive). */
    column_start: number;
    /** The last character offset of the line_end (1-based, exclusive). */
    column_end: number;
    /**
     * Whether or not this is the "primary" range.
     *
     * This indicates that this range is the focal point of the
     * diagnostic.
     *
     * There are rare cases where multiple ranges may be marked as
     * primary. For example, "immutable borrow occurs here" and
     * "mutable borrow ends here" can be two separate primary ranges.
     *
     * The top (parent) message should always have at least one
     * primary range, unless it has zero ranges. Child messages may have
     * zero or more primary ranges.
     */
    is_primary: boolean;
    /**
     * An array of objects showing the original source code for this
     * range. This shows the entire lines of text where the range is
     * located. A range across multiple lines will have a separate
     * value for each line.
     */
    text: Array<{
      /** The entire line of the original source code. */
      text: string;
      /**
       * The first character offset of the line of
       * where the range covers this line (1-based, inclusive).
       */
      highlight_start: number;
      /**
       * The last character offset of the line of
       * where the range covers this line (1-based, exclusive).
       */
      highlight_end: number;
    }>;
    /**
     * An optional message to display at this range location.
     * This is typically null for primary ranges.
     */
    label: string | null;
    /**
     * An optional string of a suggested replacement for this range to
     * solve the issue. Tools may try to replace the contents of the
     * range with this text.
     */
    suggested_replacement: string | null;
    /**
     * An optional string that indicates the confidence of the
     * "suggested_replacement". Tools may use this value to determine
     * whether or not suggestions should be automatically applied.
     *
     * Possible values may be:
     * - "MachineApplicable": The suggestion is definitely what the
     *   user intended. This suggestion should be automatically
     *   applied.
     * - "MaybeIncorrect": The suggestion may be what the user
     *   intended, but it is uncertain. The suggestion should result
     *   in valid Rust code if it is applied.
     * - "HasPlaceholders": The suggestion contains placeholders like
     *   `(...)`. The suggestion cannot be applied automatically
     *   because it will not result in valid Rust code. The user will
     *   need to fill in the placeholders.
     * - "Unspecified": The applicability of the suggestion is unknown.
     */
    suggestion_applicability:
      | "MachineApplicable"
      | "MaybeIncorrect"
      | "HasPlaceholders"
      | "Unspecified"
      | null;
    /**
     * An optional object indicating the expansion of a macro within
     * this range.
     *
     * If a message occurs within a macro invocation, this object will
     * provide details of where within the macro expansion the message
     * is located.
     */
    expansion: {
      /**
       * The range of the macro invocation.
       * Uses the same range definition as the "ranges" array.
       */
      range: {
        file_name: string;
        byte_start: number;
        byte_end: number;
        line_start: number;
        line_end: number;
        column_start: number;
        column_end: number;
      };
      /** Name of the macro. */
      macro_decl_name: string;
      /**
       * Optional range where the relevant part of the macro is
       * defined.
       */
      def_site_range: {
        file_name: string;
        byte_start: number;
        byte_end: number;
        line_start: number;
        line_end: number;
        column_start: number;
        column_end: number;
      } | null;
    } | null;
  }>;
  /**
   * Array of attached diagnostic messages.
   * This is an array of objects using the same format as the parent
   * message. Children are not nested (children do not themselves
   * contain "children" definitions).
   */
  children: DiagnosticMessage[];
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Problems with the compiler parsing source code. docgen
Projects
None yet
Development

No branches or pull requests

3 participants