feat(eslint-plugin): [strict-enums] new rule #4864

Zamiell · 2022-04-26T03:46:34Z

PR Checklist

Addresses an existing open issue: Rule proposal: Disallow literals where enums are expected #308
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Makes working with enums not suck. See the docs that I wrote for the rule.

This PR is a draft; I am still working on it but I will start a PR for now in case anyone wants to give me early feedback.

nx-cloud · 2022-04-26T03:46:36Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 871e9ca. 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

✅ Successfully ran 43 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-04-26T03:46:40Z

Thanks for the PR, @Zamiell!

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-04-26T03:46:41Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`871e9ca`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6296c3849d15340008441833
😎 Deploy Preview	https://deploy-preview-4864--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.

codecov · 2022-04-26T03:56:37Z

Codecov Report

Merging #4864 (a50f6e1) into main (7275977) will decrease coverage by 2.72%.
The diff coverage is 81.85%.

❗ Current head a50f6e1 differs from pull request most recent head 871e9ca. Consider uploading reports for the commit 871e9ca to get more accurate results

@@            Coverage Diff             @@
##             main    #4864      +/-   ##
==========================================
- Coverage   94.25%   91.53%   -2.73%     
==========================================
  Files         153      228      +75     
  Lines        8305    10856    +2551     
  Branches     2702     3351     +649     
==========================================
+ Hits         7828     9937    +2109     
- Misses        263      637     +374     
- Partials      214      282      +68

Flag	Coverage Δ
unittest	`91.53% <81.85%> (-2.73%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/configs/all.ts	`100.00% <ø> (ø)`
...ackages/eslint-plugin/src/rules/no-implied-eval.ts	`97.01% <ø> (-0.05%)`	⬇️
packages/eslint-plugin/src/rules/return-await.ts	`99.05% <ø> (ø)`
packages/eslint-plugin/src/rules/unbound-method.ts	`92.13% <0.00%> (ø)`
packages/eslint-plugin/src/util/misc.ts	`93.75% <ø> (-0.25%)`	⬇️
packages/type-utils/src/getEnum.ts	`14.28% <14.28%> (ø)`
...s/eslint-plugin/src/util/collectUnusedVariables.ts	`93.16% <50.00%> (ø)`
packages/type-utils/src/typeFlagUtils.ts	`65.38% <55.55%> (ø)`
packages/eslint-plugin/src/rules/strict-enums.ts	`84.43% <84.43%> (ø)`
...t-plugin/src/rules/no-confusing-void-expression.ts	`98.83% <100.00%> (-0.02%)`	⬇️
... and 86 more

Zamiell · 2022-04-26T04:08:12Z

No idea why linting is failing in CI. It passes locally when I do npm run lint.

Zamiell · 2022-04-28T13:57:36Z

I also found a bug with the typeChecker.getTypeAtLocation method while writing this PR.
If you pass it ts.VariableDeclaration instead of ts.BindingName, it will cause a run-time error.

Which means that the function signature of this method needs to be updated in the TypeScript repository.
This is over my pay grade to fix, but someone should look into this.

test

.vscode/settings.json

also fixes for codebase

Zamiell · 2022-04-29T20:27:09Z

Some notes:

I fixed a few bad JSDoc comments (@return --> @returns). You should add a lint rule for this to the project, I recommend: https://github.com/gajus/eslint-plugin-jsdoc
As per Josh's recommendation, I fixed some bugs in the rest of the codebase that this lint rule caught, which is why there are seemingly unrelated code changes in this PR.
I converted all tsutils.isTypeFlagSet to util.isTypeFlagSet, as the former is an unmaintained 3rd-party API, and the latter is a (presumably-still-maintained) internal function. I also fixed the definition for this function, as it really takes an integer instead of an enum.
I made util.isSymbolFlagSet, then converted all tsutils.isSymbolFlagSet to util.isSymbolFlagSet for similar reasons, and did some small refactoring in "typeFlagUtils.ts" to make things more clear.

Needs to still be addressed:

CUSTOM_RULES.md

The documentation here recommends using tsutils.isTypeFlagSet, but this code won't work because the type signature for that function is bugged and the library is no longer maintained. Thus, I think util.isTypeFlagSet should be moved to the @typescript-eslint/utils or something, because right now it isn't exported for end-users to use (I think?). And then this file should be updated to use the new exported function.

Here's a list of errors that I ignored:

semi.ts

This file has code that uses a reducer on an enum array, but with a starting value of {} to compose a new record. This is a non-standard way to build a record (I think?), since normally the reduce function is supposed to operate on the array elements themselves.

Thus, the strict-enums rule rightly complains that the reduce function is being used incorrectly.

no-redundant-type-constituents.ts

This uses a complex type of type PrimitiveTypeFlag = typeof primitiveTypeFlags[number], and I'm not sure what that even means.

no-implied-eval.ts

My first thought was to change this to InternalSymbolName.Function, but that doesn't actually match the value that is being used here, so I don't know what to do. I assume that should be a corresponding enum to use, I just don't know what it is.

The comment for the ___String type gives more information about this:

    /**
     * This represents a string whose leading underscore have been escaped by adding extra leading underscores.
     * The shape of this brand is rather unique compared to others we've used.
     * Instead of just an intersection of a string and an object, it is that union-ed
     * with an intersection of void and an object. This makes it wholly incompatible
     * with a normal string (which is good, it cannot be misused on assignment or on usage),
     * while still being comparable with a normal string via === (also good) and castable from a string.
     */
    export type __String = (string & {
        __escapedIdentifier: void;
    }) | (void & {
        __escapedIdentifier: void;
    }) | InternalSymbolName;

So I guess it's just weird and I should just put // eslint-disable-line?

isTypeReadonly.ts

Same as "no-implied-eval.ts".

no-implied-eval.ts

Same as "no-implied-eval.ts".

consistent-type-exports.ts

Same as "no-implied-eval.ts".

This monorepo is a good test case for finding bugs in the rule. For example, it caught a bug with variadic functions, which I didn't have a test case for originally. Still working on some of the other errors.

packages/eslint-plugin/docs/rules/strict-enums.md

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

bradzacher

This is looking really good to me as it currently stands.

HOWEVER, I do see a large DevX problem in terms of "flags" enums. Now, flags enums are known to be unsafe.
Personally, I think in JS you should be using a Set or array to store the a list of possible values, but I get why one might use a "flags" enum.:

they're often used is for memory efficiency.
- for example - this is why TS went with a "flags" enum for its fields - saves a lot of memory across an entire project's ASTs.
some would argue that there's a nice DevX to using bitwise-ops on flag enums.
- personally I think that they're a bit confusing to think about, though I can understand how they're familiar for someone comfortable with lower-level languages.

The point is though, this may be something we need to solve for in terms of devx.
Right now the answer is "too bad, so sad - union in number", but I don't think this is a good answer for those users because it destroys the type documentation for the usecases, for example:

Whilst I understand that in these cases it is "technically correct" for the type to be number, it's also not truly correct as it suggests things like "it's valid to pass 100 to this function", when it's not - because 100 satisfies neither Foo.A, nor Foo.B.

So with all that said, I think we need to determine a means for people to "opt out" of the rule for flags enums.

Perhaps it's something as simple as adding an option with an "allow flags naming pattern" similar to the no-unused-vars rule's varsIgnorePattern option:

{
  "@typescript-eslint/strict-enums": [
    "error",
    // treats any enum with name ending in "Flags" as a flags enum
    {"flagsEnumPattern": "Flags^"},
  ],
}

This way users can have the rule on for strict usecase, and opt-out (with a clear naming pattern) known "unsafe" usecases.

I know we'd certainly find value within this very codebase thinks to the TS flag enums!

WDYT?

bradzacher · 2022-05-30T05:14:10Z

packages/scope-manager/src/referencer/Reference.ts

@@ -65,7 +65,7 @@ class Reference {
  /**
   * In some cases, a reference may be a type, value or both a type and value reference.
   */
-  readonly #referenceType: ReferenceTypeFlag;
+  readonly #referenceType: number | ReferenceTypeFlag;


instead of unioning in number here, could we define TypeOrNumber as an enum member, and remove the bitwise operations on the enum?

bradzacher · 2022-05-30T05:18:26Z

packages/type-utils/src/typeFlagUtils.ts

 */
-export function getTypeFlags(type: ts.Type): ts.TypeFlags {
-  let flags: ts.TypeFlags = 0;
+export function getTypeFlags(type: ts.Type): number | ts.TypeFlags {


@JoshuaKGoldberg the point @Zamiell was making was that the code is written like this:

getTypeFlags(flag.one | flag.two | ...);

when could be written like this:

getTypeFlags(new Set([flag.one, flag.two, ...]);

packages/eslint-plugin/src/rules/strict-enums.ts

bradzacher · 2022-05-30T07:05:25Z

packages/eslint-plugin/src/rules/strict-enums.ts

+      VariableDeclaration(node): void {
+        for (const declaration of node.declarations) {


nitpick - we don't care about the declaration itself, just the declarators.

Suggested change

VariableDeclaration(node): void {

for (const declaration of node.declarations) {

VariableDeclarator(node): void {

This is good because it saves you manually iterating and it means your report will be cleaner.

I.e. instead of:

const x = 1, ^^^^^^^^^^^^ y: Foo = 99; ^^^^^^^^^^^^^^^^^

(where ^ is the report underline)

you'll instead have:

const x = 1, y: Foo = 99; ^^^^^^^^^^^

Which is clearer and more accurate!

packages/eslint-plugin/src/rules/strict-enums.ts

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Zamiell · 2022-05-31T19:37:08Z

There are branch conflicts in thre README.md files.

Why are there 2 duplicated README.md files? That doesn't make any sense.
Is there a script to generate the README.md tables?

Zamiell · 2022-06-01T00:58:13Z

Also, after Brad's updates, I get the following errors when building:

james@1qaz MINGW64 /d/Repositories/typescript-eslint/packages/eslint-plugin (strict-enums)
$ yarn build
yarn run v1.22.19
$ tsc -b tsconfig.build.json
../types/src/ts-estree.ts:1:27 - error TS2307: Cannot find module './generated
/ast-spec' or its corresponding type declarations.

1 import * as TSESTree from './generated/ast-spec';
                            ~~~~~~~~~~~~~~~~~~~~~~

../types/src/ts-estree.ts:4:16 - error TS2664: Invalid module name in augmenta
tion, module './generated/ast-spec' cannot be found.

4 declare module './generated/ast-spec' {
                 ~~~~~~~~~~~~~~~~~~~~~~

../types/src/ts-estree.ts:21:27 - error TS2307: Cannot find module './generate
d/ast-spec' or its corresponding type declarations.

21 export * as TSESTree from './generated/ast-spec';
                             ~~~~~~~~~~~~~~~~~~~~~~

../types/src/index.ts:1:49 - error TS2307: Cannot find module './generated/ast
-spec' or its corresponding type declarations.

1 export { AST_NODE_TYPES, AST_TOKEN_TYPES } from './generated/ast-spec';
                                                  ~~~~~~~~~~~~~~~~~~~~~~

../visitor-keys/src/get-keys.ts:1:26 - error TS2307: Cannot find module '@type
script-eslint/types' or its corresponding type declarations.

1 import { TSESTree } from '@typescript-eslint/types';
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~

../visitor-keys/src/visitor-keys.ts:1:42 - error TS2307: Cannot find module '@
typescript-eslint/types' or its corresponding type declarations.

1 import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/types';
                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~

../visitor-keys/src/visitor-keys.ts:152:62 - error TS2345: Argument of type 'A
dditionalKeys' is not assignable to parameter of type 'VisitorKeys'.
  'string' index signatures are incompatible.
    Type 'readonly GetNodeTypeKeys<T>[] | undefined' is not assignable to type
 'readonly string[]'.
      Type 'undefined' is not assignable to type 'readonly string[]'.

152 const visitorKeys: VisitorKeys = eslintVisitorKeys.unionWith(additionalKey
s);
                                                                 ~~~~~~~~~~~~~
~


Found 7 errors.

Anyone know why that is? I'm unable to work on the branch until that is resolved.

bradzacher

I think this is looking good so far.
I'm happy if we want to get this in and out to people to play with.

Fix the merge conflicts and we can get to it!

bradzacher · 2022-07-18T04:44:52Z

packages/eslint-plugin/docs/rules/strict-enums.md

+## Banned Patterns
+
+This rule bans:
+
+1. Enum incrementing/decrementing - `incorrectIncrement`
+1. Mismatched enum declarations/assignments - `mismatchedAssignment`
+1. Mismatched enum comparisons - `mismatchedComparison`
+1. Mismatched enum function arguments - `mismatchedFunctionArgument`


this is just a less-verbose duplicate of "Error Information" below - so let's remove this.

Zamiell · 2022-07-19T03:44:26Z

Brad can you respond to my messages on May 31st?

bradzacher · 2022-08-07T11:17:49Z

@Zamiell - both packages/eslint-plugin/README.md and packages/eslint-plugin/docs/rules/README.md no longer have rules table in them.

At the time the "canonical" table was the former (root readme), and the latter was added as part of the website build.

However the rules table is now no longer generated in source files - instead they are generated as part of the website build.

JoshuaKGoldberg · 2022-10-18T15:06:19Z

👋 @Zamiell, just checking in - do you still have time to work on this? And if so, are there still open questions you're waiting on answers to?

JoshuaKGoldberg · 2022-11-16T15:01:27Z

Closing this PR as it's been stale for a few months without activity. Feel free to reopen @Zamiell if you have time - but no worries if not! If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding Co-authored-by: Zamiell at the end of your PR description. Thanks! 😊

JoshuaKGoldberg · 2022-11-25T09:01:56Z

Moving to #6091. I want these checks to exist 😄

Zamiell marked this pull request as ready for review April 27, 2022 16:27

feat: adding enums to member-ordering rule

481fb44

Zamiell added 7 commits April 28, 2022 13:25

Merge branch 'main' into strict-enums

5776a01

feat(eslint-plugin): [strict-enums] adding check for null/undefined

094c90e

feat(eslint-plugin): [strict-enums] refactoring tests to separate files

311b1f8

feat(eslint-plugin): [strict-enums] allowing more operators

ac5e6d9

fix(eslint-plugin): [strict-enums] adding union test

7d55248

fix(eslint-plugin): [strict-enums] refactoring + adding failing class

74af3d1

test

fix(eslint-plugin): [strict-enums] adding constraint code

1eb70ee

JoshuaKGoldberg reviewed Apr 29, 2022

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

fix(eslint-plugin): [strict-enums] better eslint messages

cf006fd

Zamiell mentioned this pull request Apr 29, 2022

fix: add rule to vscode settings #4881

Merged

3 tasks

Zamiell added 9 commits April 29, 2022 08:59

Merge branch 'main' into strict-enums

4d7f3fe

fix(eslint-plugin): [strict-enums] removing vscode setting changes

2bbc2b7

Merge branch 'main' into strict-enums

75c2252

fix: changing function definition to reflect reality

3976fdc

fix: pass string enum literal into function that take string

2406d03

fix: allow passing enums to functions with any/unknown

4244376

fix: using flags instead of names

8b4dcdc

also fixes for codebase

fix: adding test that breaks the rule

12f8120

fix: adding test for variadic functions

e638718

Zamiell added 2 commits April 29, 2022 16:27

fix: adding isSymbolFlagSet internally

aef971a

fix: adding ignoring for remaining lint failures

75f60b9

Zamiell added 6 commits May 5, 2022 22:27

fix: adding failing test

2395998

fix: making boolean tests pass

2851c3b

Merge branch 'main' into strict-enums

149c049

fix: refactor tests + fix linter

cf7c9b7

fix: adding brads tests

3b2fa57

fix: brads tests now pass

6c43f71

Josh-Cena reviewed May 7, 2022

View reviewed changes

packages/eslint-plugin/docs/rules/strict-enums.md Outdated Show resolved Hide resolved

Zamiell and others added 2 commits May 7, 2022 13:00

Update packages/eslint-plugin/docs/rules/strict-enums.md

af7d5da

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

Merge branch 'main' into strict-enums

a50f6e1

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label May 16, 2022

bradzacher reviewed May 30, 2022

View reviewed changes

Zamiell and others added 3 commits May 30, 2022 03:41

Update packages/eslint-plugin/src/rules/strict-enums.ts

85563be

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Update packages/eslint-plugin/src/rules/strict-enums.ts

ee96ce5

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Update packages/eslint-plugin/src/rules/strict-enums.ts

943bf9f

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 31, 2022

Zamiell added 2 commits May 31, 2022 20:59

fix: make brads updates actually compile

7679c4a

Update strict-enums.ts

871e9ca

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 13, 2022

bradzacher approved these changes Jul 18, 2022

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jul 18, 2022

JoshuaKGoldberg closed this Nov 16, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022

Zamiell deleted the strict-enums branch September 17, 2023 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [strict-enums] new rule #4864

feat(eslint-plugin): [strict-enums] new rule #4864

Zamiell commented Apr 26, 2022 •

edited by Josh-Cena

Loading

nx-cloud bot commented Apr 26, 2022 •

edited

Loading

typescript-eslint bot commented Apr 26, 2022

netlify bot commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

Zamiell commented Apr 26, 2022 •

edited

Loading

Zamiell commented Apr 28, 2022 •

edited

Loading

Zamiell commented Apr 29, 2022 •

edited

Loading

bradzacher left a comment

bradzacher May 30, 2022

bradzacher May 30, 2022

bradzacher May 30, 2022

Zamiell commented May 31, 2022 •

edited

Loading

Zamiell commented Jun 1, 2022

bradzacher left a comment

bradzacher Jul 18, 2022

Zamiell commented Jul 19, 2022

bradzacher commented Aug 7, 2022 •

edited

Loading

JoshuaKGoldberg commented Oct 18, 2022

JoshuaKGoldberg commented Nov 16, 2022

JoshuaKGoldberg commented Nov 25, 2022

		VariableDeclaration(node): void {
		for (const declaration of node.declarations) {

	VariableDeclaration(node): void {
	for (const declaration of node.declarations) {
	VariableDeclarator(node): void {

feat(eslint-plugin): [strict-enums] new rule #4864

feat(eslint-plugin): [strict-enums] new rule #4864

Conversation

Zamiell commented Apr 26, 2022 • edited by Josh-Cena Loading

PR Checklist

Overview

nx-cloud bot commented Apr 26, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Apr 26, 2022

netlify bot commented Apr 26, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Apr 26, 2022 • edited Loading

Codecov Report

Zamiell commented Apr 26, 2022 • edited Loading

Zamiell commented Apr 28, 2022 • edited Loading

Zamiell commented Apr 29, 2022 • edited Loading

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher May 30, 2022

Choose a reason for hiding this comment

bradzacher May 30, 2022

Choose a reason for hiding this comment

bradzacher May 30, 2022

Choose a reason for hiding this comment

Zamiell commented May 31, 2022 • edited Loading

Zamiell commented Jun 1, 2022

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Jul 18, 2022

Choose a reason for hiding this comment

Zamiell commented Jul 19, 2022

bradzacher commented Aug 7, 2022 • edited Loading

JoshuaKGoldberg commented Oct 18, 2022

JoshuaKGoldberg commented Nov 16, 2022

JoshuaKGoldberg commented Nov 25, 2022

Zamiell commented Apr 26, 2022 •

edited by Josh-Cena

Loading

nx-cloud bot commented Apr 26, 2022 •

edited

Loading

netlify bot commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

Zamiell commented Apr 26, 2022 •

edited

Loading

Zamiell commented Apr 28, 2022 •

edited

Loading

Zamiell commented Apr 29, 2022 •

edited

Loading

Zamiell commented May 31, 2022 •

edited

Loading

bradzacher commented Aug 7, 2022 •

edited

Loading