Skip to content

Add support to dart analyze for a JSON based output #45068

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

Closed
bwilkerson opened this issue Feb 21, 2021 · 14 comments
Closed

Add support to dart analyze for a JSON based output #45068

bwilkerson opened this issue Feb 21, 2021 · 14 comments
Assignees
Labels
dart-cli-analyze Issues related to the 'dart analyze' tool devexp-command Issues with the command-line dartanalyzer tool legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on

Comments

@bwilkerson
Copy link
Member

There are at least two uses cases that would benefit from dart analyze having a more extensible and more complete machine readable output format. I believe that JSON would be a good format for this purpose. The proposal is to support a new output format named "json" (as in dart analyze --format=json).

@bwilkerson bwilkerson added legacy-area-analyzer Use area-devexp instead. devexp-command Issues with the command-line dartanalyzer tool dart-cli-analyze Issues related to the 'dart analyze' tool labels Feb 21, 2021
@bwilkerson
Copy link
Member Author

@bwilkerson
Copy link
Member Author

Below is one proposal for the structure of the JSON to be produced. It's expressed as pseudo-Dart code, where an abstract class denotes a JSON map and typedefs introduce semantic names for primitive types. Comments and alternate designs are welcome, this is merely a starting point for discussion.

I will note that this object can't currently be produced from the content of either the legacy protocol nor the content of LSP. In order to support producing this format in dart analyze we will either need to enhance the legacy protocol to include the additional information or compute the missing information in dartdev.

/// The top level JSON object produced for the 'json' format.
abstract class AnalysisResults {
  /// The version of this JSON object.
  int version;

  /// The diagnostics that were reported.
  List<Diagnostic> diagnostics;
}

/// An indication of an issue in the code being analyzed.
abstract class Diagnostic {
  /// The code associated with the diagnostic.
  String code;

  /// The severity of the diagnostic.
  DiagnosticSeverity severity;

  /// The type of the diagnostic.
  ///
  /// TODO In my opinion, this information is not useful to clients, and I would
  ///  prefer to drop it from the output, but I mentioned it because it's
  ///  included in the 'machine' format.
  DiagnosticType type;

  /// The location associated with the problem message.
  Location location;

  /// A textual description of the problem represented by the diagnostic.
  String problemMessage;

  /// A textual description of how to correct the problem.
  String? correctionMessage;

  /// The severity of the diagnostic.
  List<ContextMessage>? contextMessages;

  /// The severity of the diagnostic.
  Url? documentation;
}

/// The severity of a diagnostic. One of 'error', 'warning', 'hint', 'lint', and
/// 'info'.
typedef DiagnosticSeverity = String;

/// The type of a diagnostic. One of 'compileTimeError',
/// 'checkedModeCompileTimeError', 'staticWarning', 'staticTypeWarning',
/// 'syntacticError', 'hint', 'lint', and 'toDo'.
typedef DiagnosticType = String;

/// A range of characters within a given file.
abstract class Location {
  /// The file containing the location.
  FilePath file;

  /// The range of characters representing the location.
  Range range;
}

/// The absolute and normalized path to a file.
typedef FilePath = String;

/// A range of characters in a file.
abstract class Range {
  /// The position immediately to the left of the first character in the range.
  Position start;

  /// The position immediately to the right of the last character in the range.
  Position end;
}

/// A position in a file. Positions are conceptually between two characters.
abstract class Position {
  /// The distance between the beginning of the file and the position. The
  /// position before the first character in the file has an offset of zero.
  int offset;

  /// The zero-based index of the line containing the position.
  int line;

  /// The zero-based index of the character to the right of the position. A
  /// position at the beginning of a line has a column of zero.
  int column;
}

/// Additional information associated with a diagnostic that helps to explain
/// the problem or draws the user's attention to some location that is relevant
/// to the problem.
abstract class ContextMessage {
  /// The location associated with the context message.
  Location location;

  /// A textual description of the additional information associated with the
  /// diagnostic.
  String message;
}

/// The string encoding of a URL.
typedef Url = String;

@stereotype441
Copy link
Member

This format SGTM

@devoncarew
Copy link
Member

From this:

/// The top level JSON object produced for the 'json' format.
abstract class AnalysisResults {
  /// The version of this JSON object.
  int version;

  /// The diagnostics that were reported.
  List<Diagnostic> diagnostics;
}

It looks like the initial json structure we parse will be an object, and that will contain a list of Diagnostic objects. Having the outer wrapper will allow us to version the format.

You might instead consider having the outer structure just be a json list of Diagnostic objects. There would be one less layer of json structure in the output; we'd want to be reasonable certain that we could make backwards compatible changes to the format in the future, or, use the sdk version to indicate to the user which format we produce.

Alternatively, having the outer wrapper would allow us to more easily include more information in the future (instead of just producing diagnostics, we could produce timing information and diagnostics for example).

@stereotype441
Copy link
Member

My two cents on the versioning question: I think it's better to have a top level object with a version number. It's a very small investment (overhead is a small constant), but the potential benefit is huge, because we don't know how this data structure may need to evolve in the future, and it's quite conceivable that some future improvement would be quite difficult to do in a backward-compatible way.

@stereotype441
Copy link
Member

@bwilkerson any update on this?

@bwilkerson
Copy link
Member Author

Not that I'm aware of. I think the next step is to assign someone to actually implement it. @devoncarew

@devoncarew
Copy link
Member

Brian, I'd assumed that you were driving this. If not then we might want to rethink how we're addressing #44905. If we put this in the critical path of testing the why not promoted work, we'd either have to:

  • add a json format to dart analyze and switch test.py over to using dart analyze
  • add a json format to dartanalyzer and switch test.py over to using the json format (from the machine format)

Without somebody driving both the json format changes and the test.py changes we're highly unlikely to get this done by the next stable release.

#44905 can also be satisfied by slightly extending the existing machine format. We might want to do that and decouple the json format with the why not promoted work.

@bwilkerson
Copy link
Member Author

Brian, I'd assumed that you were driving this.

Sorry. If you explicitly stated that then I missed it. I assumed that either you or Paul would be driving the work. I just offered to help by creating an initial proposal of what the protocol might look like. I can certainly do more if you want me to.

#44905 can also be satisfied by slightly extending the existing machine format.

I might be wrong, but I don't think that's true. I think we'd still need to update test.dart to recognize the extended machine format and use the additional information as part of the test support. I doubt that it would take significantly more work to convert it over to use the json format, and that approach has the advantage that the two steps (producing the new format and consuming the new format) are highly separable.

@stereotype441
Copy link
Member

Brian, I'd assumed that you were driving this.

Sorry. If you explicitly stated that then I missed it. I assumed that either you or Paul would be driving the work. I just offered to help by creating an initial proposal of what the protocol might look like. I can certainly do more if you want me to.

To avoid further confusion, I suggest we use the "assignee" field for this bug to track who is working on it. I'm assigning to Brian for now because my current understanding is that he's the one who has the best combination of time, knowledge, and interest to be successful at it. Brian/Devon, feel free to change that if you decide it's a better fit for someone else.

@davidmorgan
Copy link
Contributor

FYI we no longer rely on serialized analyzer output in google3--"analyzer as a library" makes it unnecessary--so I will not be following this work ;) thanks.

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Mar 18, 2021
@bwilkerson
Copy link
Member Author

Status update.

The analysis server now sends back all of the information required in order to produce this format.

The dart analyze command now has a json format option that produces this format.

The dartanalyzer command does not yet have this support, but it should be relatively easy to add that support. I'm hoping to get that implemented by early next week.

@stereotype441
Copy link
Member

Addressed in e4bb500.

@bwilkerson
Copy link
Member Author

Support for the dartanalyzer command was added in https://dart-review.googlesource.com/c/sdk/+/193526.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart-cli-analyze Issues related to the 'dart analyze' tool devexp-command Issues with the command-line dartanalyzer tool legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

5 participants