RFC: Making the "node" and "position" of Diagnostic and Note optional

Development Swift Syntax
1

  • Link to PR: [Diagnostics] Make the node and position of Diagnostic and Note optional by DougGregor · Pull Request #3038 · swiftlang/swift-syntax · GitHub

  • Summary: The Diagnostic and Note types require a syntax node where the diagnostic will be emitted. Some clients that use (or want to use) the diagnostics machinery don't have those syntax nodes, because they might not be dealing directly with source code. For example, the compiler driver and compiler can emit diagnostics about bad command-line options, which means that can't make use of any of the diagnostics machinery from swift-syntax.

  • API Changes: The node and position properties of Diagnostic and Note change from

    public let node: Syntax
public let position: AbsolutePosition

    to

    public let node: Syntax?
public let position: AbsolutePosition?

    There is a corresponding change in the initializers to take a (some SyntaxProtocol)? (for Diagnostic) or Syntax? (for Note).

  • Migration: Clients that consume Diagnostic or Note instances will probably be looking at the node, and therefore will have to deal with the fact that it becomes optional. See the swift-syntax pull request or this compiler pull request for examples of how this impacts some clients.

I don't know if this change is the last word on the generalization of location information in Diagnostic and Note. I think there's a reasonable case for adding the ability to provide a SourceLocation directly to the diagnostic, instead of "no information", for cases where we were provided with filename/line/column but don't have access to the source code. (Or maybe it's not Swift code). In such cases, node and position would still need to be optional, but the internal storage would be some switch over a node or source location.

Doug

2

I am a little worried about the source breakage this might cause and also about ambiguities about diagnostics that have node == nil && position != nil or node != nil && position == nil – I don’t know what that should signify and the diagnostic formatter also seems to be inconsistent about this.

Maybe we need to introduce another type like DiagnosticLocation to wrap node and position. That would also have the advantage that we could keep node and position as deprecated non-optional compatibility accessors.

Something like

enum DiagnosticLocation {
  /// The diagnostic is anchored at the start of the given syntax node.
  case startOfNode(Syntax)
  /// The diagnostic is anchored at the given position within the syntax tree that contains the given node.
  case position(AbsolutePosition, inTreeOf: Syntax)
  /// The diagnostic does not have a source location associated with it.
  case unknown

  /// The node at whose start location the message should be displayed.
  public var node: Syntax? { … }

  /// The position at which the location should be anchored.
  public var position: AbsolutePosition? { … }
}
3

Yeah, I've been liking my design less and less. I think I'm going to abandon it in favor of a different approach, where diagnostics that don't have source-specific information should just be DiagnosticMessage-conforming types. Let's consider my proposal retracted.

Doug