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

deleting a prop doesn't invalidate the interface #13783

Closed
donaldpipowitch opened this issue Jan 31, 2017 · 33 comments · Fixed by #37921
Closed

deleting a prop doesn't invalidate the interface #13783

donaldpipowitch opened this issue Jan 31, 2017 · 33 comments · Fixed by #37921
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@donaldpipowitch
Copy link
Contributor

TypeScript Version: 2.1? (Playground)

Code

interface Props {
    foo: string;
    bar: string;
}

const p: Props = {
    foo: 'hello',
    bar: 'world'
}

const say = (props: Props) => console.log(`${props.foo.length} ${props.bar.length}`);

say(p);

delete p.bar;
say(p); // works, but should throw

See here.

Expected behavior:

Don't compile, because we delete bar from p.

Actual behavior:

Compiles. Resulting in a runtime error: Uncaught TypeError: Cannot read property 'length' of undefined.

@ahejlsberg
Copy link
Member

To track effects of delete you need to compile with --strictNullChecks and declare the property optional. Upon seeing delete p.bar the control flow analyzer will then consider p.bar to have the value undefined.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jan 31, 2017
@donaldpipowitch
Copy link
Contributor Author

I do use --strictNullCheck, but I don't see why this (and declaring the property) is optional. Why is this working as intended? E.g. this won't compile:

interface Props {
    foo: string;
    bar: string;
}

const p: Props = {  // an error happens here, because `p` is not assignable to type `Props`
    foo: 'hello'
}

const say = (props: Props) => console.log(`${props.foo.length} ${props.bar.length}`);

say(p);

Using delete p.bar changes the p from my initial example essentially to a p without bar as in my second example. And a p without bar doesn't match Props, so it shouldn't be possible to call say with it.

Or am I wrong?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2017

this compiles too without --strictNullChecks:

const p: Props = {  
    foo: 'hello'
    bar: undefined
}

and will result in the same runtime behavior.

@donaldpipowitch
Copy link
Contributor Author

I try to summarise:

Given we have the following interface, function and using --strictNullChecks:

interface Props {
    foo: string;
    bar: string;
}

const say = (props: Props) => console.log(`${props.foo.length} ${props.bar.length}`);

This is invalid:

const p = {
    foo: 'hello',
    bar: undefined
};
say(p);

This is invalid:

const p = {
    foo: 'hello'
};
say(p);

This is invalid:

const p = {
    foo: 'hello',
    bar: 'world'
};
p.bar = undefined;
say(p);

But this is valid and intended?:

const p = {
    foo: 'hello',
    bar: 'world'
};
delete p.bar;
say(p);

That means I'd need to mark every property in every interface as optional, if I'd want nobody to accidentally delete a required property. Isn't that more of a bug instead of an intended solution?


Note that this works, too. But it is probably also a bug.

const p1 = {
    foo: 'hello',
    bar: 'world'
};
const p2 = {
    ...p1,
    bar: undefined
};
say(p2);

@mhegazy mhegazy closed this as completed Apr 21, 2017
@donaldpipowitch
Copy link
Contributor Author

@mhegazy you closed the issue, so this works as intended? Looking at my examples from three months ago this still looks wrong to me.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 21, 2017

The last case is tracked by #13195.

@mhegazy mhegazy reopened this Apr 21, 2017
@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Apr 21, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Apr 21, 2017

for this case, with --strictNullChecks:

const p = {
    foo: 'hello',
    bar: 'world'
};
delete p.bar;
say(p);

I do not think control flow analysis is the right tool here, but we could disallow deleting a property whose type does not include undefined; treeing it the same way as p.bar = undefined;.

@donaldpipowitch
Copy link
Contributor Author

Thank you!

@jmlopez-rod
Copy link

I think treating delete p.bar as p.bar = undefined is the right approach since they both do the same thing.

@w0rp
Copy link

w0rp commented Aug 22, 2017

I think treating delete p.bar as p.bar = undefined is the right approach since they both do the same thing.

No, they have different semantics.

var x = {}
'y' in x // false
x.y = undefined
'y' in x // true
delete x.y
'y' in x // false

TypeScript currently lets you delete properties from objects even if they are non-optional, and even if you have every kind of option on like --strictNullChecks, --noImplicitAny, or whatever.

interface Foo {
    y: number
}

const x: Foo = { y: 3 }

delete x.y
// Now x is no longer a 'Foo', with no warnings or errors.

I'd class that as a bug.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

TypeScript currently lets you delete properties from objects even if they are non-optional, and even if you have every kind of option on like --strictNullChecks, --noImplicitAny, or whatever.

i agree. delete should no be allowed under --stricitNullChecks if the property does not include undefined (the type system does not make a distinction between properties that are optional and those that are possibly undefined).

@alex3165
Copy link

alex3165 commented Sep 18, 2017

The best way I found to remove a prop from the object and infer a new object type is to use es6 destruct and rest features:

interface Props {
    foo: string;
    bar: string;
}

const p: Props = {
    foo: 'hello',
    bar: 'world'
}

const say = (props: Props) => console.log(`${props.foo.length} ${props.bar.length}`);

say(p);

const { bar, ...newP } = p;
say(newP);

But I agree delete on a non optional property should be forbidden.

@AlexAtHome
Copy link

AlexAtHome commented Jan 31, 2019

This issue still appears at 3.2.4 version. Any chances to get it fixed atm?

@kimroen
Copy link

kimroen commented Feb 20, 2019

I ran into this today and was surprised:

const myObject = {
  myProperty: 'content',
  myOtherProperty: 'other content',
};

delete myObject.myProperty;

myObject.myProperty.padStart(2); // => Runtime error

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Feb 25, 2019
@ryanberckmans
Copy link

TypeScript currently lets you delete properties from objects even if they are non-optional, and even if you have every kind of option on like --strictNullChecks, --noImplicitAny, or whatever.

This behavior surprised me, too.

@kkaefer
Copy link

kkaefer commented Jan 15, 2020

Facebook's Flow added checking of delete statements in v0.109.

@Vanuan
Copy link

Vanuan commented Feb 24, 2020

So sad that typescript gives you a false sense of security.
Given this:

type SomeType = { a: string };
const someObject: SomeType = {
  a: string;
}

This code

delete someObject.a;

should be treated the same as casting someObject to Omit<SomeType, 'a'> and assigning to a variable of SomeType

@WORMSS
Copy link

WORMSS commented Feb 24, 2020

So sad that typescript gives you a false sense of security.
Given this:

type SomeType = { a: string };
const someObject: SomeType = {
  a: string;
}

This code

delete someObject.someProperty;

should be treated the same as casting someObject to Omit<SomeType, 'a'> and assigning to a variable of SomeType

That rule would then fail with

delete someObject.a;
someObject.a= 'a'; // someProperty does not exist on Omit<SomeTime, 'a'>;

@Vanuan
Copy link

Vanuan commented Feb 24, 2020

That rule would then fail with
delete someObject.a;
someObject.a= 'a'; // someProperty does not exist on Omit<SomeTime, 'a'>;

Nope, someObject type shouldn't be changed as a result of the delete operation. Type casting should only be used to validate the delete operation. I.e. whether someObject would be valid if we assign it to an object without a property.

So if a is not optional:

delete someObject.a; // Error: deleting non-optional property `a`
                     // (e.g. can't assign an object of type
                     // Omit<SomeType, 'a'> to `SomeType`)
someObject.a= 'a'; // `a` can be safely re-assigned, so this operation is valid

If a is optional:

delete someObject.a; // Success: deleting optional property `a`.
                     // Omit<SomeType, 'a'> is essentially the same as `SomeType`
                     // if a is optional
someObject.a= 'a'; // `a` can be safely re-assigned, so this operation is valid

If the type is not specified:

const someObject = {};
someObject.a = ''; // Property 'a' does not exist on type '{}'.

@Vanuan
Copy link

Vanuan commented Feb 24, 2020

So, delete someObject.a boils down to:

const discardedVar: SomeType = someObject as Omit<SomeType, 'a'>;

Which would succeed if a exists in SomeType and is optional. And would fail if a doesn't exist in SomeType or is not optional.

@Vanuan
Copy link

Vanuan commented Feb 24, 2020

So, if this is an implementation of the delete type checking:

        function checkDeleteExpression(node: DeleteExpression): Type {
            checkExpression(node.expression);
            const expr = skipParentheses(node.expression);
            if (!isAccessExpression(expr)) {
                error(expr, Diagnostics.The_operand_of_a_delete_operator_must_be_a_property_reference);
                return booleanType;
            }
            if (expr.kind === SyntaxKind.PropertyAccessExpression && isPrivateIdentifier((expr as PropertyAccessExpression).name)) {
                error(expr, Diagnostics.The_operand_of_a_delete_operator_cannot_be_a_private_identifier);
            }
            const links = getNodeLinks(expr);
            const symbol = getExportSymbolOfValueSymbolIfExported(links.resolvedSymbol);
            if (symbol && isReadonlySymbol(symbol)) {
                error(expr, Diagnostics.The_operand_of_a_delete_operator_cannot_be_a_read_only_property);
            }
            return booleanType;
        }

The proper delete validation should be something like this:

...
const symbol = getExportSymbolOfValueSymbolIfExported(links.resolvedSymbol);
if (symbol && isReadonlySymbol(symbol)) {
  error(expr, Diagnostics.The_operand_of_a_delete_operator_cannot_be_a_read_only_property);
}
if (symbol && !isOptionalSymbol(symbol)) {
  error(expr, Diagnostics.The_operand_of_a_delete_operator_cannot_be_a_non_optional_property);
}

Where isOptionalSymbol should be similar to isReadonlySymbol. Although, optional is probably a type inference property, not a syntax flag?

Well, we should probably try implementing some ESLint rule first using the discarded variable casting approach above. But I'm not sure whether typescript exposes required API. And where to even start.

@Mouvedia
Copy link

Mouvedia commented Mar 12, 2020

I am probably in the minority here but I think that delete object.a; should be allowed but it should be invalid to pass/assign object as FooType unless you add back that required a property.

i.e. it should throw on the next bar(object); or const qux: FooType = object; not on the delete line which should be considered a transitionary state

@Vanuan
Copy link

Vanuan commented Mar 12, 2020

AFAIK, TypeScript doesn't have a notion transitionary state. I.e. there's no Schrodinger type.

A variable value never changes type. Types are immutable.

You can't assign a field a type that is not in the list of allowed types. You can only "remove" types via typeguards but even typeguards don't change the type of a variable. It's still the same type when you pass an object to a function.

It makes sense that types are immutable and don't change in the course of the program. They're checked statically, during the build. That's the reason why we have unions: we need to specify all the types a give variable can be of. Because can't add them. We can only reduce (typeguard) them. But even then we can't assign an existing variable a new type. We can only create a new variable with new type.

The delete operator doesn't create a new variable. It's more like a function call on existing reference. Reference doesn't change its value. The value of reference is static and can't be changed.

Of course you can use typecasting, but that would be cheating.

@Mouvedia
Copy link

Mouvedia commented Mar 12, 2020

It's more like a function call on existing reference.

I disagree with your analogy.

Until the object is used to do something that actually needs to re-evaluate the type of that object, nothing is actually invalid in my book.

@Vanuan
Copy link

Vanuan commented Mar 12, 2020

@Mouvedia ok, what would be a type of a deleted property in this case, when passing it to console.log? At which line a type error should be shown?

const printNumber = (n: number) => console.log(n.toFixed());

interface ANumber {
  a: number;
}
const obj: ANumber = { a: 42 };
delete obj.a;
printNumber(obj.a);

@Mouvedia
Copy link

Mouvedia commented Mar 12, 2020

If I was using an IDE Id expect a wavy line under the argument of printNumber l8.
Not at l7 where the deletion is operated.

Because this

// snip
const obj: ANumber = { a: 42 };
delete obj.a;
obj.a = 42;
printNumber(obj.a);

should be allowed.

@rodrigok
Copy link

If such property isn't optional I don't think you should be able to delete it, if you need to change the value just assign the new value. Delete is "the same" of assign undefined to the property.

@Vanuan
Copy link

Vanuan commented Mar 12, 2020

This could be easily fixed like this:

type AnOptionalNumber { a?: number; }
const obj: ANumber = { a: 42 };
delete (obj as AnOptionalNumber).a;
obj.a = 42;
printNumber(obj.a);

That is if you know what you're doing you're welcome to shoot yourself in the foot. But by default, TypeScript should aim to prevent runtime errors, not tolerate some temporary untyped state of a variable.

@taueres
Copy link

taueres commented Apr 12, 2020

Deleting a property from an object is not exactly the same as setting undefined. Deleted properties are no longer enumerable.

interface ExampleType {
    foo: number,
    bar: number | undefined
}

const obj: ExampleType = { foo: 1, bar: 2 };

console.log(Object.keys(obj)); // [ 'foo', 'bar' ]

obj.bar = undefined; // This is allowed

console.log(Object.keys(obj)); // [ 'foo', 'bar' ]

delete obj.bar; // This should not be allowed as `bar` won't be enumerable

console.log(Object.keys(obj)); // [ 'foo' ]

@gztomas
Copy link

gztomas commented Apr 22, 2021

🤔 It seems the implementation from #37921 is not considering JS prototypes.
I mean, deleting a prop doesn't necessarily invalidate the interface, that would depend on the object prototype as well.

For instance:

const el = document.createElement('div');
el.getBoundingClientRect = () => new DOMRect();
delete el.getBoundingClientRect; // TS complains here, but this doesn't invalidate the interface, as 
                                 // `getBoundingClientRect` is still available from `el` prototype chain.

I wonder if there was a way to tell TS it's fine to delete own getBoundingClientRect prop, other than doing some casting, which wouldn't feel right.

@Vanuan
Copy link

Vanuan commented Apr 23, 2021

That would mean TypeScript needs a notion of "overridable" in addition to "optional". So that interface prop has a default value/type. Alternatively, TypeScript should only allow a subset of what's possible with JavaScript, which is kind of the point of TS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.