[@typescript-eslint/naming-convention] Less strict handling of merged declarations

[octogonz]

Repro

TypeScript allows two different declarations to use the same name; they get merged into a single type. Besides a small handful of useful scenarios (e.g. namespace+enum), merged declarations generally arise as hacks in advanced abstractions (e.g. mixins) or retrofitted types for a legacy API. To avoid these complexities, I'll give somewhat contrived examples below.

The problem is that @typescript-eslint/naming-convention seems to check each declaration independently, whereas merged declarations must all have the same name. As a result, ESLint may reject every possible name.

{
  "rules": {
    "@typescript-eslint/naming-convention": ["error"]
  }
}

A couple example inputs:

export class SomeClass {}

// ('typeLike' must have 'PascalCase')
export type Example = SomeClass;

// ERROR: Variable name `Example` must match one of 
// the following formats: camelCase, UPPER_CASE
export const Example = SomeClass;

// ('typeLike' must have 'PascalCase')
export interface Example2 {
  x: boolean;
}

// ERROR: Function name `Example2` must match one of the following formats: camelCase
export function Example2(): Example2 {
  return { x: false };
}

Expected Result

Here's a better way to handle merged declarations: The @typescript-eslint/naming-convention rule should recognize merged declarations, and accept ANY applicable pattern, instead of applying ALL patterns.

For the sample declaration Example2 above, ESLint could accept EITHER 'PascalCase' OR 'camelCase' (whereas currently it requires BOTH).

Versions

package	version
@typescript-eslint/eslint-plugin	3.3.0
@typescript-eslint/parser	3.3.0
TypeScript	3.5.3
ESLint	7.2.0
node	12.17.0
npm	6.14.4

[bradzacher]

I could see some limited value in adding a modifier to help you configure for this case, but definitely shouldn't work silently out of the box.

Making it work silently out of the box would be problematic IMO, as it means that unintentional name clashes would silently be ignored - which may cause inconsistencies in your codebase, and will confuse people:

const example = 1;

type example = string | bool;
// OOPS - didn't upper case the E!
// But there won't be an error due to it automatically being detected as a merged declaration

Part of me thinks that this case might be rare enough that the implementation complexity, user configuration complexity, and potential silent weirdness might not be worth it; instead it might just be better to use a disable comment.

A disable comment has the added bonus of being very clear and explicit and showing exactly why you've broken naming convention in this case.
Though I concede that in a very legacy codebase, it might a huge burden to do this everywhere.

// ('typeLike' must have 'PascalCase')
export interface Example2 {
  x: boolean;
}

// eslint-disable-next-line @typescript-eslint/naming-convention -- intentionally breaking naming convention for declaration merging with the type
export function Example2(): Example2 {
  return { x: false };
}

[octogonz]

If it helps, here is a real world example that encountered this problem:

• function ApiNameMixin(): given a base class, returns a child class that adds the mixin functionality
• interface ApiNameMixin: describes the non-static members of the mixin class
• namespace ApiNameMixin: defines the static members of the mixin class

Sample usage: The ApiPackage class extends from ApiDocumentedItem and then mixes in ApiNameMixin and ApiItemContainerMixin:

export class ApiPackage extends ApiItemContainerMixin(ApiNameMixin(ApiDocumentedItem)) {
  public constructor(options: IApiPackageOptions) {
    super(options);
  }
  . . .
}

All of the mixins in that project rely on merged declarations in this way.

[octogonz]

🤔 Why doesn't @typescript-eslint/naming-convention provide a selector for the namespace declaration? Seems it isn't checked at all.

[bradzacher]

Why doesn't @typescript-eslint/naming-convention provide a selector for the namespace declaration?

The rule was inspired by this tslint rule, which didn't have a selector for it.

And because I rarely ever use namespaces, I may or may not have completely forgotten about them during the initial implementation..... 😅

---

With the concrete examples, I had a play around with it and to clarify "declaration merging" is half right.

You've got both declaration merging and you've also got variable shadowing across scopes (type scope / value scope).

declaration merging what's happening to join the function and the namespace together.
shadowing is what's happening to use the same name in the type and value scopes.

Both cases have this problem, however.

if you merge a namespace into a function, they need the same name.
if you shadow a type into a value, they need the same name.

---

I'm still not sure if this should be built into the rule or not.

The problem is that adding a modifier for the thing doesn't really work, because how do you configure it?

For example, if you want to configure the rule to allow an interface to shadow a variable or a function, but variables require camelCase, and functions require snake_case, then how do you configure that?

Is there one modifier per shadow case - shadow-function, shadow-variable, ...?
Is it just one modifier, and you accept some level of looseness in the naming by configuring to allow both snake_case and camelCase?

There's also the question of how you validate a merged/shadowed name? You can't just ignore it because otherwise you could do weird things like name a variable with snake case and an interface with snake case, but it'd be all okay purely because of the shadowing.

So which format do you use? The format for the one that appears in the source code? Do we add configuration for a "priority" order for picking the format (i.e. variable before interface before namespace...)

The more I think about it, the more I think that a disable comment is the best solution.

[octogonz]

declaration merging what's happening to join the function and the namespace together.
shadowing is what's happening to use the same name in the type and value scopes.

It's true that the type system embodies a runtime "value" and a compile-time "type" that coexist. But I'm not sure there's any meaningful distinction between "merging" value+value (e.g. function+namespace) versus "shadowing" value+type (e.g. function+interface). The reason is that certain constructs (class and enum) contribute both a type and value together. At the end of the day, the important aspect is that a person reading the code sees multiple declarations, but they get rolled into a single compiler ts.Symbol entity, and their names must be the same. That's why I refer to it categorically as declaration merging.

There's also the question of how you validate a merged/shadowed name?

Above, I proposed a simple and well-defined answer: Collect all the declarations that have the same name, test each of their naming-convention selectors, and if any ONE of the selectors succeeds, then accept all declarations.

In other words, we accept the name for a namespace+function+class if it would be allowable for any one of those things, according to the ESLint configuration.

[bradzacher]

There's a slight difference in the way the two work - shadowing can work regardless of the scope or file, but you can only merge things declared in the same scope in the same file.

Eg; you can shadow any imported type with a local variable, or a variable from an upper scope with a type in a lower scope. OTOH, you cannot merge an imported class with a local namespace, nor can you merge a class declared in an upper scope with an interface declared in a lower scope.

This means there is an important difference in the way these two are handled, because it means that each style can be applied to certain constructs in certain ways

• declaration merging requires:
  • class + namespace / interface (can be a 3-way merge)
  • function + namespace
  • enum + namespace
  • we can ignore these cases ofc:
    • enum + enum
    • interface + interface
    • namespace + namespace
• shadowing requires:
  • any value that is not a class or enum + any type

Declaration merging is simple enough for most cases:

• a namespace must always be after the class/enum/function it's merging, so in these cases we could just treat the namespace format matches whatever it's being merged with.
  • that's what I'd expect at least - eg you declare a class and then merge a namespace with the same name as the class.
• merging an interface and a class - IMO this should follow class naming, not interface naming, because it adds properties/methods onto the class, and the interface is essentially "consumed" into the class.

Shadowing is where things get more complex, and I think we should ignore it completely. Declaration merging is an intentional thing IMO, and thus it's easy to detect and deal with
Shadowing is easy to do unintentionally - eg my above example, and if you unintentionally do it, I'd rather you get an error instead of it silently working. I think that shadowing is definitely edge-case enough that it's better to just use a disable comment for it.

---

Note that doing any form of work on based on the scopes will require the scope manager work (#1939), which is still a ways off.
I've got it working, but haven't fixed up the rules to properly handle it yet (#2039)

[octogonz]

There's a slight difference in the way the two work - shadowing can work regardless of the scope or file, but you can only merge things declared in the same scope in the same file.

I never noticed that! Agreed, it is a significant difference.

a namespace must always be after the class/enum/function it's merging, so in these cases we could just treat the namespace format matches whatever it's being merged with.

Hmm... In the example that I gave, the mixin function+interface+namespace really should be capitalized as a class, because semantically it behaves as a base class.

These cases are exotic enough that perhaps the lint rules should simply ignore them: A person who declares a weird merged type is already doing something relatively advanced, so maybe we can trust that they are smart enough to capitalize the name properly.