Enforce error diagnostics by aborting execution

[czechboy0]

          I vote we keep it; maybe even rename it fatal. WDYT? Could you open an issue and we can converge on a stance? Even if we end up removing it, we can close the loop on this.

Originally posted by @simonjbeaumont in #178 (comment)

See the conversation above.

Today, we have the issue that .error diagnostics don't terminate execution. We should decide whether to remove that error severity, and simply use error throwing to propagate fatal issues, or to somehow make emitting an error diagnostic become fatal.

[czechboy0]

@simonjbeaumont If we keep it, we have to find a way to propagate the signal that an error diagnostic was emitted to the generator. I'd like to avoid using fatalError (as that produces a crash log), and ideally have something testable (so exit(1) also isn't great).

But I wonder if we're over-engineering it. Essentially, the place that emits the error diagnostic wants to terminate execution anyway, which is what error throwing is for in Swift. Otherwise we need to find a way that some code in the generator emits an error diagnostic, which gets added to the diagnostic collector, and the signal that it was fatal is propagated back to the caller. Maybe the call to emit can just throw if the severity is .error? That'd work, I think.

[simonjbeaumont]

Maybe the call to emit can just throw if the severity is .error? That'd work, I think.

That works for me.

[czechboy0]

Ugh, I forgot that emit isn't throwing today. We can change that though.

[arthurcro]

Hey! 👋 I'd like to take a look at implementing this if it's available! 😊

[czechboy0]

@arthurcro, sure, go ahead! 🙏

[arthurcro]

Hi! 👋 I had a chance to look at implementing this change.
If my understanding is correct we want to change DiagnosticCollector.emit(_:) to be throwing. I am wondering if this is the best approach to propagate fatal issues.

First, changing the DiagnosticCollector to:

public protocol DiagnosticCollector {
  /// Submits a diagnostic to the collector.
  /// - Parameter diagnostic: The diagnostic to submit.
  /// - Throws: Throws when the diagnostic severity is `error`.
  func emit(_ diagnostic: Diagnostic) throws
}

would be an API breaknig chnge.

Further, although it would be mentioned in the documentation of DiagnosticCollector there is no way to enforce that a error diagnostic would make all the implementations of DiagnosticCollector actually throw an error which mght be misleading.

Lastly, when the caller wants to emit a diagnostic, having to try to emit the diagnostic implies that emiting could fail. However, that's not always true, a diagnsotic collector implementation could log the error diagnoctic, so succeed at recieving the error and then throw.

It feels to me like emiting a diagnostic and throwing a fatal diagnsotic should be handled seperately. Maybe that's the way we want to go and I'm not sure what a better alternative would be but I wanted to share my thoughts here to resume the discussion.

[czechboy0]

Actually, another idea that should address these issues.

Create a new concrete reference type DiagnosticSubmitter, which holds a reference to a DiagnosticCollector.

final class DiagnosticSubmitter {
  private let collector: any DiagnosticCollector
  init(collector: any DiagnosticCollector) {
    self.collector = collector
  }

  func submit(_ diagnostic: Diagnostic) throws {
    collector.emit(diagnostic)
    if diagnostic.severity == .error {
      throw diagnostic
    }
  }
}

And then move the utility functions like emitUnsupported... onto the submitter, as submitUnsupported....

This way, we can enforce the unified behavior without asking all collectors to manually implement it. What do you think?

[arthurcro]

This makes a lot of sense to me! I'll try it out, thank you for the great suggestion! 🙂