Saxon

I have reproduced the problem (it seems we never "productised" the test case for bug #3483 to make it part of the standard test suite).

The failure occurs while we are inlining a call on m:string() - where the function being inlined is in a precompiled library module. The process of copying the function body appears to be incorrectly making a modification to the original. Specifically, LetExpression.copy() contains the logic

        Expression newAction = getAction().copy(rebindings);
        let.setAction(newAction);
        ExpressionTool.rebindVariableReferences(newAction, this, let);

with rebindings set to null, which means that copying the body (action part) of the LetExpression rebinds the variable references in the new copy to the declaration in the old copy (which adds the copied variable references to the reference list of the original LetExpression); they are then subsequently rebound to the new copy using the rebindVariableReferences() call, but this is too late.

It seems we introduced the RebindingMap passed as an argument to copy() methods way back in Saxon 9.7 in order to prevent this kind of problem, but we never made full use of the mechanism. In fact there are only two paths on which we add variable bindings to the map: in LocalParamBlock, and in PatternThatSetsCurrent.

Changing the code in LetExpression.copy() to add the call rebindings.put(this, let); fixes the problem (this also makes the call in rebindVariableReferences() superfluous, since the rebinding will have been done on the fly).

However, (a) this change will require careful regression testing, and (b) LetExpression is not the only expression affected; the same logic appears in other expressions that bind local variables, for example ForExpression and QuantifiedExpression.

Note the comment in ForExpression.copy():

// TODO: should be able to do this by adding a mapping to rebindings as above. But -s:app-Walmsley -t:d1e41271 crashes.

However, I made the change and ran this test, and it works fine after the change.

I've applied the corresponding change to ForExpression, QuantifiedExpression, and OuterForExpression, and have run the QT3 and XSLT3 test suites, with no apparent regression.

There are a couple of other places where ExpressionTool.rebindVariableReferences() is called where it's less clear what needs to be done:

(a) FLWORExpression.copy() --- this should set up the rebindings when copying the subexpressions, but it's rather more complicated to achieve because we're dealing with multiple variables.

(b) FLWORExpression.rewriteForOrLet() - this doesn't actually copy the subexpressions, it modifies them in situ. In general doing this is not safe, and has been the source of quite a few bugs. But I think in the absence of a test case that reveals a bug, I will leave this code alone. The same applies when we rewrite a ForExpression as a LetExpression in cases where the cardinality of the selection is 1.

(c) OptimizerEE.tryInlineFunctionCall(). Here, given a function such as function f ($m) {$m+1} and a function call f(12), we first copy the function body $m+1 (with an empty Rebinding map); then for each function parameter generate a LetExpression which in this case takes the form let $m := 12, and then, in the copied function body, rebind the variable reference $m to the new LetExpression. This is certainly vulnerable to the same problem that the reference list for the original parameter is being modified. (The inlined function call turns into let $m := 12 return $m+1, which subsequently gets rewritten as let $m := 12 return 13, and finally as 13.)

I think (a) and (c) should probably be addressed.

I've made the change for (a) and it turns out the new code is a bit simpler than the old. Appears to cause no regression.

Regarding (c) the code as written appears to be safe because when a LocalVariableReference is bound to a UserFunctionParameter, no modifications are made to the UserFunctionParameter object. (The object has a referenceCount field, but this is never updated and never used).

Tested OK with 9.9.1.6. Thanks!