feat(eslint-plugin): add rule no-unused-type-properties

[eltonio450]

Hello everyone,

I tried to implement a rule to disallow unused type properties (#1529)

Our use case is to catch forgotten props when refactoring a React component, but I guess there are plenty of others. I included the example in the doc, but I copy paste it here:

type MyProps = {
  a: string,
  b: string //forgotten and now unused props
}


// Error: Destructuring does not use all of the properties of the type MyProps, you should either remove the properties from the type or use Omit<MyProps, ...> to explicitly exclude the properties.',

const MyComp = ({ a } : MyProps) => {
    return <div>{a}</div>
}

This is my first rule, so this PR is a draft (but working !).

I have a few questions :

Config:

• about the requiresTypechecking options : I am not really sure if I have to enable it or not...
• I could not really test my rule by linking @typescript-eslint/eslint-plugin package in some of my projects, did I miss something ? Should it work as any other package ?

Coding style:

• I saw that many rules don't use arrow function : is there a reason ? Should I comply to this or it does not matter ?
• are there some shared AST_NODE_TYPES typecheckers somewhere in the package ?

The rule itself

• I am not sure which case should I handle. For example, should I handle Omit or not ? Does this require to enable the requiresTypeChecking option ?
• I did not include the case where the type is exported, not sure if it makes sense
• should I include interface declaration in the check ?

I hope this rule makes sense and will be merged someday, feel free to share your remarks, I'll be glad to make the corrections !

All the best

[EDIT : deleting some questions, as I have found the answer while trying to increase coverage :)]

[typescript-eslint]

Thanks for the PR, @eltonio450!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #1936 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1936      +/-   ##
==========================================
+ Coverage   94.36%   94.39%   +0.03%     
==========================================
  Files         166      167       +1     
  Lines        7608     7657      +49     
  Branches     2185     2198      +13     
==========================================
+ Hits         7179     7228      +49     
  Misses        183      183              
  Partials      246      246              

Flag	Coverage Δ
#unittest	94.39% <100.00%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
...lint-plugin/src/rules/no-unused-type-properties.ts	100.00% <100.00%> (ø)

[eltonio450]

Hello,

@bradzacher what is the next step ? I think a good first one would be you to tell me if we should include interfaces in this rule :

interface MyProps {
  a: string,
  b: string //forgotten and now unused props
}


// Error: Destructuring does not use all of the properties of the interface MyProps, you should either remove the properties from the type or use Omit<MyProps, ...> to explicitly exclude the properties.',

const MyComp = ({ a } : MyProps) => {
    return <div>{a}</div>
}

I lack a bit of experience to really see if it fits in the spirit of a typescript rule.

If yes, I need a few more days to adapt the rule and messages

Thank you

[bradzacher]

https://github.com/typescript-eslint/typescript-eslint/blob/master/CONTRIBUTING.md#addressing-feedback-and-beyond

[eltonio450]

oopsi, sorry, I did not mean to be pushy or so 😢 , the question was real, and since the last time it took me two months to address your advice, I was like, ok, let's ask in advance this time 😄

if you have time in advance to give a piece of advice on this interface thing, great, if not, it will wait for the complete review 👍

thank you!

[bradzacher]

I don't think this rule belongs inside this plugin.
It's very specific to react - whilst there are non-react use cases that where this would apply, I think the vast majority would be react components.

I think it would be better as a rule in eslint-plugin-react:

• it would reach more people that would be specifically interested in the rule (the react plugin has 6m weekly downloads that are obviously react users, whereas we have 5m weekly downloads that are a mixture of front-end/backend and frameworks).
• it could then be generalised to handle flow as well (the two aren't so different), further increasing its reach.

---

That being said, answering your questions:

about the requiresTypechecking options : I am not really sure if I have to enable it or not...

You only need to enable it if you're using type information.
If you have to ask, you're not using it 😄

I could not really test my rule by linking @typescript-eslint/eslint-plugin package in some of my projects, did I miss something ? Should it work as any other package ?

I've never linked it, but it should just work after a build.

I saw that many rules don't use arrow function : is there a reason ? Should I comply to this or it does not matter ?

In this codebase, we tend to prefer function declarations where possible.

are there some shared AST_NODE_TYPES typecheckers somewhere in the package ?

What do you mean by this?

I am not sure which case should I handle. For example, should I handle Omit or not ? Does this require to enable the requiresTypeChecking option ?

Omit (well, any utility type) would require type information to handle correctly.
Any imported type would require type information to handle correctly.

I did not include the case where the type is exported, not sure if it makes sense

I think it still makes sense to report on this case.
The properties are for this component, and you don't want someone to provide extra properties to the component unless they're used.
Exporting the type just means others will consume the type, but they shouldn't dictate whether or not the component declares unused props.

should I include interface declaration in the check ?

Definitely - the rule should handle both type declarations and interface declarations.

[eltonio450]

Good guess for the eslint react repo, thank you :) :

I did not know we could write typescript specific rules, but yes, it's possible ! And even better, a rule for this use case exists: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-unused-prop-types.md (I thought it was specific to dynamic propTypes 😢, but the doc has been clarified lately )

Turns it it does not work (jsx-eslint/eslint-plugin-react#2569) but that's another story I guess

As for the PR, is it worth finalizing it based on your remarks ? I think the path is pretty clear now, thank you for the information, but does it add value ? If yes, I'll try do finish asap

For the type checkers, I have three functions in my PR :

//TODO(question): are there type checker somewhere ?
function isProperty(
  property: TSESTree.Property | TSESTree.RestElement,
): property is TSESTree.Property {
  return property.type === AST_NODE_TYPES.Property;
}

function isObjectPattern(
  param: TSESTree.Parameter,
): param is TSESTree.ObjectPattern {
  return param.type === AST_NODE_TYPES.ObjectPattern;
}

function isTypeAliasDeclaration(
  item: TSESTree.Statement,
): item is TSESTree.TSTypeAliasDeclaration {
  return item.type === AST_NODE_TYPES.TSTypeAliasDeclaration;
}

And I was wondering if I missed some shared utilities somewhere

Thank you !

[bradzacher]

For the type checkers, I have three functions in my PR :

Oh I get you, no we don't have ones for those.

All utilities are available via the utils namespace.

We can always add more to https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/experimental-utils/src/ast-utils/predicates.ts if we want those.

---

As for the PR, is it worth finalizing it based on your remarks ?

I don't think it does make sense to progress with this PR.
I think it would be better to fix the mentioned bug in the eslint-plugin-react rule, and use that rule instead.

Chances are you could probably also discuss with the maintainers and consume type information in that rule to make it more accurate (after first fixing the bug)

[eltonio450]

ok, closing then :)

thank you for your help along the process 👍