DiagnosticConsoleWriter reads from incorrect file paths in tests that use TestFileSystem

[d-ronnqvist]

DiagnosticConsoleWriter reads from incorrect file paths in tests that use TestFileSystem

[arthurcro]

@arthurcro is this something that you want to work on (changing to the DiagnosticConsoleWriter initializer and adding support for a TestFileSystem to the default formatter)?

@d-ronnqvist Yes, I would be happy to look into this! I'll start on it shortly, I'll post my questions and findings here!

[d-ronnqvist]

2. Use different (public) option types for each formatter

@d-ronnqvist could you elaborate on this idea? I want to confirm I understand this correctly.

Originally I was thinking that we could introduce a DiagnosticFormattingOption protocol. However, there are issues with using that as an initializer parameter. More thought about this below.

---

After thinking about this some more and experimenting a bit with this code I have a couple of thoughts:

• The DiagnosticConsoleWriter does very little general work.
• All the alternative use a lot of abstractions to generalize an initializer.

All the if formattingOptions.contains(.formatConsoleOutputForTools) checks in the console writer makes me feel like there's two different implementations in the same type that should probably be solved with some form of polymorphism. That the console writer needs to hold on the formatting options to make decision about how it uses the formatter also makes me feel like the formatting options and the formatter are too tightly coupled.

If I were to name what these formatting options checks are doing I would say that they check if the formatter is "streaming" (writing the formatter problem descriptions as they are received) or "buffered" (holding on to any received problems until the console writer is "finalized" (which #664 renames to "flush")).

The coupling between the formatting options and the formatter could be addressed by adding a isStreaming read-only property to the formatter but the console writer would still be doing both streaming and buffering in the same implementation. We could split that into StreamingDiagnosticConsoleWriter and BufferedDiagnosticConsoleWriter but both of those implementations would be very simple and are probably not worth exposing publicly. We'd still need some common interface for both console writers. That could be a base class but since most of its API would be abstract—to be overridden—it would probably be better implemented with a protocol.

Regarding the initialization abstractions; the issue with the current approach is that everything goes through one initializer, meaning that it needs the default URL and highlight parameters even when the IDE formatter will be used. There's also a less important goal that the caller shouldn't inspect the formatting options to know which formatter to use. Another way to phrase these goals could be:

• Each formatter's options should be separate
• There should be some way to create a console writer from a formatter option.

The first goal could be accomplished with either an enum or a protocol.

public enum DiagnosticFormatterOptions {
    case humanReadable(...)
    case ide
}

public protocol DiagnosticFormatterOptions { ... }

struct HumanReadableDiagnosticFormatterOptions: { ... }
struct IDEDiagnosticFormatterOptions: { ... }

The main difference between these two approaches is how they handle changes. Using a protocol it's easier to add new formatter options and to add new arguments to an existing formatter.

For the second goal, the most straight-forward way to create a console writer would be an initializer.

public class DiagnosticConsoleWriter {
    init(_ stream: TextOutputStream = LogHandle.standardError, formattingOptions options: DiagnosticFormattingOptions) { ... }
}

but this means that the console writer needs some way to create the formatter from the formatter options. It could be responsible for inspection the options to make this decision—like it does today—but this only knows with an enum when there's a known number of formatters

// options as an enum
switch options {
case .humanReadable(let ...):
    self.formatter = DefaultDiagnosticConsoleFormatter(...)
case .ide:
    self.formatter = IDEDiagnosticConsoleFormatter(...)
}

the same inspection with a protocol would always have an unhandled default case:

// options as a protocol
if let humanReadableFormatterOptions = options as? HumanReadableDiagnosticFormatterOptions {
    self.formatter = DefaultDiagnosticConsoleFormatter(...)
} else if let ideFormatterOptions = options as? IDEDiagnosticFormatterOptions {
    self.formatter = IDEDiagnosticConsoleFormatter(...)
} else {
    // What do we do here?
}

We could solve this issue in the console writer initializer by adding API to the formatter options protocol to create the formatter, but that would require making the the formatter protocol public—and like you already said; we haven't settled on what that API should be and aren't sure that we want to solidify the current formatter API.

public protocol DiagnosticFormatterOptions {
    ...
    func makeFormatter() -> DiagnosticConsoleFormatter
}

One way that we avoid making the formatter public is to instead create the console writer directly from the options

public protocol DiagnosticFormatterOptions {
    ...
    func makeFormatter(stream: TextOutputStream) -> DiagnosticConsoleWriter
}

This would require adding an internal console writer initializer that takes a DiagnosticConsoleFormatter argument.

One limitation with this approach is that DiagnosticFormatterOptions types can only be defined within the SwiftDocC module (otherwise they wouldn't have access to this initializer). This limitation is probably fine since the console writer implementation is fairly simple and another developer could define their own type that conform to DiagnosticConsumer if they wanted to integrate with DocC but use a custom formatter for diagnostics.

---

There are many different ways that we can put these pieces together. We could use an internal StreamingDiagnosticConsoleWriter that uses the DefaultDiagnosticConsoleFormatter, we could use a single DiagnosticConsoleWriter that changes its behavior based on formatter.isStreaming, or we could combine the console writer and formatter pairs into internal DefaultDiagnosticConsoleWriter and IDEDiagnosticConsoleFormatter classes.

Maintaining source backwards compatibility is going to make some of these things harder to implement. We can't change a struct or a class to a protocol so we can't use the DiagnosticConsoleWriter or DiagnosticFormattingOptions as protocol names. Renaming Console to OutputStream avoid the first naming issue. For the options name; if creating the writer is part of the API then we could include OutputStream in the options name as well.

---

I said in the beginning that "[...] the caller shouldn't inspect the formatting options" is a less important goal.

If we're fine with the caller checking if the user passed formatConsoleOutputForTools then each formatter could add an initializer or a static "factory" method directly to the DiagnosticConsoleWriter and we wouldn't really need a formatting options type to group each formatter's configuration.

if formattingOptions.contains(.formatConsoleOutputForTools) {
    diagnosticEngine.add(
        DiagnosticConsoleWriter.tools()
    )
} else {
    diagnosticEngine.add(
        DiagnosticConsoleWriter.humanReadable(baseURL: documentationBundleURL ?? URL(fileURLWithPath: fileManager.currentDirectoryPath))
    )
}

[d-ronnqvist]

@arthurcro is this something that you want to work on (changing to the DiagnosticConsoleWriter initializer and adding support for a TestFileSystem to the default formatter)?

[arthurcro]

Hey! 😄 I just started looking at those changes. I would like to start discussions here around revising the DiagnosticConsoleWriter initiliazer.

Looking at this comment, there are a couple of suggested ways to fix it.

1. Take the formatter as an argument

We pass a DiagnosticConsoleFormatter to the DiagnosticConsoleWriter initiliazer.

This forces us to make the DiagnosticConsoleFormatter protocol public. However, it's API is not refined enough yet so it might be too early to do that.

2. Use different (public) option types for each formatter

@d-ronnqvist could you elaborate on this idea? I want to confirm I understand this correctly.

My understanding is that we could map DiagnosticFormattingOptions into seperate new public types specific for each formatter. Something like:

public enum DiagnosticConsoleFormattingOption {
  case humanReadable(
    DiagnosticConsoleHumanReadableFormattingOptions,
    DiagnosticConsoleHumanReadableFormattingConfiguration
  )
  case tools(
    DiagnosticConsoleToolsFormattingOptions
  )
}

and pass that to the DiagnosticConsoleWriter initializer.

However, that would mean we would need to update the DiagnosticFormattingConsumer protocol.

3. Remove the formatter abstraction in favor of DiagnosticFormattingConsumer

Rather than instantiating different DiagnosticConsoleFormatter, we could remove this abstraction completely and rely on DiagnosticFormattingConsumer.

This makes sense to me. I also noticed DiagnosticConsoleWriter has some logic and properties that are used only when the default formatter is used. I have also noticed that DiagnosticConsoleFormatter is never used without a DiagnosticFormattingConsumer. Finally, DiagnosticConsoleFormatter is passed all the formatting options which might not be what we want since some might not be relevant for a specific formatter.

However, again we might have to update the DiagnosticFormattingConsumer protocol to make sure a DiagnosticFormattingConsumer only deals with the formatting options it cares about.

All those approaches seems to me that they will introduce breaking API changes.
I will start trying out the different approaches but I wanted to first resume this discussion from where we left it here.

Sorry if that's a bit messy and still very abstract, I am hoping we can clarify how we want implement those improvements soon!
As always, thanks for all the support! 🙂