143291 - function parameters should be checked at parse time

Status: ASSIGNED

(Core :: XSLT, defect)

Description

[Axel Hecht]

currently, FunctionCalls check their params at eval time, but that should happen
on parsing.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One way to do this could be to have a |validate| call after we are done parsing,
ExprParser::createExpr(const String& aExpression, txIParseContext* aContext).
That call could go recursivly through the entire tree of expressions and
nodetests. It could possibly also move some more validations steps from evaltime
to parsetime if we can think of any

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll have a stab at this once the branch has landed

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When doing this we need to keep forwards-compatible-processing in mind. The spec
says that in fcp "if the expression calls a function with a number of arguments
that XSLT does not allow or with arguments of types that XSLT does not allow,
then an error must not be signaled unless the function is actually called."

Either we could call the validate function in ExprParser::createFunctionCall and
replace the function with a ErrorExpr if validate returned false and f-c-p is
enabled.

Or we could let validate take an |MBool aFCPEnabled| argument and replace all
subexpressions that doesn't validate with ErrorExpr if f-c-p is enabled.

Comment 4

[Axel Hecht]

taking, patch coming up. This is subject of merges to come, I guess.

Comment 5

[Axel Hecht]

Attachment: Add FunctionCall::CheckParams to test for param count (may test expr return type in the future)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 132749 [details] [diff] [review]
Add FunctionCall::CheckParams to test for param count (may test expr return type in the future)

This breaks f-c-p. "false() && not()" must not fail in f-c-p. Behaviour in
non-fcp is ok either way.

Comment 7

[Axel Hecht]

re #6, I don't think that this is crucial in any way.
"false() && not()" is a counterproductive example, and there are no productive
ones, AFAICT. The logic in itself only works for binary functions, and only if
false is an acceptable default value.
Let alone that there isn't that much to be forwards compatible in the first place.

I vote for leaving the spec as it wants to be here and do something useful.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v2

This depends on bug 361441.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

Updated to trunk.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v2