Heuristic to detect incorrectly documented Optional member parameters

[sap]

Heuristic to detect incorrectly documented Optional member parameters

[bd82]

Thanks @RobertoMalatesta

The TypeScript signatures are generated from OpenUI5 api.json output, which is itself
generated from the JSDocs that are part of the OpenUI5 source code.

If you look the nav container source code in OpenUI5 you will see that the JSDocs are incorrect
and do not specify that arguments 2 and 3 are optional.

• https://github.com/SAP/openui5/blob/824d44b11c710de181363541b153bd93deef11e6/src/sap.m/src/sap/m/NavContainer.js#L853-L880

To specific an optional argument in JSDocs you would need to use square brackets.

// @param {string} [fooOptional]
// @param {string} barMandatory

See: http://usejsdoc.org/tags-type.html
Basically this needs to be fixed in the OpenUI5 sources directly.
Do you want to try creating a PR there?

[RobertoMalatesta]

Hi @bd82 !

The TypeScript signatures are generated from OpenUI5 api.json output [...]
JSDocs are incorrect and do not specify that arguments 2 and 3 are optional

I know it since I managed to get my own bindings some time ago.
Sometimes parameter optionality is only mentioned in the textual description.
This is the case in lots of components.
On the TS side I resolved by adding lots of union types (type|undefined) with the rationale: if the description mentions a possible optionality of the parameter add |undefined.

Then there's one other problem, Typescript-related:
if you have multiple optional parameters and if you want to skip one
basically you need to adopt the union types solution (or the boxing/unboxing approach)
This depends on the unavailability of (C#-like) named parameters in function/method calling in TS.

Do you want to try creating a PR there?

I seem to recall someone already did open it or mentioned the documentation issue in a discussion.
I suppose it won't be an easy task for the UI5 team to go after all declarations in a reasonable ETA.
(The case opened for TS bindings itself is dated 2014.)

--R

[bd82]

Sometimes parameter optionality is only mentioned in the textual description.
This is the case in lots of components.
On the TS side I resolved by adding lots of union types (type|undefined) with the rationale: if the description mentions a possible optionality of the parameter add |undefined.

That an interesting heuristic that should be considered for implementation.
There are already multiple heuristics during the transformation flow...

Then there's one other problem, Typescript-related:
if you have multiple optional parameters and if you want to skip one
basically you need to adopt the union types solution (or the boxing/unboxing approach)
This depends on the unavailability of (C#-like) named parameters in function/method calling in TS.

I solved a similar problem of optional params followed by required params by creating all possible overloads of a function/method so all possible argument lists would be valid.

• https://github.com/SAP/ui5-typescript/blob/master/packages/dts-generator/lib/phases/ast-fixer.js#L328-L373

However I am not sure it is true to say that any instance where there is a sequence of optional arguments
can be replaced with type unions combos because not all implementations actually
perform these type checks and assignments at runtime.

In those cases I would expect the JSDocs documentation to be corrected to represent those type unions.

[bd82]

I seem to recall someone already did open it or mentioned the documentation issue in a discussion.
I suppose it won't be an easy task for the UI5 team to go after all declarations in a reasonable ETA.

One could potentially automate this: if we have a heuristic that identifies a description that specifies an optional parameter but the parameter's JSDocs is marked as required, one could create a list of issues that need to be resolved.

In theory it may even be possible to create this in an auto-fixable manner, e.g: embedding the logic
in an eslint rule with auto-fix capabilities.

[RobertoMalatesta]

There are already multiple heuristics during the transformation flow...

Documentation has greatly improved in the years but still cannot IMHO used per se, whithout referring to the source, since it still shows wrong info between fields.

A simple example, let's stay on sap.m.NavContainer.to method:
pageid is marked as type string, but the description states: "... The ID or the control itself can be given."
[this affected me since I never in my code use string IDs since TypeScript (but well written JS as well) allows you to work with references and variables]:

(So, pageId should be a union type as well...)

[RobertoMalatesta]

That an interesting heuristic that should be considered for implementation.

Another one, more subtle is collecting all double quoted strings in the description of an argument marked as -say- string (generic string) to extract all possible values and create a string enum /string const array.
i.e. the sap.m.NavContainer.to transitionName parameter:

[RobertoMalatesta]

I solved a similar problem of optional params followed by required params by creating all possible overloads of a function/method so all possible argument lists would be valid.

Saw the code and liked it. 👍
And other people at SAP can assure you that I'm very spare on compliments.

[RobertoMalatesta]

However I am not sure it is true to say that any instance where there is a sequence of optional arguments
can be replaced with type unions combos because not all implementations actually
perform these type checks and assignments at runtime.

Hm. Interesting.
Do you have an example?

[bd82]

Regarding Incorrect / Imprecise docs:

These are interesting use cases, But we have to consider what is the scope of this project?

• Should fixing the content of the OpenUI5 docs be a part of it?
• Should we apply heuristics that would add 2 "bugs' for every 8 issues fixed?
  Also consider that in the longer term this project would hopefully not be limited to only OpenUI5...

My personal opinion is that many of these things should be outside the scope
of this project, unless there are some very low hanging fruits (e.g all constructor signatures in OpenUI5 are slightly incorrect).

@codeworrior , @petermuessig what do you think?

Regarding sequence of optional parameters

I think I had some examples in an internal discussion we had months ago,
I will look for those. Anyhow this is a specific case of the scope question:
The author may have had 2 possible intents, should we guess the real intent or should
the author properly document his or hers code?

We already have quite a bit of complexity here in aligning UI5 type system assumptions with TypeScript's more robust type system, do we want to add more?

[bd82]

I could not find a specific example for sequence of optional parameters but I found a similar example
demonstrating the mis-match between the definition and implementation (this may have been fixed since...0

• scrollToElement
  • In JSDocs, 2nd argument is defined as optional and 3rd argument is mandatory.
    This can be done in JavaScript, but it requires some runtime magic to distinguish the arguments.
  • The implementation does not perform any such runtime magic so in actuality the 2nd parameter is
    mandatory!
  • Also note that the 3rd argument is implemented as optional even though it is defined as mandatory.
• attachValidationSuccess
  • In this case the "runtime magic" was actually implemented to properly support interweaving optional and none optional arguments.
  • This is equivalent to function overloading.

The point I am trying to demonstrate is that I cannot guess the intent of the author.
In some cases there is special runtime handling of different orders of call arguments.
In other cases there are none.

[bd82]

After some more thinking these are my conclusions:

I don't think such a heuristic should be part of the dts-generator as I believe those kinds of errors
should be fixed originally in the UI5 Docs instead of adding even more complexity here.

I do believe we can help the UI5 teams to fix their Docs by creating a report/log where we detect
cases where incorrectly documented optional parameters.

I will open a new issue for the report.

[bd82]

#25