playcanvas / engine

Powerful web graphics runtime built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.73k stars 1.36k forks source link

Investigate function parameter validation for public Engine API #2529

Open mvaligursky opened 4 years ago

mvaligursky commented 4 years ago

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:

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

kungfooman commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

What stops them to use DEBUG build in such case?

mvaligursky commented 3 years ago

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?