[Proposal] Updates to Problem to enable richer reporting

[ckipp01]

Hello! I'd like to suggest some changes to the current Problem interface, and get some consensus on the best way to move forward before I start hacking stuff in. The larger context to this proposal can be found in the contributors forum, but I'll included a brief summary down below.

Summary

Much of the Scala ecosystem is relying on the same mechanism to report errors from Dotty, and much of the boils down to implementing the CompilerInterface2, which includes a Reporter, which includes some Problems. Many of the build tools use this Problem as the core way that information about a diagnostic is passed around. For example you can see examples of this in Bloop, Mill, and it's of course used all over in Zinc and sbt. The current structure of the Problem looks like this:

Current interface

public interface Problem {
  String category();
  Severity severity();
  String message();
  Position position();

  // Default value to avoid breaking binary compatibility
  /**
   * If present, the string shown to the user when displaying this Problem.
   * Otherwise, the Problem will be shown in an implementation-defined way
   * based on the values of its other fields.
   */
  default Optional<String> rendered() { return Optional.empty(); }
}

While this indeed is the core information needed, there is also extra information that the compiler knows that I believe we could add similar to how rendered was added in a way that wouldn't break compatibility with CompilerInterface2 or Problem. The minimal things that could really help editor-like tooling would be if the ErrorMessageID could be exposed allowing for tools to easily know the error that they're working with to potentially then offer code actions or auto-fixes, and then also a structured relationship of "related" information to a diagnostic. For example in BSP there is already a field in their Diagnostic, DiagnosticRelatedInformation modeled after the LSP version, which can also contain a Position and a message. There are scenarios in Dotty already where being able to expose this in a structured way would avoid having to render multiple positions in a string to show to the user. For example.

This in the current format

vs what it could be like if it was given to your editor structured via DiagnosticRelatedInformation

Therefore I'd like to propose something similar to the following:

Suggested interface

public interface Problem {
  String category();
  Severity severity();
  String message();
  Position position();

  // Default value to avoid breaking binary compatibility
  /**
   * If present, the string shown to the user when displaying this Problem.
   * Otherwise, the Problem will be shown in an implementation-defined way
   * based on the values of its other fields.
   */
  default Optional<String> rendered() { return Optional.empty(); }

  default Optional<DiagnosticCode> diagnosticCode() { return Optional.empty(); }

  default List<DiagnosticRelatedInformation> diagnosticReleatedInforamation() { return List.empty(); }
}

public interface DiagnosticCode {
  String code();
  Optional<String> explanation();
}

public interface DiagnosticRelatedInformation {
  Position position();
  String message();
}

I believe the best way to add them to Problem would probably be with defaults to ensure this won't break when using against old versions of the compiler that don't report this. If I'm following the entire chain correctly and if we were to move in this direction the change would first need to be made in Problem and then everything else could start using it after Dotty then adds this information to the Problem in it's Reporter.

Any thoughts on this, potential issues you see, or other suggestions? If you're happy with this, I'd be happy to start adding this in.

[ckipp01]

One other thing I'm thinking about is what exactly to do with DiagnosticCode. I modeled the version you see up above off the DiagnosticCode from Rust, but looking at the LSP spec their CodeDescription is a URI. I can see how both might be handy, especially if we have something like their Error Index that we could like to. I'm working on that as well. However then that'd need to be a stable unchanged location.

[eed3si9n]

As long as we can maintain forward and backward compatibility, I think it would be great to get necessary data in to Problem.

One thing I'd watch out for is the potential for increased memory, storage, and network usage whenever we add information to something that's used fairly common.

UpdateReport

At some point in 2014 I realized that sbt was throwing away the details of dependency resolution like license and caller info, since they aren't needed to calculate classpath. I thought we can mine interesting thing about of that and expanded it in 855e7f1 as part of a PR to implement eviction report. As it turned out, eviction warning was kind of interesting, but not all that effective, but the performance penalty of persisting ginormous JSON of UpdateReport for hundreds of JARs was real. Eventually in 2019 I added updateFull (#4606) that contains extra information, but update went back to the slimmer data.

The larger the graph, the effect would be more pronounced. For example, I saw a graph reduce from 5.9MB to 1.1MB in JSON file.

File (absolute path) vs VirtualFileRef

Similarly for Zinc, we use to represent all file information using java.io.File, which contains absolute paths. It seems trivial, but since most of the information Zinc needs to persist involves files, containing sometimes-long absolute path bloated both the storage and memory representation. We tried to address that also in 2019 using VirtualFileRef.

back to Diagnostic

It might be useful to estimate the performance impact. I think there's some cap (100?) warnings we display per subproject, but for the purpose of talking to Metals etc, I don't think there's good reason to cut that off since it has a better UX to consume compiler warnings/errors.

For the purpose of incremental compilation Analysis is often read from disk and kept in memory. We can hopefully keep a unique set of DiagnosticCode and reference them, but I don't know how much data diagnosticReleatedInforamation would add. If it becomes and issue, we'd deal with it, but something to keep an eye out.

[ckipp01]

Thanks for the insight! I'll be honest, I didn't really consider the performance overhead, but I guess if we're talking very large projects this becomes much more relevant.

It might be useful to estimate the performance impact.

So just thinking a bit about it a bit, I'm assuming to start out the thing that will be utilized most is literally just the code itself. At least right now in Dotty there is no description or message that exists with the code unless we'd want to move the explain text to the DiagnosticCode.message. I just realize now, there is probably a performance hit then when using -explain. I never thought about that, but the messages at times are significantly larger, more so than adding a code field probably. There are only 2 instances that exist this far (that I know of) in Dotty where the diagnosticRelatedInformation would be used, but once added there is probably the potential to then add more. Either way, I'll keep this in mind. When I start playing around with this, I may ping you for some insight on the best way to actually measure the performance impact.

[adpi2]

I like the proposal. It seems you scratched the thing quite far already and that you know what you are doing. Don't hesitate to ping me if you need some review, testing or anything.

[ckipp01]

Great, thanks @adpi2. Are you able to allow CI to run for #6874?

[adpi2]

It's running!