biomejs / biome

A toolchain for web projects, aimed to provide functionalities to maintain them. Biome offers formatter and linter, usable via CLI and LSP.
https://biomejs.dev
Apache License 2.0
15.39k stars 476 forks source link

💅 lint/suspicious/noEmptyInterface false positive #1157

Closed jer-sen closed 10 months ago

jer-sen commented 11 months ago

Environment information

CLI:
  Version:                      1.4.1
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.10.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "yarn/1.22.21"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Rule name

lint/suspicious/noEmptyInterface

Playground link

https://biomejs.dev/playground/?lintRules=all&code=aQBuAHQAZQByAGYAYQBjAGUAIABCACAAewAKACAAIABhADoAIABiAG8AbwBsAGUAYQBuACwACgAgACAAYgA6ACAAYgBvAG8AbABlAGEAbgAsAAoAfQAKAGkAbgB0AGUAcgBmAGEAYwBlACAAQQAgAGUAeAB0AGUAbgBkAHMAIABQAGkAYwBrADwAQgAsACAAJwBhACcAPgAgAHsAfQA%3D

Expected result

Should not report an error when extending something since the result is not an empty interface.

Code of Conduct

Conaclos commented 11 months ago

noEmptyInterface also reports empty interfaces that extend a type as showed in the diagnostic:

main.tsx:5:1 [lint/suspicious/noEmptyInterface](https://biomejs.dev/linter/rules/no-empty-interface)  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ An interface declaring no members is equivalent to its supertype.

    3 │   b: boolean,
    4 │ }
  > 5 │ interface A extends Pick<B, 'a'> {}
      │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  ℹ Safe fix: Convert empty interface to type alias.

    3 3 │     b: boolean,
    4 4 │   }
    5   │ - interface·A·extends·Pick<B,·'a'>·{}
      5 │ + type·A·=·Pick<B,·'a'>
jer-sen commented 11 months ago

What's your point?

Type alias is not the same than an interface. Cf https://www.sitepoint.com/typescript-type-vs-interface/

Conaclos commented 11 months ago

We implemented the same behavior as the TypeScript Eslint rule no-empty-interface.

Why a type alias doesn't fit your use-case?

jer-sen commented 11 months ago

We try to always use interfaces cf https://typescript-eslint.io/rules/consistent-type-definitions For consistency, performance and inconsistency detection when declaring interface (instead of getting a never type detected where it is used).

Conaclos commented 11 months ago

For consistency, performance

Performance?

Are you aware that Pick is defined with a type alias? This seems really questionable that an interface extends a type alias...

Moreover, I think that an empty interface that extends a type is non-idiomatic TypeScript code. Even the TypeScript team that advocates the use of interfaces, resort to type aliases for this kind of thing.

jer-sen commented 11 months ago

You can have a look at this: https://github.com/microsoft/TypeScript/wiki/Performance#preferring-interfaces-over-intersections

Conaclos commented 11 months ago

Thanks for sharing this resource!

They recommend using interfaces that extend two or more types instead of using type intersections. Thus, this doesn't apply to empty interfaces that extend a single type. Although not explicitly said, I think the TypeScript team recommend using type aliases instead of interfaces when the interface is empty and extends a single type. This is why the TypeScript ESLint rule no-empty-interface only recommend removing empty interface that extends zero or a single type.

If you are always using interfaces over type aliases, I think you should disable the noEmptyInterface rule?

blutorange commented 11 months ago

Another case where you must use an interface: When you need to augment an interface. With the infamous JQuery as an example:

declare global {
  interface JQuery extends JQueryFunctionExtensionsMyAwesomePlugin {}
}

The linter would rewrite the above as

declare global {
  type JQuery = JQueryFunctionExtensionsMyAwesomePlugin;
}

which does not augment the module, and is also a compile error. (Cannot augment module 'JQuery' with value exports because it resolves to a non-module entity.ts(2649))

Or a self contained example:

interface Foobar {
  foo: () => void;
  bar: () => void;
}
interface Baz {
  baz: () => void;
}
interface Foobar extends Baz {}

Replacing the last last line with type Foobar = Baz changes the semantics and also results in a compile error.

Conaclos commented 11 months ago

@blutorange I recently fixed this for external module (See #959). Unfortunately I didn't think about global declarations :/

I opened #1243 to allow empty interfaces that extend a type in global declarations.

blutorange commented 11 months ago

@Conaclos That sounds like a good idea, and should fix that use case, thanks for the PR : )

Out of curiosity, how do you feel about allowing empty interfaces that extend something in general, not only in module / global declarations? Code such as this is possible, albeit I'd hope less common (personally I don't think I'd need it):

interface Foo {
  x: () => void;
}

interface Bar {
  y: () => void;
}

interface Baz {
  z: () => void;
}

interface Foo extends Bar {}

One could argue that the interface is not empty when properties are pulled in via extends. It might also be a bit more understandable to users, since no error is reported if the number of extended interface is > 1:

// lint error (why do I have to change this to "type Foo = Bar" ?)
interface Foo extends Bar {}

// no lint error (why do I not have to change this to "type Foo = Bar & Baz" ?)
interface Foo extends Bar, Baz {}
Conaclos commented 11 months ago

Yes, I have thought about this change. However, I am a bit hesitant about it because it will be a divergence from TypeScript ESLint. We could wait a bit to get more feedback.

jer-sen commented 11 months ago

I think the (not obvious) choice between type/interface should be handled by @typescript-eslint/consistent-type-definitions . The noEmptyInterface should prevent use of {} when the developer wants an "empty" object.

Some other interesting content:

Conaclos commented 10 months ago

The rule is now ignoring interfaces that extend a type. Thanks for your valuable comments!