[Feat] function type checks #3389
Conversation
line-o
added
deprecation
xquery
exist-core/src/main/java/org/exist/xquery/functions/fn/FunHigherOrderFun.java
Outdated
Show resolved
Hide resolved
line-o
force-pushed
the
feat/function-type-checks
branch
from
a475f6d
to
ef097a5
Compare
line-o
force-pushed
the
feat/function-type-checks
branch
from
ef097a5
to
021dee1
Compare
- adapt tests - remove hack for flipped fn:filter parameters - specify function parameter types where necessary - add FunctionTypeCheck class (which mostly calls checkType of FunctionParameterFunctionSequenceType)
line-o
force-pushed
the
feat/function-type-checks
branch
from
d43dff6
to
1403b00
Compare
FunctionParameterFunctionSequenceType.arityMatches failed to detect the arity of a concrete reference of an overloaded function like `concat#3`.
|
Nine additional XQTS cases pass:
failing XQTS tests are due to
|
`EXXQST0001` indicates a missing implementation (class or function).
- Static errors in Function.createFunction now throw an XPathException with error code `EXXQST0001`. - remove redundant null initialization - replace call to PathExpr.getExpression with getSubExpression
|
SonarCloud Quality Gate failed.
|
|
As discussed in the Community Call, this PR helps with XQuery conformance, achieves 9 more passing XQTS tests that previously failed, provides better static checking, and lays the groundwork for additional type checking of function parameters and return values. |
There was a problem hiding this comment.
I think this is a great start, and I am excited to see it. I do think it requires some work yet to complete it and polish it.
Apart from my specific code comments I am a bit confused by this statement in the PR description:
NOTE:
An implementation decision was, to still allow developers to writefilter(1, function ($a) { exists($a) })instead of having to write
filter(1, function ($a) as xs:boolean { exists($a) })
Unless I am mistaken, that isn't a decision for an Implementation... It is actually required by the XQuery 3.1 specification, see: https://www.w3.org/TR/xquery-31/#prod-xquery31-InlineFunctionExpr
I am also not sure I understand this statement:
This later check is currently done in the function implementation itself (see fn:filter).
I understand that every function that accepts a function (as a parameter) will need its function signature modified to declare the additional parameter/return types, but
does that also mean that each of those also needs to be modified to perform the actual type checks of the function parameter's parameters? If so, it would seem that this checking should be moved out into something like the DynamicCardinalityCheck class.
| * @param description A description of the parameter in the <strong>FunctionSignature</strong>. | ||
| * @see org.exist.xquery.FunctionSignature @see Type @see org.exist.xquery.Cardinality | ||
| */ | ||
| public FunctionParameterFunctionSequenceType(final String attributeName, final int primaryType, final SequenceType[] parameterTypes, final SequenceType returnType, final Cardinality cardinality, final String description) { |
There was a problem hiding this comment.
If I am not mistaken then the value supplied to primaryType appears to always be Type.FUNCTION_REFERENCE. If that is the case, then the parameter can be removed in favour of the constant.
In the same vein it might make sense to modify FunctionParameterSequenceType so that its public constructor cannot accept a Type.FUNCTION_REFERENCE; instead throwing an IllegalArgumentException indicating that FunctionParameterFunctionSequenceType should be constructed instead.
| * mimics isOverloaded property of FunctionSignature | ||
| * @see org.exist.xquery.FunctionSignature | ||
| */ | ||
| private int arity = -1; |
There was a problem hiding this comment.
It is not clear to me how this relates to isOverloaded could you add some information? It is not possible to have overloaded function signatures in XQuery, this was previously just a non-compliant thing offered by eXist-db that was later removed.
Could you add some explanation of why this is needed? It looks to me that the arity of the function (as a a parameter) is simply the value of parameterTypes.length, and that we don't need an additional class member variable for that.
| */ | ||
| public class FunctionParameterFunctionSequenceType extends FunctionParameterSequenceType { | ||
|
|
||
| private SequenceType[] parameters = null; |
There was a problem hiding this comment.
This is only set from the constructor and could instead be assigned there and declared final here
| public class FunctionParameterFunctionSequenceType extends FunctionParameterSequenceType { | ||
|
|
||
| private SequenceType[] parameters = null; | ||
| private SequenceType returnType = new SequenceType(Type.ITEM, Cardinality.ZERO_OR_MORE); |
There was a problem hiding this comment.
This is only set from the constructor and could instead be assigned there and declared final here
| * @see org.exist.xquery.FunctionSignature @see Type @see org.exist.xquery.Cardinality | ||
| */ | ||
| public FunctionParameterFunctionSequenceType(final String attributeName, final int primaryType, final SequenceType[] parameterTypes, final Cardinality cardinality, final String description) { | ||
| super(attributeName, primaryType, cardinality, description); |
There was a problem hiding this comment.
Perhaps consider calling this(...) instead.
| * @param primaryType The <strong>Type</strong> of the parameter. | ||
| * @param parameterTypes The <strong>parameters</strong> the function(s) must accept. | ||
| * @param returnType The <strong>Type</strong> the function(s) needs to return. | ||
| * @param cardinality The <strong>Cardinality</strong> of the parameter. |
There was a problem hiding this comment.
The cardinality parameter should follow the primaryType parameter, as they are part of the same sequence type. This is how it currently works in FunctionParameterSequenceType and that should be preserved here, as it makes the code for defining a FunctionSignature more logical to both write and read.
| /** | ||
| * Check a function parameter type at runtime. | ||
| * | ||
| * @author wolf |
There was a problem hiding this comment.
Did you perhaps mean @author line-o?
| private final Expression expression; | ||
| private final FunctionParameterFunctionSequenceType requiredType; | ||
|
|
||
| public FunctionTypeCheck(XQueryContext context, final FunctionParameterFunctionSequenceType requiredType, Expression expr) { |
There was a problem hiding this comment.
parameters can be final. This applies throughout this class...
| /* (non-Javadoc) | ||
| * @see org.exist.xquery.Expression#analyze(org.exist.xquery.AnalyzeContextInfo) | ||
| */ | ||
| public void analyze(AnalyzeContextInfo contextInfo) throws XPathException { |
There was a problem hiding this comment.
Please remove the /* (non-Javadoc) (which is not used since very old versions of auto-code generation in Eclipse IDE), and replace with JavaDoc. This applies throughout this class...
| * Method {@link #setArguments(java.util.List)} will set it to false | ||
| * (unless overwritten), thus enforcing a check. | ||
| */ | ||
| protected boolean argumentsChecked = true; |
There was a problem hiding this comment.
From reading the JavaDoc and the naming of the variable. It is not clear to me whether argumentsChecked means that: "The arguments have already been checked", or whether it means that "The arguments will need to be checked". Perhaps some rewording/renaming here would help?
There was a problem hiding this comment.
Also, is protected scope correct here?
Description:
BREAKING CHANGE
Internal module functions can now enforce function parameters to adhere to a given signature.
This is added to all HoFs
fn:filter,fn:for-each,fn:fold-left,fn:fold-rightas wellas their Array and Map function variants.
It is written such that other function values can also be checked
array(xs:string)or complex ones likemap(xs:integer, function(array(*)) as xs:boolean))NOTE:
An implementation decision was, to still allow developers to write
instead of having to write
For function parameters that return
item()*, defer the return type check. Throw an error when the first non-matching result is returned.This later check is currently done in the function implementation itself (see
fn:filter).A better alternative would be:
item()*Examples:
fn:filter->function (item()) as xs:booleanarray:for-each-pair->function (item()*, item()*) as item()*map:for-each->function (xs:anyAtomicValue, item()*) as item()*Reference:
Result of discussion with @adamretter in #3364
Is related to #1917
supersedes #3361 (the expathrepo related commit will be added here)
fixes #3382
related #3370
Type of tests:
All tests pass, two where adapted to match correct behaviour.