Skip to content

Avoid expensive string array operations when performing isDescendent checks on AbsolutePath #9351

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Avoid expensive string array operations when performing isDescendent checks on AbsolutePath #9351

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c668f92
Avoid expensive string array operations when performing isDescendent …
rconnell9 Nov 8, 2025
9fff80a
Augment error description
rconnell9 Nov 8, 2025
68fe3c8
Check for windows path separators.
rconnell9 Nov 10, 2025
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
28 changes: 24 additions & 4 deletions Sources/Basics/FileSystem/AbsolutePath.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,31 +220,51 @@ extension AbsolutePath {
/// This method is strictly syntactic and does not access the file system
/// in any way.
public func isAncestor(of descendant: AbsolutePath) -> Bool {
self.underlying.isAncestor(of: descendant.underlying)
descendant.isDescendant(of: self)
}

/// Returns true if the path is an ancestor of or equal to the given path.
///
/// This method is strictly syntactic and does not access the file system
/// in any way.
public func isAncestorOfOrEqual(to descendant: AbsolutePath) -> Bool {
self.underlying.isAncestorOfOrEqual(to: descendant.underlying)
descendant.isDescendantOfOrEqual(to: self)
}

/// Returns true if the path is a descendant of the given path.
///
/// This method is strictly syntactic and does not access the file system
/// in any way.
public func isDescendant(of ancestor: AbsolutePath) -> Bool {
Copy link
Contributor

@owenv owenv Nov 10, 2025

Choose a reason for hiding this comment

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

Might be worth renaming this to isSyntacticDescendant of since at a glance it looks like it's being misused in a few places where we should be considering symlinks, etc.

self.underlying.isDescendant(of: ancestor.underlying)
let selfPath = self.pathString
let ancestorPath = ancestor.pathString

guard selfPath.count > ancestorPath.count else {
return false
}

guard selfPath.hasPrefix(ancestorPath) else {
return false
}

if ancestorPath.hasSuffix("/") {
Copy link
Contributor

@owenv owenv Nov 10, 2025

Choose a reason for hiding this comment

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

AFAICT pathString will have windows separators on Windows, which this won't match

Copy link
Contributor Author

@rconnell9 rconnell9 Nov 10, 2025

Choose a reason for hiding this comment

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

I originally thought they may all be normalized to "/" by TSC.

return true
}

// Handle non-root paths by ensuring the character after the prefix is a separator
let indexAfterPrefix = selfPath.index(selfPath.startIndex, offsetBy: ancestorPath.count)
return selfPath[indexAfterPrefix] == "/"
}

/// Returns true if the path is a descendant of or equal to the given path.
///
/// This method is strictly syntactic and does not access the file system
/// in any way.
public func isDescendantOfOrEqual(to ancestor: AbsolutePath) -> Bool {
self.underlying.isDescendantOfOrEqual(to: ancestor.underlying)
if self == ancestor {
return true
}
return self.isDescendant(of: ancestor)
}
}

Expand Down
6 changes: 5 additions & 1 deletion Sources/PackageLoading/PackageBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,11 @@ public final class PackageBuilder {
let publicHeaderComponent = manifestTarget.publicHeadersPath ?? ClangModule.defaultPublicHeadersComponent
let publicHeadersPath = try potentialModule.path.appending(RelativePath(validating: publicHeaderComponent))
guard publicHeadersPath.isDescendantOfOrEqual(to: potentialModule.path) else {
throw ModuleError.invalidPublicHeadersDirectory(potentialModule.name)
let publicHeadersPathString = publicHeadersPath.pathString
let potentialModulePathString = potentialModule.path.pathString
let potentialModuleName = potentialModule.name
let errorMessage = "\(publicHeadersPathString) isDescendantOfOrEqual \(potentialModulePathString) - potentialModule.name(\(potentialModuleName))"
throw ModuleError.invalidPublicHeadersDirectory(errorMessage)
}

let sourcesBuilder = TargetSourcesBuilder(
Expand Down