Skip to content

Respect #sourceLocation directives in SourceLocationConverter #1827

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

Merged
merged 2 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ let package = Package(

.testTarget(
name: "SwiftSyntaxTest",
dependencies: ["_SwiftSyntaxTestSupport", "SwiftSyntax"]
dependencies: ["_SwiftSyntaxTestSupport", "SwiftSyntax", "SwiftSyntaxBuilder"]
),

// MARK: SwiftSyntaxBuilder
Expand Down
184 changes: 159 additions & 25 deletions Sources/SwiftSyntax/SourceLocation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,40 @@
//===----------------------------------------------------------------------===//

/// Represents a source location in a Swift file.
public struct SourceLocation: Hashable, Codable, CustomDebugStringConvertible {

/// The UTF-8 byte offset into the file where this location resides.
public let offset: Int
public struct SourceLocation: Hashable, Codable {

/// The line in the file where this location resides. 1-based.
///
/// ### See also
/// ``SourceLocation/presumedLine``
public var line: Int

/// The UTF-8 byte offset from the beginning of the line where this location
/// resides. 1-based.
public let column: Int

/// The UTF-8 byte offset into the file where this location resides.
public let offset: Int

/// The file in which this location resides.
///
/// ### See also
/// ``SourceLocation/presumedFile``
public let file: String

/// Returns the location as `<line>:<column>` for debugging purposes.
/// Do not rely on this output being stable.
public var debugDescription: String {
// Print file name?
return "\(line):\(column)"
}
/// The line of this location when respecting `#sourceLocation` directives.
///
/// If the location hasn’t been adjusted using `#sourceLocation` directives,
/// this is the same as `line`.
public let presumedLine: Int

/// The file in which the the location resides when respecting `#sourceLocation`
/// directives.
///
/// If the location has been adjusted using `#sourceLocation` directives, this
/// is the file mentioned in the last `#sourceLocation` directive before this
/// location, otherwise this is the same as `file`.
public let presumedFile: String

/// Create a new source location at the specified `line` and `column` in `file`.
///
Expand All @@ -47,16 +60,31 @@ public struct SourceLocation: Hashable, Codable, CustomDebugStringConvertible {
/// location in the source file has `offset` 0.
/// - file: A string describing the name of the file in which this location
/// is contained.
public init(line: Int, column: Int, offset: Int, file: String) {
/// - presumedLine: If the location has been adjusted using `#sourceLocation`
/// directives, the adjusted line. If `nil`, this defaults to
/// `line`.
/// - presumedFile: If the location has been adjusted using `#sourceLocation`
/// directives, the adjusted file. If `nil`, this defaults to
/// `file`.
public init(
line: Int,
column: Int,
offset: Int,
file: String,
presumedLine: Int? = nil,
presumedFile: String? = nil
Comment on lines +74 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to debug description?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super happy with the debug description as-is anyway because it also doesn’t include the file name and have been tempted to just remove it. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just extend it to include file as well 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to remove debugDescription

The debug description was lossy because it didn’t print file and offset. If we include that information, it’s so verbose that the debug description doesn’t really provide any benefit over the default debug description. Let’s just remove it.

) {
self.line = line
self.offset = offset
self.column = column
self.file = file
self.presumedLine = presumedLine ?? line
self.presumedFile = presumedFile ?? file
}
}

/// Represents a half-open range in a Swift file.
public struct SourceRange: Hashable, Codable, CustomDebugStringConvertible {
public struct SourceRange: Hashable, Codable {

/// The beginning location of the source range.
///
Expand All @@ -69,12 +97,6 @@ public struct SourceRange: Hashable, Codable, CustomDebugStringConvertible {
/// ie. this location is not included in the range.
public let end: SourceLocation

/// A description describing this range for debugging purposes, don't rely on
/// it being stable
public var debugDescription: String {
return "(\(start.debugDescription),\(end.debugDescription))"
}

/// Construct a new source range, starting at `start` (inclusive) and ending
/// at `end` (exclusive).
public init(start: SourceLocation, end: SourceLocation) {
Expand All @@ -83,18 +105,85 @@ public struct SourceRange: Hashable, Codable, CustomDebugStringConvertible {
}
}

/// Collects all `PoundSourceLocationSyntax` directives in a file.
fileprivate class SourceLocationCollector: SyntaxVisitor {
private var sourceLocationDirectives: [PoundSourceLocationSyntax] = []

override func visit(_ node: PoundSourceLocationSyntax) -> SyntaxVisitorContinueKind {
sourceLocationDirectives.append(node)
return .skipChildren
}

static func collectSourceLocations(in tree: some SyntaxProtocol) -> [PoundSourceLocationSyntax] {
let collector = SourceLocationCollector(viewMode: .sourceAccurate)
collector.walk(tree)
return collector.sourceLocationDirectives
}
}

fileprivate struct SourceLocationDirectiveArguments {
enum Error: Swift.Error, CustomStringConvertible {
case nonDecimalLineNumber(TokenSyntax)
case stringInterpolationInFileName(StringLiteralExprSyntax)

var description: String {
switch self {
case .nonDecimalLineNumber(let token):
return "'\(token.text)' is not a decimal integer"
case .stringInterpolationInFileName(let stringLiteral):
return "The string literal '\(stringLiteral)' contains string interpolation, which is not allowed"
}
}
}

/// The `file` argument of the `#sourceLocation` directive.
let file: String

/// The `line` argument of the `#sourceLocation` directive.
let line: Int

init(_ args: PoundSourceLocationArgsSyntax) throws {
guard args.fileName.segments.count == 1,
case .stringSegment(let segment) = args.fileName.segments.first!
else {
throw Error.stringInterpolationInFileName(args.fileName)
}
self.file = segment.content.text
guard let line = Int(args.lineNumber.text) else {
throw Error.nonDecimalLineNumber(args.lineNumber)
}
self.line = line
}
}

/// Converts ``AbsolutePosition``s of syntax nodes to ``SourceLocation``s, and
/// vice-versa. The ``AbsolutePosition``s must be originating from nodes that are
/// part of the same tree that was used to initialize this class.
public final class SourceLocationConverter {
let file: String
private let file: String
/// The source of the file, modelled as data so it can contain invalid UTF-8.
let source: [UInt8]
private let source: [UInt8]
/// Array of lines and the position at the start of the line.
let lines: [AbsolutePosition]
private let lines: [AbsolutePosition]
/// Position at end of file.
let endOfFile: AbsolutePosition
private let endOfFile: AbsolutePosition

/// The information from all `#sourceLocation` directives in the file
/// necessary to compute presumed locations.
///
/// - `sourceLine` is the line at which the `#sourceLocation` statement occurs
/// within the current file.
/// - `arguments` are the `file` and `line` arguments of the directive or `nil`
/// if spelled as `#sourceLocation()` to reset the source location directive.
private var sourceLocationDirectives: [(sourceLine: Int, arguments: SourceLocationDirectiveArguments?)] = []

/// Create a new ``SourceLocationConverter`` to convert betwen ``AbsolutePosition``
/// and ``SourceLocation`` in a syntax tree.
///
/// This converter ignores any malformed `#sourceLocation` directives, e.g.
/// `#sourceLocation` directives with a non-decimal line number or with a file
/// name that contains string interpolation.
///
/// - Parameters:
/// - file: The file path associated with the syntax tree.
/// - tree: The root of the syntax tree to convert positions to line/columns for.
Expand All @@ -104,11 +193,29 @@ public final class SourceLocationConverter {
self.source = tree.syntaxTextBytes
(self.lines, endOfFile) = computeLines(tree: Syntax(tree))
precondition(tree.byteSize == endOfFile.utf8Offset)

for directive in SourceLocationCollector.collectSourceLocations(in: tree) {
let location = self.physicalLocation(for: directive.positionAfterSkippingLeadingTrivia)
if let args = directive.args {
if let parsedArgs = try? SourceLocationDirectiveArguments(args) {
// Ignore any malformed `#sourceLocation` directives.
sourceLocationDirectives.append((sourceLine: location.line, arguments: parsedArgs))
}
} else {
// `#sourceLocation()` without any arguments resets the `#sourceLocation` directive.
sourceLocationDirectives.append((sourceLine: location.line, arguments: nil))
}
}
}

/// - Important: This initializer does not take `#sourceLocation` directives
/// into account and doesn’t produce `presumedFile` and
/// `presumedLine`.
///
/// - Parameters:
/// - file: The file path associated with the syntax tree.
/// - source: The source code to convert positions to line/columns for.
@available(*, deprecated, message: "Use init(file:tree:) instead")
public init(file: String, source: String) {
self.file = file
self.source = Array(source.utf8)
Expand Down Expand Up @@ -145,13 +252,40 @@ public final class SourceLocationConverter {
}
}

/// Convert a ``AbsolutePosition`` to a ``SourceLocation``. If the position is
/// Convert a ``AbsolutePosition`` to a ``SourceLocation``.
///
/// If the position is exceeding the file length then the ``SourceLocation``
/// for the end of file is returned. If position is negative the location for
/// start of file is returned.
public func location(for position: AbsolutePosition) -> SourceLocation {
let physicalLocation = physicalLocation(for: position)
if let lastSourceLocationDirective = sourceLocationDirectives.last(where: { $0.sourceLine < physicalLocation.line }),
let arguments = lastSourceLocationDirective.arguments
{
let presumedLine = arguments.line + physicalLocation.line - lastSourceLocationDirective.sourceLine - 1
return SourceLocation(
line: physicalLocation.line,
column: physicalLocation.column,
offset: physicalLocation.offset,
file: physicalLocation.file,
presumedLine: presumedLine,
presumedFile: arguments.file
)
}

return physicalLocation
}

/// Compute the location of `position` without taking `#sourceLocation`
/// directives into account.
///
/// If the position is
/// exceeding the file length then the ``SourceLocation`` for the end of file
/// is returned. If position is negative the location for start of file is
/// returned.
Comment on lines 284 to 285
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow negative AbsolutePosition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think there’s a real use case. Would you prefer to assert or precondition? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

precondition seems fine to me

public func location(for origpos: AbsolutePosition) -> SourceLocation {
private func physicalLocation(for position: AbsolutePosition) -> SourceLocation {
// Clamp the given position to the end of file if needed.
let pos = min(origpos, endOfFile)
let pos = min(position, endOfFile)
if pos.utf8Offset < 0 {
return SourceLocation(line: 1, column: 1, offset: 0, file: self.file)
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftSyntaxMacrosTestSupport/Assertions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func assertNote(
expected spec: NoteSpec
) {
assertStringsEqualWithDiff(note.message, spec.message, "message of note does not match", file: spec.originatorFile, line: spec.originatorLine)
let location = note.location(converter: SourceLocationConverter(file: "", source: tree.description))
let location = note.location(converter: SourceLocationConverter(file: "", tree: tree))
XCTAssertEqual(location.line, spec.line, "line of note does not match", file: spec.originatorFile, line: spec.originatorLine)
XCTAssertEqual(location.column, spec.column, "column of note does not match", file: spec.originatorFile, line: spec.originatorLine)
}
Expand Down Expand Up @@ -187,7 +187,7 @@ func assertDiagnostic(
XCTAssertEqual(diag.diagnosticID, id, "diagnostic ID does not match", file: spec.originatorFile, line: spec.originatorLine)
}
assertStringsEqualWithDiff(diag.message, spec.message, "message does not match", file: spec.originatorFile, line: spec.originatorLine)
let location = diag.location(converter: SourceLocationConverter(file: "", source: tree.description))
let location = diag.location(converter: SourceLocationConverter(file: "", tree: tree))
XCTAssertEqual(location.line, spec.line, "line does not match", file: spec.originatorFile, line: spec.originatorLine)
XCTAssertEqual(location.column, spec.column, "column does not match", file: spec.originatorFile, line: spec.originatorLine)

Expand Down
6 changes: 3 additions & 3 deletions Tests/SwiftParserTest/Assertions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ func assertLocation<T: SyntaxProtocol>(
line: UInt = #line
) {
if let markerLoc = markerLocations[locationMarker] {
let locationConverter = SourceLocationConverter(file: "", source: tree.description)
let locationConverter = SourceLocationConverter(file: "", tree: tree)
let actualLocation = location
let expectedLocation = locationConverter.location(for: AbsolutePosition(utf8Offset: markerLoc))
if actualLocation.line != expectedLocation.line || actualLocation.column != expectedLocation.column {
Expand All @@ -355,7 +355,7 @@ func assertNote<T: SyntaxProtocol>(
expected spec: NoteSpec
) {
XCTAssertEqual(note.message, spec.message, file: spec.file, line: spec.line)
let locationConverter = SourceLocationConverter(file: "", source: tree.description)
let locationConverter = SourceLocationConverter(file: "", tree: tree)
assertLocation(
note.location(converter: locationConverter),
in: tree,
Expand All @@ -374,7 +374,7 @@ func assertDiagnostic<T: SyntaxProtocol>(
markerLocations: [String: Int],
expected spec: DiagnosticSpec
) {
let locationConverter = SourceLocationConverter(file: "", source: tree.description)
let locationConverter = SourceLocationConverter(file: "", tree: tree)
assertLocation(
diag.location(converter: locationConverter),
in: tree,
Expand Down
13 changes: 13 additions & 0 deletions Tests/SwiftParserTest/DirectiveTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,19 @@ final class DirectiveTests: XCTestCase {
}
"""
)

assertParse(
"""
#sourceLocation(file: "f.swift", line: 1️⃣-1)
""",
diagnostics: [
DiagnosticSpec(message: "expected line number in '#sourceLocation' arguments", fixIts: ["insert line number"]),
DiagnosticSpec(message: "unexpected code '-1' in '#sourceLocation' directive"),
],
fixedSource: """
#sourceLocation(file: "f.swift", line: <#integer literal#>-1)
"""
)
}

public func testUnterminatedPoundIf() {
Expand Down
Loading