Salsa type inference should find or types

[egamma]

From @alexandrudima on January 27, 2016 10:49

Testing #2218

function a1() {
    return 5;
}

function a2() {
    if (1) {
        return 5;
    } else {
        return '5';
    }
}

Hovering over a1() shows its return type is number. Hovering over a2() shows its return type is any. IMHO it should show number|string -- maybe this inference code was written before or types were introduced?

Copied from original issue: microsoft/vscode#2431

[billti]

In TypeScript this is an error of "unknown type". If the types are relatable, we return the type that is the super-type of all return expressions. We explicitly don't try and synthesize a type as this code pattern often indicates a bug.

[mhegazy]

@billti, we should reconsider this one. the reason we stop this in TS, is we want ppl to go look at their code and make sure this is what they really intend. for JS, we do not have this error, so it seems unjustified to just block inference from the return types.

[RyanCavanaugh]

We could easily make the checker say that the function returns number|string (instead of unknownType) but still issue an error (in TypeScript). I don't think that would have any meaningful impact to TypeScript. Seems reasonable?

[billti]

I'm wary of adding complexity to the code base and diverging the TypeScript and JavaScript behavior too greatly. If it's simple enough that the benefit outweighs the cost, sounds good to me.

I'd note though that for compatible types we already return the most common super type (which effectively gives you the same completions as a union type). For cases when the types are truly distinct (e.g. the number and string scenario above), is a union type going to be of much value? I suppose if folks are using type guards, or we add better narrowing going forward...

[alexdima]

Thank you! 👍