Adjust more TreePaths to have correct leaf before calling getTreeType#1570
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR threads explicit TreePath context through generics inference and related flows: method-reference qualifier resolution uses a TreePath-rooted state; generateConstraintsForCall builds invocation and per-argument TreePaths and runs pseudo-assignment under state.withPath(...); ReturnFinder is rewritten to return TreePath objects; a pathWithLeaf helper ensures dataflow/refinement queries use paths whose leaf matches the target expression; stripParensAndUpdateTreePath now throws on mismatched path leaves. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
2192-2202: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePrefer the caller's structurally-correct parent path over re-deriving it from
state.getPath().When
path.getLeaf() != assignment,pathWithLeaf(path, assignment)produces aTreePathwhereassignmentis appended below the current state path (typically the invocation), even though in the real AST the assignment is an ancestor of that invocation. The currentgetTreeTypeimplementation only requires leaf-correctness forAssignmentTree/VariableTree, so this happens to work today, but it relies on the same "sketchy" pattern called out in therefineArgumentTypeWithDataflowcomment. Here, the immediate caller (getInvocationAndContextForInference) already computed a structurally-correctparentPathto the assignment — threading it through avoids the issue entirely and makes the path safe for any future use.♻️ Suggested change (call-site + signature)
if (parent instanceof AssignmentTree || parent instanceof VariableTree) { - return getInvocationInferenceInfoForAssignment(parent, invocation, state); + return getInvocationInferenceInfoForAssignment(parent, parentPath, invocation, state); }private InvocationAndContext getInvocationInferenceInfoForAssignment( - Tree assignment, MethodInvocationTree invocation, VisitorState state) { + Tree assignment, TreePath assignmentPath, MethodInvocationTree invocation, VisitorState state) { Preconditions.checkArgument( assignment instanceof AssignmentTree || assignment instanceof VariableTree); - TreePath path = state.getPath(); - if (path.getLeaf() != assignment) { - state = state.withPath(pathWithLeaf(path, assignment)); + if (state.getPath().getLeaf() != assignment) { + state = state.withPath(assignmentPath); } Type treeType = getTreeType(assignment, state); return new InvocationAndContext(invocation, treeType, isAssignmentToLocalVariable(assignment)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 2192 - 2202, The current getInvocationInferenceInfoForAssignment re-derives a TreePath from state.getPath() and calls pathWithLeaf, which can produce an incorrect descendant path; change the API to accept the caller's structurally-correct parent TreePath (e.g., add a TreePath parentPath parameter or pass through the existing parentPath from getInvocationAndContextForInference) and use that parentPath when creating the path for state.withPath instead of path.getLeaf()/pathWithLeaf; update the caller (getInvocationAndContextForInference) to pass its computed parentPath, and keep the remainder (calls to getTreeType and construction of InvocationAndContext) unchanged.
1670-1700: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueHoist the duplicate
pathWithLeaf(state.getPath(), currentActualParam)computation.Lines 1678 and 1686 compute the same
TreePathand bind it to two different local names (pathToActualParamandpathToParam). SincestateandcurrentActualParamdon't change between the two sites, you can compute this once and reuse it for both thegetTreeTypecall and theinferGenericMethodCallTypecall. This also makes it clearer that the two call sites are operating on the same path context.♻️ Suggested simplification
Type actualParameterType = null; if (currentActualParam instanceof LambdaExpressionTree) { maybeStorePolyExpressionTypeFromTarget(currentActualParam, formalParameter); } Type inferredPolyType = inferredPolyExpressionTypes.get(currentActualParam); + TreePath pathToActualParam = pathWithLeaf(state.getPath(), currentActualParam); if (inferredPolyType != null) { actualParameterType = inferredPolyType; } else { - TreePath pathToActualParam = pathWithLeaf(state.getPath(), currentActualParam); actualParameterType = getTreeType(currentActualParam, state.withPath(pathToActualParam)); } if (actualParameterType != null) { if (isGenericCallNeedingInference(currentActualParam)) { // infer the type of the method call based on the assignment context // and the formal parameter type - TreePath pathToParam = pathWithLeaf(state.getPath(), currentActualParam); actualParameterType = inferGenericMethodCallType( - state.withPath(pathToParam), + state.withPath(pathToActualParam), (MethodInvocationTree) currentActualParam, - pathToParam, + pathToActualParam, formalParameter, false, false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 1670 - 1700, Compute TreePath once and reuse it: replace the two separate calls to pathWithLeaf(state.getPath(), currentActualParam) (previously bound to pathToActualParam and pathToParam) with a single local variable (e.g., pathToActualParam) created once before calling getTreeType and inferGenericMethodCallType; then pass that same TreePath into getTreeType(...) and into inferGenericMethodCallType(...) (used in the isGenericCallNeedingInference branch) and remove the duplicate variable to avoid recomputation while keeping references to currentActualParam, state, getTreeType, and inferGenericMethodCallType intact.nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
754-760: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the stale comment and drop the dead commented-out fallback.
The comment at lines 754–757 still describes the old fallback ("we just return the original expression and state ... remove this fallback case"), but the code now throws unconditionally on a leaf mismatch. Line 760 also leaves the previous return commented out as dead code. These should be brought in sync with the new behavior so future readers (and the issue tracker note) aren't misled.
♻️ Proposed cleanup
TreePath path = state.getPath(); if (path.getLeaf() != expr) { - // if the expression is not the leaf of the path, we can't update the path to point to the - // stripped expression, so we just return the original expression and state - // TODO fix all cases where this happens and remove this fallback case - // Tracked in https://github.com/uber/NullAway/issues/1479 + // The caller is required to pass a state whose tree path's leaf is `expr`; otherwise we + // cannot reliably update the path to point at the stripped expression. + // See https://github.com/uber/NullAway/issues/1479 for tracking remaining call sites. throw new RuntimeException( String.format("Wrong leaf %s in path to %s", path.getLeaf(), expr)); - // return new ExprTreeAndState(expr, state); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java` around lines 754 - 760, The comment describing a now-removed fallback is stale and the commented-out return is dead; update the comment above the RuntimeException in NullabilityUtil (the block that references path.getLeaf(), expr and currently throws RuntimeException) to state that a leaf mismatch is a fatal/unrecoverable error and that we intentionally throw, and remove the commented-out "return new ExprTreeAndState(expr, state);" (and any other obsolete text referencing the old fallback) so the code and comment accurately reflect the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 2192-2202: The current getInvocationInferenceInfoForAssignment
re-derives a TreePath from state.getPath() and calls pathWithLeaf, which can
produce an incorrect descendant path; change the API to accept the caller's
structurally-correct parent TreePath (e.g., add a TreePath parentPath parameter
or pass through the existing parentPath from
getInvocationAndContextForInference) and use that parentPath when creating the
path for state.withPath instead of path.getLeaf()/pathWithLeaf; update the
caller (getInvocationAndContextForInference) to pass its computed parentPath,
and keep the remainder (calls to getTreeType and construction of
InvocationAndContext) unchanged.
- Around line 1670-1700: Compute TreePath once and reuse it: replace the two
separate calls to pathWithLeaf(state.getPath(), currentActualParam) (previously
bound to pathToActualParam and pathToParam) with a single local variable (e.g.,
pathToActualParam) created once before calling getTreeType and
inferGenericMethodCallType; then pass that same TreePath into getTreeType(...)
and into inferGenericMethodCallType(...) (used in the
isGenericCallNeedingInference branch) and remove the duplicate variable to avoid
recomputation while keeping references to currentActualParam, state,
getTreeType, and inferGenericMethodCallType intact.
In `@nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java`:
- Around line 754-760: The comment describing a now-removed fallback is stale
and the commented-out return is dead; update the comment above the
RuntimeException in NullabilityUtil (the block that references path.getLeaf(),
expr and currently throws RuntimeException) to state that a leaf mismatch is a
fatal/unrecoverable error and that we intentionally throw, and remove the
commented-out "return new ExprTreeAndState(expr, state);" (and any other
obsolete text referencing the old fallback) so the code and comment accurately
reflect the current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b55719b-3ca9-4dba-aa2f-feddc9d7fe67
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
1047-1067:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRe-anchor
stateandpathafter stripping parentheses.Line 1047 can change
rhsExprfrom aParenthesizedTreeto its child, but the method keeps using the oldstate/path. That makesgetTreeType(rhsExpr, state)violate the new strict leaf invariant, and it also passes a stale path into recursive inference for parenthesized nested calls. Valid inputs like parenthesized actuals or lambda returns will now blow up here.Suggested fix
- rhsExpr = ASTHelpers.stripParentheses(rhsExpr); + ExpressionTree strippedRhsExpr = ASTHelpers.stripParentheses(rhsExpr); + if (strippedRhsExpr != rhsExpr) { + TreePath rhsPath = pathWithLeaf(path != null ? path : state.getPath(), strippedRhsExpr); + path = rhsPath; + state = state.withPath(rhsPath); + } + rhsExpr = strippedRhsExpr;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41a160f8-265d-4d4e-8072-f917b2e3b9e9
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
1983-1988:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the explicit invocation path into nested-nullability repair.
substituteTypeArgsInGenericMethodType(...)already carries an explicitpathfor cases wherestate.getPath()is not the invocation leaf, butrestoreNestedNullabilityForTypeVarArguments(...)drops that and rebuildspathToInvocationfromstate.getPath()again. In those callers, Line 2031 can still derive argument paths from the wrong leaf, so thegetTreeType(...)call on Line 2055 can miss the exact path/leaf alignment this PR is trying to enforce.Proposed fix
- methodTypeAtCallSite = - restoreNestedNullabilityForTypeVarArguments( - invocationTree, methodType, methodTypeAtCallSite, state); + methodTypeAtCallSite = + restoreNestedNullabilityForTypeVarArguments( + invocationTree, path, methodType, methodTypeAtCallSite, state); @@ private Type.MethodType restoreNestedNullabilityForTypeVarArguments( MethodInvocationTree invocationTree, + `@Nullable` TreePath invocationPath, Type.MethodType origMethodType, Type.MethodType methodTypeAtCallSite, VisitorState state) { @@ - TreePath pathToInvocation = pathWithLeaf(state.getPath(), invocationTree); + TreePath pathToInvocation = + invocationPath != null + ? pathWithLeaf(invocationPath, invocationTree) + : pathWithLeaf(state.getPath(), invocationTree);Also applies to: 2017-2055
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 1983 - 1988, The nested-nullability repair drops the explicit invocation path: change restoreNestedNullabilityForTypeVarArguments to accept an explicit TreePath/Path parameter (instead of always using state.getPath()), and use that passed-in path when computing pathToInvocation/getTreeType; then update callers (notably substituteTypeArgsInGenericMethodType and the call sites that build methodTypeAtCallSite) to thread the explicit invocation leaf/path through into the new parameter so argument paths are derived from the correct leaf. Ensure method signatures and all call sites (including the branch handling InferenceSuccess where methodTypeAtCallSite is set) are updated to pass the invocationTree/path consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1983-1988: The nested-nullability repair drops the explicit
invocation path: change restoreNestedNullabilityForTypeVarArguments to accept an
explicit TreePath/Path parameter (instead of always using state.getPath()), and
use that passed-in path when computing pathToInvocation/getTreeType; then update
callers (notably substituteTypeArgsInGenericMethodType and the call sites that
build methodTypeAtCallSite) to thread the explicit invocation leaf/path through
into the new parameter so argument paths are derived from the correct leaf.
Ensure method signatures and all call sites (including the branch handling
InferenceSuccess where methodTypeAtCallSite is set) are updated to pass the
invocationTree/path consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24a5183d-7813-4e2e-81b1-0b0155267288
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
1221-1271: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider tightening
ReturnFindervisibility.
ReturnFinderis declared package-private (static class ReturnFinder). It is only used withinGenericsChecks, and other private helpers in this file (e.g.pathWithLeaf, the variousreport...methods) are alreadyprivate. Marking the classprivatewould match that style and prevent accidental external use.♻️ Proposed refactor
- static class ReturnFinder extends TreePathScanner<@Nullable Void, `@Nullable` Void> { + private static class ReturnFinder extends TreePathScanner<@Nullable Void, `@Nullable` Void> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 1221 - 1271, ReturnFinder is declared package-private but is only used inside GenericsChecks; make it a private static nested class to match the other private helpers and prevent external use. Change the declaration of ReturnFinder to private static class ReturnFinder and ensure any internal references (e.g., the static factory method findReturnPaths, the getReturnPaths accessor, and overrides like visitLambdaExpression, visitClass, visitMethod, visitReturn) remain accessible from within GenericsChecks; no other code changes should be necessary if all usages are internal.
2187-2197: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor: defensive
path.getLeaf() != assignmentcheck is dead with the only current caller.The only caller (
getInvocationAndContextForInference, line 2122-2123) already invokes this withstate.withPath(parentPath)whereparentPath.getLeaf() == parent == assignment, so the branch never fires today. It's harmless as defensive code in case a future caller passes an unalignedstate, but you could simplify to an unconditionalstate.withPath(pathWithLeaf(state.getPath(), assignment))(which is also a no-op when leaves match) or drop the wrapping entirely and require callers to pre-align. Up to you.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 2187 - 2197, The conditional defensive check in getInvocationInferenceInfoForAssignment is unnecessary given current callers; replace the if-check and conditional state reassignment with an unconditional state = state.withPath(pathWithLeaf(path, assignment)) (using the existing local path and assignment) so alignment is always applied (it's a no-op when path.getLeaf() already equals assignment) and leave getTreeType and the InvocationAndContext creation unchanged; alternatively, if you prefer requiring callers to pass an aligned state, remove the path alignment entirely—if you choose removal, update or document callers such as getInvocationAndContextForInference to ensure they pass a state whose leaf equals the assignment.
2299-2312: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using symmetric state path handling in both branches for consistency and maintainability.
The current code is functionally safe: although the
ifbranch passes unmodifiedstatetoinferGenericMethodCallType, thereceiverPathparameter is used at line 1016 ingenerateConstraintsForCallto establish thepathToInvocationbasis. By the timestripParensAndUpdateTreePathis invoked withingenerateConstraintsForPseudoAssignment, the state has been properly re-anchored viastate.withPath(pathToArgument), so the throwing contract instripParensAndUpdateTreePath(which requirespath.getLeaf() == expr) will not be triggered.However, the asymmetry is unnecessarily confusing: the
ifbranch effectively delegates path correction to thepathparameter, while theelsebranch (line 2311) directly passes the corrected state. For code clarity and to protect against future refactoring, consider applying the same pattern in both branches by usingstate.withPath(receiverPath)in the inference branch as suggested.♻️ Suggested symmetric update
if (isGenericCallNeedingInference(receiver)) { enclosingType = inferGenericMethodCallType( - state, + state.withPath(receiverPath), (MethodInvocationTree) receiver, receiverPath, null, false, calledFromDataflow); } else { enclosingType = getTreeType(receiver, state.withPath(receiverPath)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 2299 - 2312, The two branches handling receiver path are asymmetric: the else branch calls getTreeType(state.withPath(receiverPath)) but the if branch calls inferGenericMethodCallType(state, ..., receiverPath, ...) leaving state unmodified; change the if branch to pass state.withPath(receiverPath) into inferGenericMethodCallType so both branches consistently anchor the state to receiverPath (referencing inferGenericMethodCallType, getTreeType, receiverPath, and state.withPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 789-794: The explicit path argument passed alongside an updated
state is redundant: when you call inferGenericMethodCallType /
generateConstraintsForCall with state.withPath(pathToRhs) (or
state.withPath(pathToRetExpr)) and also pass the same path (e.g., the call site
that currently passes state.withPath(pathToRhs), (MethodInvocationTree) rhsTree,
pathToRhs, ...), remove the duplicate by passing null for the explicit path
parameter (or otherwise stop passing pathToRhs) so generateConstraintsForCall
will derive the path from state.getPath(); apply the same change to the
analogous call sites (including the other occurrence noted around the second
block) where the explicit path equals the new state path.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1221-1271: ReturnFinder is declared package-private but is only
used inside GenericsChecks; make it a private static nested class to match the
other private helpers and prevent external use. Change the declaration of
ReturnFinder to private static class ReturnFinder and ensure any internal
references (e.g., the static factory method findReturnPaths, the getReturnPaths
accessor, and overrides like visitLambdaExpression, visitClass, visitMethod,
visitReturn) remain accessible from within GenericsChecks; no other code changes
should be necessary if all usages are internal.
- Around line 2187-2197: The conditional defensive check in
getInvocationInferenceInfoForAssignment is unnecessary given current callers;
replace the if-check and conditional state reassignment with an unconditional
state = state.withPath(pathWithLeaf(path, assignment)) (using the existing local
path and assignment) so alignment is always applied (it's a no-op when
path.getLeaf() already equals assignment) and leave getTreeType and the
InvocationAndContext creation unchanged; alternatively, if you prefer requiring
callers to pass an aligned state, remove the path alignment entirely—if you
choose removal, update or document callers such as
getInvocationAndContextForInference to ensure they pass a state whose leaf
equals the assignment.
- Around line 2299-2312: The two branches handling receiver path are asymmetric:
the else branch calls getTreeType(state.withPath(receiverPath)) but the if
branch calls inferGenericMethodCallType(state, ..., receiverPath, ...) leaving
state unmodified; change the if branch to pass state.withPath(receiverPath) into
inferGenericMethodCallType so both branches consistently anchor the state to
receiverPath (referencing inferGenericMethodCallType, getTreeType, receiverPath,
and state.withPath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6d95894-5572-4f78-ad8c-ebe21e944f32
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1570 +/- ##
============================================
- Coverage 88.44% 88.40% -0.04%
Complexity 2881 2881
============================================
Files 103 103
Lines 9609 9637 +28
Branches 1938 1940 +2
============================================
+ Hits 8499 8520 +21
- Misses 532 535 +3
- Partials 578 582 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Fixes #1479
Now the
stripParensAndUpdateTreePathutility method will throw if the leaf of theTreePathpassed in doesn't matchexpr. This is a good invariant to enforce. There are a lot of code changes here, but they are all related to manipulatingTreePaths rather than core logic, and the newthrowwill guard against getting this wrong in the future.Summary by CodeRabbit
Bug Fixes
Improvements