-
Notifications
You must be signed in to change notification settings - Fork 115
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
Continues searching if a non-symbol matches a symbol link #901
Changes from 2 commits
6abed54
45a398b
b9badcf
cfa4103
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -18,14 +18,7 @@ extension PathHierarchy { | |||
/// - Returns: Returns the unique identifier for the found match or raises an error if no match can be found. | ||||
/// - Throws: Raises a ``PathHierarchy/Error`` if no match can be found. | ||||
func find(path rawPath: String, parent: ResolvedIdentifier? = nil, onlyFindSymbols: Bool) throws -> ResolvedIdentifier { | ||||
let node = try findNode(path: rawPath, parentID: parent, onlyFindSymbols: onlyFindSymbols) | ||||
if node.identifier == nil { | ||||
throw Error.unfindableMatch(node) | ||||
} | ||||
if onlyFindSymbols, node.symbol == nil { | ||||
throw Error.nonSymbolMatchForSymbolLink(path: rawPath[...]) | ||||
} | ||||
return node.identifier | ||||
return try findNode(path: rawPath, parentID: parent, onlyFindSymbols: onlyFindSymbols).identifier | ||||
} | ||||
|
||||
private func findNode(path rawPath: String, parentID: ResolvedIdentifier?, onlyFindSymbols: Bool) throws -> Node { | ||||
|
@@ -219,10 +212,21 @@ extension PathHierarchy { | |||
var node = startingPoint | ||||
var remaining = pathComponents[...] | ||||
|
||||
/// Ensure that the return value is valid for this search. | ||||
func validateReturnValue(_ node: Node) throws -> Node { | ||||
if node.identifier == nil { | ||||
throw Error.unfindableMatch(node) | ||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this check needed if the fileprivate(set) var identifier: ResolvedIdentifier! If there's a possibility for this value to be nil, then shouldn't it be an optional value? All the tests pass without this check, so I'm not clear on whether it's needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code didn't change, I just moved it.
Yes, it can be The reason why this exists is that the convert service build documentation with a "sparse" symbol graph file that only contains a single symbol. However, this symbol may have path components that represent symbols that aren't in that symbol graph. For example, if the SGF only contains The reason for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a comment elsewhere in the path hierarchy implementation that talk about this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, but I agree with @sofiaromorales this is confusing or even dangerous. We need to be 100% sure that the identifier will never be nil before trying to access it... Maybe that's the entire purpose of this function? To insure that no other code that uses |
||||
if onlyFindSymbols, node.symbol == nil { | ||||
throw Error.nonSymbolMatchForSymbolLink(path: rawPathForError) | ||||
} | ||||
return node | ||||
} | ||||
|
||||
// Third, search for the match relative to the start node. | ||||
if remaining.isEmpty { | ||||
// If all path components were consumed, then the start of the search is the match. | ||||
return node | ||||
return try validateReturnValue(node) | ||||
} | ||||
|
||||
// Search for the remaining components from the node | ||||
|
@@ -238,11 +242,12 @@ extension PathHierarchy { | |||
remaining = remaining.dropFirst() | ||||
if remaining.isEmpty { | ||||
// If all path components are consumed, then the match is found. | ||||
return child | ||||
return try validateReturnValue(child) | ||||
} | ||||
} catch DisambiguationContainer.Error.lookupCollision(let collisions) { | ||||
func handleWrappedCollision() throws -> Node { | ||||
try handleCollision(node: node, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPathForError) | ||||
let match = try handleCollision(node: node, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPathForError) | ||||
return try validateReturnValue(match) | ||||
} | ||||
|
||||
// When there's a collision, use the remaining path components to try and narrow down the possible collisions. | ||||
|
@@ -270,13 +275,13 @@ extension PathHierarchy { | |||
} | ||||
} | ||||
// A wrapped error would have been raised while iterating over the collection. | ||||
return uniqueCollisions.first!.value | ||||
return try validateReturnValue(uniqueCollisions.first!.value) | ||||
} | ||||
|
||||
// Look ahead one path component to narrow down the list of collisions. | ||||
// For each collision where the next path component can be found unambiguously, return that matching node one level down. | ||||
let possibleMatchesOneLevelDown = collisions.compactMap { | ||||
return try? $0.node.children[String(nextPathComponent.name)]?.find(nextPathComponent.disambiguation) | ||||
try? $0.node.children[String(nextPathComponent.name)]?.find(nextPathComponent.disambiguation) | ||||
} | ||||
let onlyPossibleMatch: Node? | ||||
|
||||
|
@@ -296,7 +301,7 @@ extension PathHierarchy { | |||
remaining = remaining.dropFirst(2) | ||||
if remaining.isEmpty { | ||||
// If that was the end of the path we can simply return the result. | ||||
return onlyPossibleMatch | ||||
return try validateReturnValue(onlyPossibleMatch) | ||||
} else { | ||||
// Otherwise we continue looping over the remaining path components. | ||||
node = onlyPossibleMatch | ||||
|
@@ -305,7 +310,8 @@ extension PathHierarchy { | |||
} | ||||
|
||||
// Couldn't resolve the collision by look ahead. | ||||
return try handleCollision(node: node, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPathForError) | ||||
// return try handleCollision(node: node, remaining: remaining, collisions: collisions, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPathForError) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
return try handleWrappedCollision() | ||||
} | ||||
} | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, it was very easy to state that this method returned a
PathHierarchy/Error
. However, now that this method calls another very extensive method with its own inner method calls, it's more difficult to assure that this- Throws: Raises a PathHierarchy/Error if no match can be found.
is always true.I checked all the other throw statements inside
findNode
, and it seems like they all throw aPathHierarchy/Error
. However, since there are multiple nested calls, I find it very easy to mistakenly throw another type of error without noticing this comment.Any thoughts about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment still holds true. All errors raised from
PathHierarchy/find(path:parent:onlyFindSymbols:)
arePathHierarchy/Error
. In the future when we eventually update to Swift 6 we can adopt typed throws here to have the compiler enforce this.