Warn on unused property in object literal with spread

[ghost]

TypeScript Version: 2.9.0-dev.20180411

Search Terms: object spread unused property

Code

interface Point { x: number; y: number; }
function withX(p: Point, x: number): Point {
    if (something) {
        return { x, x: p.x, y: p.y }; // Error
    } else {
        return { x, ...p }; // No error
    }
}

Expected behavior:

Error at { x, ...p }: The first x is dead because ...p has its own non-optional x.

Actual behavior:

No error at { x, ...p }.

[ajafff]

Since this will never result in an error at runtime, it should better be implemented by a linter.
Given the previous discussion about avoiding lint-like errors in the type checker, I don't think TypeScript is the right place for such a rule.

[ajafff]

@andy-ms I actually liked the idea, so I implemented it as a lint rule. The rule also covers the case where all properties of a spreaded object are overridden later in the literal.
I'd like your feedback on the PR fimbullinter/wotan#185.

On a related note: I also implemented a rule to enforcereturn or throw on function calls that return never: fimbullinter/wotan#159. This was inspired by your changes in #22922

[RyanCavanaugh]

Seems like a good idea even though it's technically a breaking change.

Accepting PRs, should be easy enough for anyone familiar with the code or maybe @ajafff wants to port over the lint logic?

[ghost]

@sandersn Might have some implementation comments -- note that we are somehow detecting dead properties as the following compiles:

const o: { x: number } = { x: "", ...{ x: 0 } };

@ajafff's solution at https://github.com/fimbullinter/wotan/pull/185/files#diff-99a6bc6475c02de7f607e3fe08ae7be0 is also pretty short.

[sandersn]

Couple of observations:

1. This error would only be useful with strict null checks. p: Partial<Point> is the main use-case for object spread. I guess you could use optionality as the sole signal with "strictNullChecks" false, but that seems too restrictive when the values could themselves be null or undefined.
2. The implementation does indeed know when you are overwriting or combining a property. The error would be easy to add, I think.

[ghost]

@sandersn But we only care about whether the properties are present, not whether they're undefined. { x: 0, ...{ x: undefined } } is { x: undefined }.

[ajafff]

Re: optionality with strictNullChecks disabled
I needed to disable the rule if strictNullChecks is not enabled, because of the following case:

let x = 1;
({ x, ...Boolean() && {x} });

The type of the spreaded expression was {x: number} instead of false | {x: number}. That causes false positives.
I don't know if that's actually a bug.

[ghost]

@ajafff See #20090

[ghost]

You're right though, we should disable this in non-strictNullChecks because the spreaded object may itself be undefined and we wouldn't know.

function f(o: { x: number } | undefined) {
    return { x: "", ...o }; // `{ x: "" }` is *not* dead
}

[RyanCavanaugh]

This is fixed now