Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule proposal: only allow function calls starting with underscore on `this` #3435

Closed
janpaul123 opened this Issue Aug 18, 2015 · 32 comments

Comments

Projects
None yet
8 participants
@janpaul123
Copy link

janpaul123 commented Aug 18, 2015

We have the convention to prefix private functions with an underscore. It would be useful to disallow direct calls to private functions unless from the same object, which can be reasonably be approximated by implementing a rule like this:

Good:

  someObject.publicFunction();
  getSomeObject().publicFunction();
  someCallback();
  _someCallbackThatHappensToStartWithAnUnderscore();
  this._privateFunction();

Bad:

  someObject._privateFunction();
  getSomeObject()._privateFunction();

To answer the questions as suggested:

  • The use case for the rule - what is it trying to prevent or flag? Include code examples.

This is to prevent using "private functions" (as commonly prefixed by an underscore) from outside the class that has "access" to them. This makes changing the internals of a class easier, since you have a stronger guarantee that private functions are not called. It thus prevents errors when changing the internals of a class.

  • Whether the rule is trying to prevent an error or is purely stylistic.

It would prevent an error, to some degree. Or to be more precise, it gives stronger guarantees that make errors less likely.

  • Why you believe this rule is generic enough to be included.

Yes, as far as I know prefixing private methods with an underscore is common practice.

  • Whether the rule should be on or off by default.

No, too high chance of false positives.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Aug 18, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Aug 18, 2015

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 18, 2015

Based on our docs, these are the points we touch on when deciding on new rules.
Link: http://eslint.org/docs/developer-guide/contributing#new-rules

Can you please provide more info.
We also provide the ability to create project or company specific rules by using plugin architecture. These come in handy specially when you have a usecase which is not generic enough.
More info: http://eslint.org/docs/developer-guide/working-with-plugins

@janpaul123

This comment has been minimized.

Copy link
Author

janpaul123 commented Aug 25, 2015

@gyandeeps I updated the original post with the points you asked about. I think this could benefit the wider developer community. Please let me know if it makes sense!

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Aug 26, 2015

Sounds good to me.

This seems to be gotten with making the no-underscore-dangle rule ignoring next of this., but the name no-underscore-dangle is not expressing this purpose.

Hmm...

My idea, more generalized:

{
    "no-member-access-violation": [2, {"private": "^_", "protected": "^_"}]
}
  • private is a regular expression for the name of private members.
    This rule warns Identifiers (excludes next of this.) which matches the regular expressions.
  • protected is a regular expression for the name of protected members.
    This rule warns Identifiers (excludes next of this. or super.) which matches the regular expressions.

Note: protected is enabled in ES6 Classes.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 26, 2015

If there is a change to be made, it would have to be for no-underscore-dangle, otherwise we'd have rules with overlapping responsibilities.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Aug 27, 2015

I thought my proposal is not related with underscore directly. But I don't oppose that the general rule is needless.

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Sep 16, 2015

Sent pull-request #3822 to resolve it

@nzakas nzakas added the evaluating label Sep 17, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 17, 2015

I don't think we have identified the best solution yet, so let's keep talking.

Ultimately, I think we want to say "allow dangling underscore if preceded by this". Does everyone agree with that?

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 17, 2015

Agreed

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Sep 18, 2015

@nzakas @mysticatea, so you want to keep this rule without configuration, but add exception regarding this keyword?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 18, 2015

Yes, that's the proposal. @janpaul123 would that work for you?

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Sep 21, 2015

Updated my PR. Now it consists only exception for properties after this

@janpaul123

This comment has been minimized.

Copy link
Author

janpaul123 commented Sep 23, 2015

Thanks all so much, this is a perfect solution! 👍

@nzakas nzakas added accepted and removed evaluating labels Sep 25, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 25, 2015

@just-boris thanks, the PR looks good.

I just want to get a final tally from everyone to make sure this is the best solution. The current solution is to just ignore (automatically, for everyone) properties that are preceded by this, so this._foo() now no longer warns for anyone. Is that the right solution? (Other approach: add an option to turn this behavior on instead of doing it by default.)

@eslint/eslint-team thoughts?

@janpaul123

This comment has been minimized.

Copy link
Author

janpaul123 commented Nov 23, 2015

Ping, it has been a few weeks. Would love to see this get merged! :)

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Nov 23, 2015

@nzakas, I missed this before, but I think that principle of least surprise dictates that this should be an option, not a default.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 23, 2015

I think I would go with an option too.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 23, 2015

@janpaul123 there's no open PR for this.

Okay, let's go for an option. Suggestions for a name?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 23, 2015

excludePrivate?

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Nov 23, 2015

exceptThis?

allowThis?

@janpaul123

This comment has been minimized.

Copy link
Author

janpaul123 commented Nov 23, 2015

@nzakas I was referring to #3822, but an option for this seems reasonable!

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 23, 2015

Well, this is what I did at first, but then @nzakas asked me to rewrite it without option.
😞

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 24, 2015

@janpaul123 yup, that PR is closed.

@just-boris I'm sorry, sometimes that happens when discussing solutions. That's why we recommend people wait until the discussion concludes before starting to implement something. We mark issues with the "accepted" label when they are ready for implementation.

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 24, 2015

Well, now issue has accepted label. And what's the final conclusion about property name?
It matters for me, because I already using my fork and I want to switch back to master as it will be possible

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 24, 2015

We are still trying to determine the option name (see last couple comments).

I think I like allowThis the best. What does everyone else think?

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 24, 2015

As for me, true value of the option should enable dangles after this. So, allowThis is ok.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 24, 2015

allowThis sounds good to me.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 25, 2015

OK, let's do it

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 27, 2015

Just found that you already have configuration option named allow. It is whitelist of identifiers which allowed to be with dangles. allowThis in this context means that it allows this identifier and it is confusing.

So, what about to name new option allowAfterThis? It will resolve the conflict in options meanings

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 27, 2015

I almost finished implementation, but stuck with naming issue

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 27, 2015

Sounds good.

@just-boris

This comment has been minimized.

Copy link
Contributor

just-boris commented Nov 27, 2015

Done. PR #4562

@nzakas nzakas closed this in #4562 Dec 1, 2015

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.