feat(eslint-plugin): [require-object-type-annotations] add rule #5666

OliverJAsh · 2022-09-16T20:34:06Z

PR Checklist

Addresses an existing open issue: fixes [explicit-literal-value] new rule for nominal typing (require type annotations) #1467
More context in Rule suggestion: require type annotations for object literals #3408
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Problem

I've added this section to elaborate on the problem I described in #3408.

For object literals without type annotations, many TypeScript features don't work. Contrived example:

type User = { name: string; age: number };

const makeUser = () => ({ name: 'bob', age: 123 });

If you try to rename a property inside of the object literal (or vice versa, the property inside the type definition), it won't update the name of the corresponding property in the User type definition. This is because TypeScript does not understand that this object literal relates to the User type.

For the same reason, other language server features like "go to definition" and "find references" on object properties will also not work.

When a type is used in many places, this makes navigation and refactoring of the code much more difficult.

Furthermore, this can result in bugs:

type Config = { foo: string; bar?: string };

const config = { foo: 'yay', bar: 'woo' };

If we rename bar in the Config type, not only are the usages not automatically renamed but also we don't get a type error to say that we're now setting an invalid property in config. In this situation it's likely we won't realise there is a problem, and this would probably cause a bug:

type Config = { foo: string; barNEW?: string };

// ❌ No type error!
const config = { foo: 'yay', bar: 'woo' };

Another example:

type State = { foo: string; bar: string };

declare const state: State;
const newState = { ...state, bar: 'woo' };

Rename bar in the State type and we get:

type State = { foo: string; barNEW: string };

declare const state: State;
// ❌ No type error!
const newState = { ...state, bar: 'woo' };

See #3408 for a real world example of this.

Note on debugging

VS Code makes it easy to identify when TypeScript understands the relationship between a value and its type because it will highlight all occurrences of a symbol when that symbol is selected:

All occurences of myString are highlighted:

Only one occurence of name is highlighted:

nx-cloud · 2022-09-16T20:34:08Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dd7c54a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

🟥 Failed Commands
`Node 18 - nx test @typescript-eslint/eslint-plugin`

✅ Successfully ran 45 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-09-16T20:34:09Z

Thanks for the PR, @OliverJAsh!

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.

netlify · 2022-09-16T20:34:10Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`dd7c54a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/63288adf5ba16e000867a435
😎 Deploy Preview	https://deploy-preview-5666--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

bradzacher · 2022-09-17T11:15:46Z

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

+      const type = checker.getTypeAtLocation(tsNode);
+
+      // Allow empty objects
+      if (type.getProperties().length === 0) {


Note that most types will have properties!
I believe that only the following types will not have any properties:

null

undefined

object

{}

It would probably be a good idea to check the type.flags for things like ts.TypeFlags.StringLike, ts.TypeFlags.NumberLike, etc to help weed out things we definitely want to ignore.

Is it possible to have an ObjectExpression node where the type is StringLike or NumberLike? 🤔

For context, I was only intending to catch {} with this branch.

bradzacher · 2022-09-17T11:21:00Z

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

+        contextualType === undefined ||
+        // Needed to catch object passed as a function argument where the parameter type is generic,
+        // e.g. an identity function
+        contextualType?.getSymbol()?.name === '__object'


I think you could do this with

(contextualType.flags & ts.TypeFlags.Intrinsic) === 0 && checker.typeToString(contextualType) === 'object'

Which is a little less abstract than using the __ string

Interesting, thanks! I will have to try that and see if my tests still pass.

It seems like Intrinsic doesn't exist? Also, what does "intrinsic" mean exactly?

OliverJAsh · 2022-09-17T16:16:11Z

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

+    const checker = parserServices.program.getTypeChecker();
+
+    return {
+      ObjectExpression: (esNode): void => {


Here's one thing I need help with:

I have this test case which currently produces 2 errors:

const xs = { ys: [{ prop: 1 }] };

This makes sense because we have two ObjectExpressions that have no contextual type.

However, I think it would be better if we only reported one error for the outermost ObjectExpression, seeing as the inner object would most likely have a contextual type as soon as we provide one to the outermost object.

I thought this might be possible if I could tell ESLint to stop traversing, but it seems like that's not possible: #2734. With that in mind, is there any other way we could do this? Perhaps there's another rule that is doing something similar which I can take a look at.

depends what the goal is for the inner object.
what is the plan for the user to "solve" this inner object?
with an as assertion? (I hope not!)
as that's the only solution - then you probably want to just ignore it entirely.

see my top level comment

with an as assertion? (I hope not!)

Certainly not 😛

what is the plan for the user to "solve" this inner object?

Once they annotate the outer object, the inner object will have a contextual type, meaning that language server features will work for properties on both the inside and outside objects i.e. "rename" / "go to definition" / "find references"). See the video below where where I've tried to demonstrate this.

As I described in #3408, the goal of this rule is to make sure TypeScript understands which type an object relates to, so that tools such as "rename" / "go to definition" / "find references" on object properties work properly and to avoid bugs. I think this can only be done by reading type information.

Screen.Recording.2022-09-19.at.08.29.56.mov

Here are some test cases for me to consider. Saving here so I don't lose it!

{ declare const fn: <T>(t: T) => T; // 2 errors const x = { prop: fn({ prop: 1 }) }; } { declare const fn: <T>(t: T) => T; // 1 error const x: { prop: { prop: number } } = { prop: fn({ prop: 1 }) }; } { declare const fn: (t: { prop: number }) => unknown; // 1 error const x = { prop: fn({ prop: 1 }) }; } { // 1 error const x = { prop: { prop: 1 } }; } { // 1 error const xs = { ys: [{ prop: 1 }] }; } { // 2 errors const x = { prop: () => { const y = { prop: 1 }; return y; }, }; } { // 1 error const x = { prop: () => ({ prop: 1 }), }; }

OliverJAsh · 2022-09-17T16:31:21Z

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

+    schema: [],
+    type: 'problem',
+  },
+  defaultOptions: [],


Continuing the thread from here: https://twitter.com/OliverJAsh/status/1570709040765997056

Sometimes we deliberately pass an object into a generic function and we don't want to annotate that object. io-ts example:

const T = t.type({ prop: t.number });

I want to add an option whereby users can define a whitelist of function names in which the error will not trigger.

The function name whitelisted by the user could be a member expression or it could just be an identifier:

ignoreFunctions: [ // Member expression 't.type', // Identifier 'foo' ]

I initially tried to do this using whitelist.includes(callee.getText()) but this of course breaks in complex cases such as:

const T = t // comment .type({ prop: t.number });

I'm not sure how to check whether the callee matches the whitelist—especially given we don't know whether the whitelisted function is a member expression (e.g. t.type) or just an identifier (e.g. foo).

Alternatively the rule option could ask for ESQuery selectors and then we check whether the selector matches the callee.

excludeCallees: [ 'MemberExpression[object.name="t"][property.name="type"]', 'Identifier[name="foo"]' ]

… but I'm not sure how to check whether a node matches a selector? Are there any examples of this in other rules?

#5271 is the current RFC for describing a standardized way to do that. It's tricky; we wouldn't want to allowlist all things named t by accident.

bradzacher

right now your rule is essentially saying "if object has no contextual type, report".
you are also using type information.

But I think you can achieve this in a different way - purely syntactically.

Specifically you're looking for these cases:

const variable = { object };
const variable = callExpr({ object });
const variable = call.expr({ object });

Which is entirely matchable via the AST!
Doing a pure AST match here means that (a) your rule will be faster and (b) you can match exact cases.

One thing to think about is this:

const variable = [{ object }];

Is this something the rule should handle?

bradzacher · 2022-09-19T06:13:40Z

packages/eslint-plugin/src/rules/require-object-type-annotations.ts

+    const checker = parserServices.program.getTypeChecker();
+
+    return {
+      ObjectExpression: (esNode): void => {


depends what the goal is for the inner object.
what is the plan for the user to "solve" this inner object?
with an as assertion? (I hope not!)
as that's the only solution - then you probably want to just ignore it entirely.

see my top level comment

OliverJAsh · 2022-09-19T07:25:03Z

@bradzacher

Specifically you're looking for these cases:

I think there are a few more cases to consider. I tried to demonstrate these in my tests. For example, function return types:

const fn = () => ({ prop: 1 });

Which is entirely matchable via the AST!

In that case how would you distinguish between these two cases that I've defined in the tests?

Invalid:

declare const f: <T>(x: T) => T;
f({ prop: 1 });

Valid:

declare const f: (x: { prop: number }) => unknown;
f({ prop: 1 });

Under this rule, any object that has a contextual type is valid, because a contextual type means that language server features will work (i.e. "rename" / "go to definition" / "find references" on object properties). Essentially we're just trying to make sure that the object is linked to a type somewhere.

Screen.Recording.2022-09-19.at.08.48.57.mov

One thing to think about is this:
const variable = [{ object }];

I think this is already covered by my tests:

Invalid:

const xs = [{ prop: 1 }];

Valid:

const xs: Array<{ prop: number }> = [{ prop: 1 }];

bradzacher · 2023-02-09T04:37:52Z

Okay so getting back to this - I definitely like the idea behind this.
I do think that we really need to be careful about how this works though as there's a lot of room for false positives if we don't get things right.

I think we're going to want to ensure we ignore all mapped types {[k in string]: v}, {k in 'union']: v} and types with index signatures {[k: string]: v}.

How should the rule handle any, object, {} and unknown - should it ignore them? I think so.

I think really we need to define the set of things we want to match against - which is just where an object falls into an anonymous object type, right?

The easy way to implement this will be to just get the contextual type of every object expression. Though it will be quite slow to go that route, I believe.
Idiomatic JS uses a lot of object literals, and if we have to look up the contextual type for every single one of those then we're going to burn all the cycles.

It's hard to say for sure though. Really need to test at scale on a "real" codebase.
Suggestion: implement the rule with just types, then perf test it with TIMING=all yarn eslint on a real codebase. If it's up there and looks to be a slow rule, then we can go the route of building in some static AST checks to circumvent the need for types.

Some perf optimisations

Cases we know for sure we can statically detect and ignore:

// variables
const a1: ObjType = { ... };

// functions with defined return types ***that aren't generics***
function b1(): ObjType { return { ... } }
const b2 = function (): ObjType { return { ... } }
const b3 = (): ObjType => ({ ... });
const b4 = (): ObjType => { return { ... } };

// function parameters with a default value AND a type ***that aren't generics***
function c(arg: ObjType = { ... }) {}

// assertions
const d1 = { ... } as ObjType;
const d2 = <ObjType>{ ... };
const d3 = { ... } satisfies ObjType;

// class props with types
class E {
  e: ObjType = { ... };
}

Cases we know are always unsafe, without needing types

// un-annotated objects -> no need to use types to check
const a1 = { ... };

// this sort of case is bad because they'll lead to anonymous types, regardless of what constraints exist on the generic
const b1 = <T,>(): T => ({ ... });

class C1 {
  // same as untyped variable
  c1 = { ... };
} 
// one caveat that *might be safe* - need use types for
class C2 extends C3 {
  c1 = { ... }; // might be typed on the parent class
}
class C4 implements C5 {
  c1 = { ... }; // might be typed on the interface
}

// untyped parameter defaults on a function declaration
function d1(arg = { ... }) {}
// but function expressions are hairy as you need to know if they are contextually typed - so should always check these
const d2: (arg: ObjType) => void = function d2(arg = { ... }) {}
const d3: (arg: ObjType) => void = (arg = { ... }) => {}

Probably more cases we can enumerate if needs be.

OliverJAsh · 2023-02-10T10:30:32Z

implement the rule with just types, then perf test it with TIMING=all yarn eslint on a real codebase.

We've been using this lint rule in production at Unsplash for about 6 months. Here's the data for the Unsplash codebase:

Rule                                             | Time (ms) | Relative
:------------------------------------------------|----------:|--------:
local/require-object-type-annotations            |  8621.609 |    60.2%
react/prefer-stateless-function                  |  1412.163 |     9.9%
@typescript-eslint/strict-boolean-expressions    |  1099.868 |     7.7%

As expected, the performance is far from ideal, but it's a price we're more than happy to pay.

Perhaps you have some ideas about how we could improve the performance of this version. I can run further tests if that helps!

Note the version we're using at Unsplash is a few steps ahead of the version in this PR: https://gist.github.com/OliverJAsh/ec82f0bfe9410b36dcf64a7000b713f6.

How should the rule handle any, object, {} and unknown - should it ignore them? I think so.

Agreed. I think the tests in my gist cover this.

I think really we need to define the set of things we want to match against - which is just where an object falls into an anonymous object type, right?

The invalid tests in my gist cover all of the cases I could think of. I don't have any tests for objects inside classes because we only use functions at Unsplash, so that's something you might want to add.

I worry that without type information this lint rule would have too many false negatives, but there's a good chance I'm wrong and I'm interested to see how it goes. Unfortunately I don't think I'm going to have much time to help with this though—especially considering we've effectively solved the problem at Unsplash and we're perfectly happy with the performance trade-off.

JoshuaKGoldberg · 2023-07-16T23:36:00Z

This PR has been stale for quite a while now. Although this is an interesting one with lots of potential, closing out inactive ones helps keep the queue clean. Closing just for housekeeping purposes.

If anybody picks up where this PR left off and uses code from it, please do add co-author credit.

Cheers!

OliverJAsh force-pushed the require-object-type-annotations branch 4 times, most recently from 5db0d5b to 3d1a2ff Compare September 16, 2022 21:39

bradzacher reviewed Sep 17, 2022

View reviewed changes

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Sep 17, 2022

bradzacher changed the title ~~Add new rule: require-object-type-annotations~~ feat(eslint-plugin): [require-object-type-annotations] add rule Sep 17, 2022

OliverJAsh added 5 commits September 17, 2022 17:03

Initial attempt

48dff89

Re-order

d375ab4

Document

4114b58

Inline

0b79c5e

Hoist

3935c66

OliverJAsh force-pushed the require-object-type-annotations branch from 3d1a2ff to 3935c66 Compare September 17, 2022 16:11

OliverJAsh requested a review from bradzacher September 17, 2022 16:12

OliverJAsh commented Sep 17, 2022

View reviewed changes

Document

043628b

bradzacher reviewed Sep 19, 2022

View reviewed changes

OliverJAsh requested review from JoshuaKGoldberg and bradzacher and removed request for JoshuaKGoldberg September 19, 2022 07:34

OliverJAsh added 3 commits September 19, 2022 16:15

Add test

082ec86

Add test

eec6fae

Add test

dd7c54a

OliverJAsh mentioned this pull request Sep 22, 2022

Rename / Find All References should find properties in indirectly contextually-typed object literals microsoft/TypeScript#50860

Open

kaczor6418 mentioned this pull request Oct 5, 2022

chore(typescript): convert project to use typescript kaczor6418/gdsc-website-template#11

Merged

bradzacher mentioned this pull request Feb 10, 2023

[explicit-literal-value] new rule for nominal typing (require type annotations) #1467

Closed

JoshuaKGoldberg closed this Jul 16, 2023

JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jul 16, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [require-object-type-annotations] add rule #5666

feat(eslint-plugin): [require-object-type-annotations] add rule #5666

OliverJAsh commented Sep 16, 2022 •

edited

Loading

nx-cloud bot commented Sep 16, 2022 •

edited

Loading

typescript-eslint bot commented Sep 16, 2022

netlify bot commented Sep 16, 2022 •

edited

Loading

bradzacher Sep 17, 2022

OliverJAsh Sep 17, 2022

bradzacher Sep 17, 2022

OliverJAsh Sep 17, 2022

OliverJAsh Sep 17, 2022 •

edited

Loading

bradzacher Sep 19, 2022

OliverJAsh Sep 19, 2022 •

edited

Loading

OliverJAsh Sep 20, 2022 •

edited

Loading

OliverJAsh Sep 17, 2022

JoshuaKGoldberg Sep 17, 2022

bradzacher left a comment

bradzacher Sep 19, 2022

OliverJAsh commented Sep 19, 2022 •

edited

Loading

bradzacher commented Feb 9, 2023

OliverJAsh commented Feb 10, 2023

JoshuaKGoldberg commented Jul 16, 2023

feat(eslint-plugin): [require-object-type-annotations] add rule #5666

feat(eslint-plugin): [require-object-type-annotations] add rule #5666

Conversation

OliverJAsh commented Sep 16, 2022 • edited Loading

PR Checklist

Problem

Note on debugging

nx-cloud bot commented Sep 16, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Sep 16, 2022

netlify bot commented Sep 16, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverJAsh Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverJAsh Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

OliverJAsh Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverJAsh commented Sep 19, 2022 • edited Loading

bradzacher commented Feb 9, 2023

OliverJAsh commented Feb 10, 2023

JoshuaKGoldberg commented Jul 16, 2023

OliverJAsh commented Sep 16, 2022 •

edited

Loading

nx-cloud bot commented Sep 16, 2022 •

edited

Loading

netlify bot commented Sep 16, 2022 •

edited

Loading

OliverJAsh Sep 17, 2022 •

edited

Loading

OliverJAsh Sep 19, 2022 •

edited

Loading

OliverJAsh Sep 20, 2022 •

edited

Loading

OliverJAsh commented Sep 19, 2022 •

edited

Loading