Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate function parameter validation for public Engine API #2529

Open
mvaligursky opened this issue Nov 4, 2020 · 5 comments · May be fixed by #5817
Open

Investigate function parameter validation for public Engine API #2529

mvaligursky opened this issue Nov 4, 2020 · 5 comments · May be fixed by #5817

Comments

@mvaligursky
Copy link
Contributor

To help engine customers catch issues early instead of getting unhandled exceptions / undefined behavior, we should consider validating parameters passed into public API functions / possibly some internal functions.

Validation:

  • should run in the debug mode only to not impact performance
  • could be specified outside of the function and code generated in debug mode to do validation
  • ideally without having to add #ifdef
  • unified approach with unified error reporting
  • ideally should check parameter types, unexpected null / undefined parameters and similar

And likely some other validations - please add comments with ideas and suggestions on how this could be implemented.

@kungfooman
Copy link
Collaborator

kungfooman commented Nov 13, 2020

It could make use of prototype extending:

image

All the required code could be autogenerated from the playcanvas.d.ts:

var original = pc.Entity.prototype.addChild;
pc.Entity.prototype.addChild = function(node) {
    if (!node) {
        console.trace("node is undefined");
        return;
    }
    original.call(this, node);
}

Otherwise I only see AST rewriting as an option (during DEBUG) with a parsed playcanvas.d.ts as "source of truth" (of whats allowed or not, like missing non-optional arguments cause a trace - howsoever trace is implemented/integrated into the UI).

@Maksims
Copy link
Contributor

Maksims commented Nov 13, 2020

Common issue I found, is when error trace shows playcanvas.js with weird error message, it is then considered as engine bug by the developer.
So better error messages only in DEBUG engine (which is default when launching from Editor) - will definitely improve it.

But it is important that it does not alter the behaviour of function, such as:

func(arg) {
    // #ifdef DEBUG
    if (arg === null) {
        console.warn('arg should not be a null');
        return; // - not good
    }
    // #endif

    return arg;
}

Here it will return undefined in DEBUG version and return null in non-DEBUG version. So this define should only be a non-logic defining, and then it should fail as usual.

@mvaligursky
Copy link
Contributor Author

some developers use integrations that grab errors reporting by the engine to console, to be able to keep an eye on their product behavior in the wild. For these developers it would probably be better to have critical errors reported at all times? @yak32 - would you know more about this?

Perhaps we need #ERRORS and have build with a without it?

@Maksims
Copy link
Contributor

Maksims commented Jan 14, 2021

What stops them to use DEBUG build in such case?

@mvaligursky
Copy link
Contributor Author

Well performance - occasionally we run slower code in #DEBUG - like _checkFbo for example, but few others.
But also spamming .. in debug mode we can on occasion print many low importance "errors".
And if we do some API calls validation, ideally that would only be in DEBUG mode, and only print very critical errors in non-debug mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants