Open Bug 143291 Opened 22 years ago Updated 2 years ago

function parameters should be checked at parse time


(Core :: XSLT, defect)






(Reporter: axel, Assigned: peterv)


(Depends on 1 open bug, Blocks 1 open bug)



(1 file, 3 obsolete files)

currently, FunctionCalls check their params at eval time, but that should happen
on parsing.
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
I'll have a stab at this once the branch has landed
Assignee: peterv → sicking
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

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.
Blocks: 164767
taking, patch coming up. This is subject of merges to come, I guess.
Assignee: bugmail → axel
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.
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.
Attached patch v2 (obsolete) — Splinter Review
This depends on bug 361441.
Assignee: axel → peterv
Attachment #132749 - Attachment is obsolete: true
Depends on: 361441
Attached patch v2 (obsolete) — Splinter Review
Updated to trunk.
Attachment #250053 - Attachment is obsolete: true
QA Contact: keith → xslt
Attached patch v2Splinter Review
Attachment #324774 - Attachment is obsolete: true
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.