[no-delete-optional] Prohibit deleting optional properties

[Vanuan]

Feature request

{
  "rules": {
    "@typescript-eslint/no-delete-optional": ["error"]
  }
}

type SomeType = { a: string };
const someObject: SomeType = {
  a: string;
}
delete someObject.a;
someObject.a.toString();

Expected Result

Should throw a compile error deleting non-optional property 'a'

Actual Result

Runtime error can't read property 'toString' of undefined

[Vanuan]

Here are some implementation suggestions:
microsoft/TypeScript#13783 (comment)

[bradzacher]

This is probably better suited to be built into the compiler itself.
The TS team only has so much bandwidth, so it probably hasn't been prioritised yet for that reason. I would be surprised if they rejected a PR on it.

That being said, it wouldn't be hard to implement this as a lint rule, as long as strictNullChecks is on

[Vanuan]

Here are my considerations:

• I figure it is a breaking change so would require a version upgrade. TypeScript is released roughly 5 times a year (every 2-3 months) while eslint plugin might have a faster turnaround.
• Changing TypeScript is scary, I don't want to be responsible for breaking stuff for so many people using it. A new optional lint rule seems like a safer approach.
• Since it's an optional rule it can be turned off if it's implemented incorrectly. Suppressing a typescript error is a lot harder. Introducing a new option to TypeScript is kind of a big deal.

Do you have any suggestions on where to start?

[bradzacher]

I figure it is a breaking change so would require a version upgrade.

Correct, but TS releases breaking changes in minor bumps, so it would only be a few months until it gets released for everyone.
We've also got a 1 month+ turnaround on PRs here, so there's no guarantee it's faster 😅.

I don't want to be responsible for breaking stuff for so many people using it

The TS maintainers wouldn't merge a PR if they weren't confident it won't break TS for anyone. They'll make sure you have enough test coverage. And then they merge something broken, it won't be you on the hook to fix it - it's then on the TS maintainers to fill in any gaps. Such is the burden of being an open source maintainer.

An example of a breaking change I recently made to TS, which will be rolled out with v3.9
microsoft/TypeScript#36636
No doubt that change will cause pains for a lot of people, but it is a net positive, so people will fix their bugs and get over it 😄

Note that your change will be available quickly in the nightly builds, and a month or so before release in the beta and then RC builds. So there's much time for the change to be validated and fixed if it needs to be.

---

Happy to accept a PR if you want to implement this as a rule here first.
The benefit of a rule is that it's backwards compatible, so everyone on <3.xx would be able to use the feature. Many codebases are stuck on older versions of TS, so this is a good thing for the community. But it does mean your rule has a shelf life; we wouldn't want a rule to exist that duplicates a TS error.

[Vanuan]

I guess it should look something like this.

    const parserServices = util.getParserServices(context);
    const checker = parserServices.program.getTypeChecker();

    return {
      'UnaryExpression[operator=delete]'(node: TSESTree.UnaryExpression): void {
        if (
          node.argument.type !== AST_NODE_TYPES.MemberExpression ||
          isValidDelete(node.argument, checker)
        ) {
          return;
        }

        context.report({
          messageId: 'deleteOptional',
          node: node.argument.property,
        });
      },
    };

...
function isValidDelete(memberExpression: TSESTree.MemberExpression): boolean {
  const memberNode = memberExpression.property;
  if (memberNode.type !== AST_NODE_TYPES.Identifier) {
    return true; // skip the cases rule doesn't handle
  }
  const memberName = memberNode.name
  const object = memberExpression.object;
  const objectType = getTypeFromNode(object, checker);
  const memberType = getObjectMember(objectType, memberName);
  return !isOptional(memberType);
}

const getObjectMember = (objectType: InterfaceDeclaration | TypeLiteralNode, memberName: string) => objectType.members.find((member: TypeElement) => member.name === memberName)

const isOptional = (member: TypeElement) => !!member.questionToken;

Am I going in the right direction?

[bradzacher]

it looks pretty good.

You can handle x['a'] as well as x.a though

const property = memberExpression.property;
if (memberExpression.computed && property.type === AST_NODE_TYPES.Identifier) {
    return true; // can't handle non-constant string property access
}
const name = property.type === AST_NODE_TYPES.Identifier ? property.name : property.value;

const objType = esTreeNodeToTSNodeMap.get(memberExpression.object);
const propType = objType.getProperty(name);
const propDecl = propType.getDeclarations();
return !isOptional(propDecl);

[Vanuan]

It looks like it would be included in TS 4.0:

microsoft/TypeScript#38173