Introduce a no-java-constructor rule. #8212

bolinfest · 2017-03-08T00:15:14Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[X] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Please describe what the rule should do:

This catches an error where the developer tries to declare an ES6 constructor by creating a method with the name of the class instead of the constructor keyword.

What category of rule is this? (place an "X" next to just one item)

[ ] Enforces code style
[X] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

This introduces a rule to catch the case where the developer writes this:

class Point {
  Point(x, y) {
    this._x = x;
    this._y = y;
  }
}

but meant to write this:

class Point {
  constructor(x, y) {
    this._x = x;
    this._y = y;
  }
}

Why should this rule be included in ESLint (instead of a plugin)?

I believe this satisfies the criteria enumerated at http://eslint.org/docs/developer-guide/contributing/new-rules. It addresses a generic, non-library specific issue and provides a convenient autofix. If you are new to JavaScript and make this mistake, it can be very frustrating to debug.

What changes did you make? (Give an overview)

Introduced a new rule with documentation.

Is there anything you'd like reviewers to focus on?

Assuming the core team is open to this rule, I could use some help to ensure I have specified the correct data for the "meta" section.

Also, I am making use of eslint-disable-next-line eslint-plugin/report-message-format, which I am not sure is kosher.

mention-bot · 2017-03-08T00:15:15Z

@bolinfest, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @platinumazure and @kaicataldo to be potential reviewers.

jsf-clabot · 2017-03-08T00:15:15Z

All committers have signed the CLA.

eslintbot · 2017-03-08T00:15:16Z

Thanks for the pull request, @bolinfest! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

This introduces a rule to catch the case where the developer writes this: ``` class Point { Point(x, y) { this._x = x; this._y = y; } } ``` but meant to write this: ``` class Point { constructor(x, y) { this._x = x; this._y = y; } } ```

eslintbot · 2017-03-08T00:28:58Z

LGTM

not-an-aardvark

Thanks for the pull request.

Personally, I'm not sure this rule is a good fit for adding to core. It seems like the problem that it's trying to prevent is Java-specific, and is also probably quite uncommon (the syntax for JS classes is very different from the syntax for Java classes anyway).

That said, let's wait and see what the rest of the team thinks.

I left a few comments on the implementation. Feel free to address them now if you'd like. Alternatively you might want to wait and see if the team accepts this proposal, so that you don't end up wasting your time if the team decides to reject the proposal. (Of course, if that happens you can always distribute your rule as a plugin and/or use it for your own projects.)

not-an-aardvark · 2017-03-08T00:20:51Z

conf/eslint-recommended.js

@@ -127,6 +127,7 @@ module.exports = {
        "no-invalid-this": "off",
        "no-irregular-whitespace": "error",
        "no-iterator": "off",
+        "no-java-constructor": "error",


We can't enable rules in eslint:recommended until the next major release, so please set this to "off" for now.

not-an-aardvark · 2017-03-08T00:21:23Z

lib/rules/no-java-constructor.js

+        docs: {
+            description: "disallow method names that match the class name",
+            category: "Possible Errors",
+            recommended: true


We can't enable rules in eslint:recommended until the next major release, so please set this to false for now.

not-an-aardvark · 2017-03-08T00:21:58Z

lib/rules/no-java-constructor.js

+        },
+
+        // TODO(mbolin): Find out what is supposed to go here.
+        schema: {


schema is used to configure options that a rule can accept.

Since this rule doesn't accept any options, you would probably want to set schema to an empty array.

Ah, thanks, done.

not-an-aardvark · 2017-03-08T00:23:46Z

lib/rules/no-java-constructor.js

+        return {
+            MethodDefinition(node) {
+                const methodName = node.key.name;
+                const className = node.parent.parent.id.name;


This will throw an error for class expressions that don't have names:

(class { foo() {} })

Good catch! I fixed the code and added a test case.

not-an-aardvark · 2017-03-08T00:25:24Z

lib/rules/no-java-constructor.js

+                        // eslint-disable-next-line eslint-plugin/report-message-format
+                        message: "Method name should not match class name. Did you mean to use `constructor`?",
+                        fix(fixer) {
+                            return fixer.replaceText(node.key, "constructor");


I don't think this rule should be autofixable. Autofixers should generally avoid changing the runtime behavior of code (e.g. causing a working program to break). If the user actually intended to use a method name which is the same as a class name, this would cause their code to break.

Would it be possible/acceptable to have an option to offer an autofix that defaults to off?

No, we generally don't have specific options related to autofixing. If a rule isn't 100% sure of the right thing to do for an autofix, it shouldn't attempt it.

For more discussion on this, see #7873 and #8018.

OK, I removed the code for the autofix and updated the test and docs.

eslintbot · 2017-03-08T00:50:57Z

LGTM

eslintbot · 2017-03-08T01:08:22Z

LGTM

bolinfest · 2017-03-08T01:13:57Z

Personally, I'm not sure this rule is a good fit for adding to core. It seems like the problem that it's trying to prevent is Java-specific, and is also probably quite uncommon (the syntax for JS classes is very different from the syntax for Java classes anyway).

I've done this twice. Both times, it was maddening to debug. As I put in the documentation, I urge you to do an informal survey with some friends and put the following code in front of them and quickly ask them what it does:

class Point {
  Point(x, y) {
    this.x = x;
    this.y = y;
  }
}

const p = new Point(3, 4);
console.log(`(${p.x}, ${p.y})`);

I suspect they don't see it, which is why I think this makes a good candidate for a core rule.

platinumazure · 2017-03-08T01:22:14Z

Maybe the issue is just with the name. For example, maybe no-class-method-same-name (ugh, that needs improvement) or use-correct-constructor might fit better?

kaicataldo · 2017-03-08T01:25:55Z

Thanks for contributing to ESLint!

Though I understand the motivation for the rule, I'm in agreement with @not-an-aardvark on this one. This seems like a better fit for a plugin rather than in core.

bolinfest · 2017-03-08T01:26:09Z

@platinumazure That seems sensible. I'm completely flexible on the name, so if someone has a strong opinion, I'm happy to go through to change it throughout.

mysticatea · 2017-03-08T02:03:10Z

Thank you for contributing.

Sounds reasonable to me. Constructors which are the same name as the class name is a general practice (C++, Java, C#, ...). I'm sure this is "good to have", but I'm not sure if this is enough important to core.

My 2 cents, I can accept this rule since the maintenance cost is mostly zero and implementation is here.

platinumazure · 2017-03-08T03:07:32Z

Alright, I'm going to champion this. So many other languages name a constructor with the class name, and on top of that, constructor isn't even a keyword but rather a context-sensitive identifier. And as noted by @bolinfest, it must be a pain in the ass to debug. So I'm in favor of adding this to core.

bolinfest · 2017-03-08T04:01:05Z

@platinumazure Awesome: thanks so much! As you point out, this pattern is not limited to Java, but also C# and C++, so giving it a more Java-agnostic name as you suggested seems apt. So far, you've thrown out:

no-class-method-same-name
use-correct-constructor

Some other ideas:

no-constructor-impostor
no-constructor-look-alike

I think that no-class-method-same-name is apt, though I think it is good to have constructor in the name. From that perspective, I think no-constructor-look-alike is the least judgmental ("correct" and "impostor" seem like slightly more loaded terms).

platinumazure · 2017-03-08T04:04:26Z

Awesome, thanks for throwing out suggestions. That said, I don't think ESLint is supposed to care about users' feelings 😀

bolinfest · 2017-03-08T04:13:12Z

@platinumazure Do you have a preference on the name? I'd just like to decide on something and turn this PR around. I guess we can wait until midday tomorrow to see if anyone else wants to chime in to bikeshed the name.

Also, do you prefer to see the commits for a PR squashed, or do you like merging in entire stacks? From http://eslint.org/docs/developer-guide/contributing/pull-requests, it seems like squashing is preferred, which is totally fine with me. It doesn't always seem to play well with GitHub's code review flow, but whatever.

platinumazure · 2017-03-08T04:35:28Z

@bolinfest I apologize, I was remiss in failing to explain our intake process. We require both a champion and team consensus on any new rule or rule change proposal. I've volunteered to champion the rule and that means I will be the team member driving this forward; however, we also need at least three other team members to support this rule (indicated via thumbs-up on the original post) and no one on the team to vote against including the rule (indicated by thumbs-down on the original post). See here for more information.

So my next step is to try to persuade @not-an-aardvark and @kaicataldo that this is at least a good enough idea that they don't need to thumbs-down. Thus, I apologize but I think this will take a while to turn around (and if we can't reach consensus before too long, we might have to close the issue). ESLint is already a massive project with a mind-boggling number of core rules, so we've had to tighten our process on including new rules. That said, I'm on your side here and I will do what I can to persuade the team that this is worth including.

Given all of the above, I'm not in a huge rush to get a name picked. I will do my best to ensure that the rule concept gets a fair evaluation, without getting bogged down in a debate on the name.

bolinfest · 2017-03-08T04:36:31Z

@platinumazure All that sounds totally fair: thanks for the detailed explanation!

platinumazure · 2017-03-08T04:38:19Z

@bolinfest Sorry, forgot to answer your other question. We prefer to merge contributions in squashed form to keep the changelog simple. However, we started using Github's "Squash and Merge" function, so we are happy to review PRs with multiple commits (and that works really well with the PR Review feature as well). So please don't feel the need to squash commits unless you feel it just makes the history more readable for later reviews. Hope this makes sense 😄

platinumazure · 2017-03-13T14:14:14Z

@not-an-aardvark @kaicataldo @hzoo If we can agree on a different name, is this use case something you could find palatable for core rule inclusion?

As I noted here, there are legitimate use cases for this for people coming in from different languages, and the OP has noted that it is very hard to debug this case. I don't think there would be much maintainability cost to this rule and it would be a shoe-in for eslint:recommended (IMO), so a lot of people could be helped by this. And it helps that we already have a PR.

gyandeeps · 2017-04-12T14:52:35Z

Thanks for contributing. My vote is also 👎 for this as a core rule. But feel free to create a plugin for this rule.
Also we need to make a decision on where we are heading with this rule. Based on votes above most of the people agree that this is not worthy of a core rule.

mysticatea · 2017-04-13T11:20:29Z

I think that the word Java in the title of this issue has given people bad first impression. However, this will solve more general problem. The use of the same name as their class for constructors is the common practice on many languages. When developers which use also other language wrote the habit, this rule will help them, because the habit never generates any syntax errors. The eslint:recommended is the best place of this rule in my opinion.

not-an-aardvark · 2017-06-03T07:00:32Z

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get enough support from the team and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after a long time tend to never do it, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.

New: Introduce a no-java-constructor rule.

8884bd8

This introduces a rule to catch the case where the developer writes this: ``` class Point { Point(x, y) { this._x = x; this._y = y; } } ``` but meant to write this: ``` class Point { constructor(x, y) { this._x = x; this._y = y; } } ```

bolinfest force-pushed the no-java-constructor branch from bbf6180 to 8884bd8 Compare March 8, 2017 00:28

not-an-aardvark suggested changes Mar 8, 2017

View reviewed changes

not-an-aardvark added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules labels Mar 8, 2017

Address initial feedback from @not-an-aardvark.

376c519

Remove autofix logic as requested by @not-an-aardvark.

62431b7

platinumazure self-assigned this Mar 8, 2017

not-an-aardvark closed this Jun 3, 2017

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Introduce a no-java-constructor rule. #8212

Introduce a no-java-constructor rule. #8212

Conversation

bolinfest commented Mar 8, 2017

mention-bot commented Mar 8, 2017

jsf-clabot commented Mar 8, 2017 • edited Loading

eslintbot commented Mar 8, 2017

eslintbot commented Mar 8, 2017

not-an-aardvark left a comment

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

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

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

Choose a reason for hiding this comment

eslintbot commented Mar 8, 2017

eslintbot commented Mar 8, 2017

bolinfest commented Mar 8, 2017

platinumazure commented Mar 8, 2017

kaicataldo commented Mar 8, 2017

bolinfest commented Mar 8, 2017

mysticatea commented Mar 8, 2017

platinumazure commented Mar 8, 2017

bolinfest commented Mar 8, 2017

platinumazure commented Mar 8, 2017

bolinfest commented Mar 8, 2017

platinumazure commented Mar 8, 2017

bolinfest commented Mar 8, 2017

platinumazure commented Mar 8, 2017

platinumazure commented Mar 13, 2017

gyandeeps commented Apr 12, 2017

mysticatea commented Apr 13, 2017 • edited Loading

not-an-aardvark commented Jun 3, 2017

jsf-clabot commented Mar 8, 2017 •

edited

Loading

mysticatea commented Apr 13, 2017 •

edited

Loading