Skip to content

[Macros] Cache SourceLocationConverter in ExportedSourceFile #70415

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
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
4 changes: 1 addition & 3 deletions lib/ASTGen/Sources/ASTGen/PluginHost.swift
Original file line number Diff line number Diff line change
Expand Up @@ -386,11 +386,9 @@ extension PluginMessage.Syntax {
}

let source = syntax.description
let sourceStr = String(decoding: sourceFilePtr.pointee.buffer, as: UTF8.self)
let fileName = sourceFilePtr.pointee.fileName
let fileID = "\(sourceFilePtr.pointee.moduleName)/\(sourceFilePtr.pointee.fileName.basename)"
let converter = SourceLocationConverter(file: fileName, source: sourceStr)
let loc = converter.location(for: syntax.position)
let loc = sourceFilePtr.pointee.sourceLocationConverter.location(for: syntax.position)

self.init(
kind: kind,
Expand Down
14 changes: 11 additions & 3 deletions lib/ASTGen/Sources/ASTGen/SourceFile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ public struct ExportedSourceFile {
/// The syntax tree for the complete source file.
public let syntax: SourceFileSyntax

/// A source location converter to convert `AbsolutePosition`s in `syntax` to line/column locations.
///
/// Cached so we don't need to re-build the line table every time we need to convert a position.
let sourceLocationConverter: SourceLocationConverter

public func position(of location: BridgedSourceLoc) -> AbsolutePosition? {
let sourceFileBaseAddress = UnsafeRawPointer(buffer.baseAddress!)
guard let opaqueValue = location.getOpaquePointerValue() else {
Expand Down Expand Up @@ -75,12 +80,15 @@ public func parseSourceFile(
let sourceFile = Parser.parse(source: buffer, experimentalFeatures: .init(from: ctx))

let exportedPtr = UnsafeMutablePointer<ExportedSourceFile>.allocate(capacity: 1)
let moduleName = String(cString: moduleName)
let fileName = String(cString: filename)
exportedPtr.initialize(
to: .init(
buffer: buffer,
moduleName: String(cString: moduleName),
fileName: String(cString: filename),
syntax: sourceFile
moduleName: moduleName,
fileName: fileName,
syntax: sourceFile,
sourceLocationConverter: SourceLocationConverter(fileName: fileName, tree: sourceFile)
)
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,6 @@ extension SourceManager.MacroExpansionContext: MacroExpansionContext {
return nil
}

// Determine the filename to use in the resulting location.
let fileName: String
switch filePathMode {
case .fileID:
fileName = "\(exportedSourceFile.moduleName)/\(exportedSourceFile.fileName.basename)"

case .filePath:
fileName = exportedSourceFile.fileName
}

// Find the node's offset relative to its root.
let rawPosition: AbsolutePosition
Expand All @@ -146,8 +137,37 @@ extension SourceManager.MacroExpansionContext: MacroExpansionContext {
let offsetWithinSyntaxNode =
rawPosition.utf8Offset - node.position.utf8Offset

var location = exportedSourceFile.sourceLocationConverter.location(
for: rootPosition.advanced(by: offsetWithinSyntaxNode)
)

switch filePathMode {
case .fileID:
// The `SourceLocationConverter` in `exportedSourceFile` uses `filePath` as the file mode. When the `fileID` mode
// is requested, we need to adjust the file and presumed file to the `fileID`.
let fileID = "\(exportedSourceFile.moduleName)/\(exportedSourceFile.fileName.basename)"
var adjustedFile = location.file
if adjustedFile == exportedSourceFile.fileName {
adjustedFile = fileID
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean if they aren't equal? I thought that would be the presumed file case, but apparently not?

Copy link
Member Author

@ahoppen ahoppen Dec 14, 2023

Choose a reason for hiding this comment

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

AFAIK it can only happen for the presumed file. I can change it to an assertion that adjustedFile == exportedSourceFile.fileName. Don’t have strong opinions here. Just thought that it would be better to recover instead of crashing/asserting.

}
var adjustedPresumedFile = location.presumedFile
if adjustedPresumedFile == exportedSourceFile.fileName {
adjustedPresumedFile = fileID
}
location = SourceLocation(
line: location.line,
column: location.column,
offset: location.offset,
file: adjustedFile,
presumedLine: location.presumedLine,
presumedFile: adjustedPresumedFile
)

case .filePath:
break
}

// Do the location lookup.
let converter = SourceLocationConverter(file: fileName, tree: sourceFile)
return AbstractSourceLocation(converter.location(for: rootPosition.advanced(by: offsetWithinSyntaxNode)))
return AbstractSourceLocation(location)
}
}