[member-ordering] support abstract methods

[mgol]

Repro

{
  "rules": {
    "@typescript-eslint/member-ordering": [
      "error",
      {
        "default": [
          "public-instance-method",
          "private-instance-method"
        ]
      }
    ],
  }
}

abstract class TestClass {
  abstract bar(): void;
  foo(): void {}
}

Expected Result

No error

Actual Result

  3:3  error  Member foo should be declared before all private instance method definitions  @typescript-eslint/member-ordering

Additional Info

Abstract methods are not necessarily private methods. There are two issues here:

1. There is no private method in this class so the error message doesn't make sense.
2. Nowhere in the settings is the order between abstract & regular methods defined so it shouldn't be enforced.

This came up when I tried to migrate my TSLint config containing:

    "member-ordering": [
      true,
      "public-before-private",
      "static-before-instance",
      "variables-before-functions"
    ],

and I failed due to errors around abstract methods handling.

Versions

package	version
@typescript-eslint/eslint-plugin	1.5.0
@typescript-eslint/parser	1.5.0
TypeScript	3.3.4000
ESLint	5.15.3
node	v8.15.1
npm	N/A
yarn	1.15.2

[bradzacher]

Abstract methods/props were never implemented in this rule.

typescript-eslint/packages/eslint-plugin/src/rules/member-ordering.ts

Lines 184 to 187 in 3b28cac

	// TODO: add missing TSCallSignatureDeclaration
	// TODO: add missing TSIndexSignature
	// TODO: add missing TSAbstractClassProperty
	// TODO: add missing TSAbstractMethodDefinition

[mgol]

@bradzacher

Abstract methods/props were never implemented in this rule.

I agree adding support for abstract methods/props would be a new feature but I think treating them as private ones is a bug in the plugin logic. It should ignore method/prop types it doesn't recognize and not misreport them as private. What do you think?

[bradzacher]

Yeah, it's a bug with the error message.
Though the bug is inherently because it doesn't know specifically that the abstract method is abstract, and instead defaulting to the lowest one in the list it can.

[David-Else]

Hi, I am having problems with my abstract classes and weird error messages, is there a workaround to stop the erroneous error messages? For example:

21 abstract class GameObject {
22  abstract position: Vector2 = [0, 0];
23  public rotation = 0;
24  public scale: Vector2 = [0, 0];
25  public get x(): number {
26    return this.position[0];
27  }
28  public get y(): number {
29    return this.position[1];
30  }
31 }

 23:3 warning  Member rotation should be declared before all method definitions  @typescript-eslint/member-ordering
 24:3 warning  Member scale should be declared before all method definitions     @typescript-eslint/member-ordering

The only method definitions are the getters, and they are after rotation and scale, so I am not sure what is happening? My rules are default:

   "@typescript-eslint/member-ordering": "warn",

If I change the order to:

  public rotation = 0;
  public scale: Vector2 = [0, 0];
  abstract position: Vector2 = [0, 0];

the error goes away, but abstract position: Vector2 = [0, 0]; is not a method, could the error message be incorrect?

[bradzacher]

as mentioned - the error message is incorrect. There is no handling for abstract members. This causes the error message to be wrong.

[David-Else]

This is the first major problem I encountered, hope it can be fixed soon :)

Sorry I don't have enough skill to help yet, thanks to you and all the community for making typescript-eslint awesome.

[glen-84]

Shouldn't this be labelled as a bug and not an enhancement?

[bradzacher]

There is a bug with the error message, sure, but this issue is asking for (intentionally) unimplemented logic to be added (as per #395 (comment)), hence enhancement.

[glen-84]

But there shouldn't be any error messages just because there happen to be abstract properties or methods in the class.

As @mgol mentioned, if you're not ready to add support for abstract members, then this rule should simply ignore them for now.

Setting that aside, couldn't TSAbstractClassProperty behave the same as ClassProperty, and TSAbstractMethodDefinition the same as MethodDefinition? That method is only trying to determine method vs constructor vs field, right?

[bradzacher]

I mean, it's really just bikeshedding - it doesn't matter if it's classed as a bug or a feature - doesn't change the fact that abstract things were not implemented.

---

That's a question for people that actually use the rule. I don't personally use the rule, so I don't have an opinion about how it should sort things.

There are two choices - either abstract should be an entirely new set of configs ('public-abstract-method' | 'private-abstract-method' | ...etc), or abstract should just be handled as part of the existing configs.

If pushed, I guess I'd lean toward the former, because I assume that the original author's intention was the former (hence they opted to skip implementation in their initial implementation).

Happy to accept a PR with either.

[glen-84]

There are two choices ...

IMO, the first option is the feature request, and the 2nd option is the bug fix.

I agree that option 1 is the better option, as it would allow developers to position abstract members differently to non-abstract members, which could result in more consistent ordering.

Looking at the code, could abstract be considered another scope, or would the naming need to be changed?

[bradzacher]

I believe it could be treated as a new scope, i.e same as instance/static.

[hsmitty93]

I created a PR to add support of abstract members to the member-ordering rule, would love a review please. #1004

[carcade]

abstract class Foo {
    public abstract name: string;

    constructor() {}

    public bar(): void {};
}

This code with default member-ordering rule config ("@typescript-eslint/member-ordering": ["error"]) gives me the following error:

ESLint: Member constructor should be declared before all method definitions. (@typescript-eslint/member-ordering)

But if I remove abstract property name, there is no such error.

Is it a bug?