New: `symbol-description` rule (fixes #6778) #6825

jrencz · 2016-08-02T08:28:12Z

What issue does this pull request address?
#6778

What changes did you make? (Give an overview)
I implemented symbol-description rule

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

whether rule is supposed to be recommended or not
in which group (Best practices or ES6) should this rule be placed in

TODO:

rule should allow string as a description
rule should allow variable as a description
if Symbol is redefined Symbol() should be valid
if Symbol is shadowed Symbol() should be valid

mention-bot · 2016-08-02T08:28:13Z

@jrencz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @alberto and @kaicataldo to be potential reviewers

eslintbot · 2016-08-02T08:28:14Z

LGTM

eslintbot · 2016-08-02T08:34:59Z

LGTM

mysticatea · 2016-08-02T10:44:14Z

Thank you for this PR!

I added do not merge label for now since #6778 has not been accepted yet.
I predict the issue to be accepted soon.

ferrante · 2016-08-02T15:07:13Z

+1

nzakas · 2016-08-02T17:31:11Z

Issue has been accepted.

nzakas · 2016-08-02T17:31:54Z

docs/rules/symbol-description.md

@@ -0,0 +1,94 @@
+# Require/Disallow Symbol Description (symbol-description)
+
+The `Symbol` constructor may have optional description:


Nitpick: it's not a constructor, just a function.

Good point. Then no-new-symbol should be fixed as well - I based this one on that.

The Symbol constructor is not intended to be used with the new operator

I'll fix it as well.

@jrencz I would recommend fixing those docs in a separate issue/PR (if you weren't already thinking of doing that)

nzakas · 2016-08-02T17:36:08Z

This is a really great start @jrencz. My comments are mostly just fine-tuning what you already have here.

platinumazure · 2016-08-02T17:38:09Z

docs/rules/symbol-description.md

+
+## When Not To Use It
+
+This rule should not be used in ES3/5 environments and in case you don't want to enforce either presence or omission of `description`.


I think this might be a little awkward as one sentence, and would do better broken up into two sentences.

For example:
This rule should not be used in ES3/5 environments. In addition, this rule can be safely turned off if you don't want to enforce either the presence or the omission of description when creating Symbols.

Not a huge deal-- your call on whether you want to change or not.

platinumazure · 2016-08-02T17:42:17Z

This is looking really good. Just two things from me:

I left an inline comment about a config section with a long and potentially awkward sentence.
I would love to see some tests around redefining or shadowing the Symbol variable:
1. Redefine: Symbol = foo; Symbol(); should be valid even if rule is set to always
2. Redefine: Symbol = foo; Symbol("stuff"); should be valid even if rule is set to never (may not apply if "never" option removed)
3. Redefine (advanced): Symbol(); Symbol = foo; should be invalid if rule is set to always
4. Shadow: function bar() { var Symbol = foo; Symbol(); } should be valid even if rule is set to always
5. Shadow: function bar() { var Symbol = foo; Symbol("stuff"); } should be valid even if rule is set to never (may not apply if "never" option removed)

eslintbot · 2016-08-02T18:28:20Z

LGTM

jrencz · 2016-08-02T18:30:16Z

@platinumazure @nzakas than you for the review.

I applied all of @nzakas comments (I have option removal commit ready yet not pushed).

I'll add tests @platinumazure asked for (but ii and v since never is removed).

platinumazure · 2016-08-02T18:34:22Z

@jrencz Thanks! I'm wondering, though, maybe we should just remove the "always" option altogether and have schema: []? Seems it would simplify some things.

eslintbot · 2016-08-02T18:45:31Z

LGTM

eslintbot · 2016-08-02T18:46:58Z

LGTM

jrencz · 2016-08-02T19:06:51Z

State of the PR: I need to add rules suggested by @platinumazure. All other comments were applied.

nzakas · 2016-08-03T16:33:26Z

docs/rules/symbol-description.md

+> console.log(foo);
+// Symbol(foo)
+
+var bar = foo + '';


This actually throws an error. Symbols cannot be coerced.

eslintbot · 2016-08-03T18:18:22Z

LGTM

eslintbot · 2016-08-03T18:21:15Z

LGTM

eslintbot · 2016-08-03T18:22:39Z

LGTM

ilyavolodin · 2016-08-09T00:14:40Z

docs/rules/symbol-description.md

+
+## Rule Details
+
+This rule allows enforcing or disallowing symbol descriptions.


I'm not sure I like "allows" followed by "disallowing", seems a bit confusing. I think it's better to follow the language we used in other rules like arrow-body-style, func-name, etc. "This rule can enforce or disallow the use of braces around arrow function body."

Now it's more like, "This rules requires a description when creating symbols."

ilyavolodin · 2016-08-09T00:18:59Z

Just a few minor comments about documentation, otherwise LGTM.

nzakas · 2016-08-10T16:14:05Z

docs/rules/symbol-description.md

@@ -0,0 +1,61 @@
+# Require Symbol Description (symbol-description)


These words should be all lowercase

platinumazure · 2016-08-18T14:48:57Z

Friendly ping @jrencz, need any help?

jrencz · 2016-08-19T09:32:18Z

@platinumazure I had no time to start. I'll try today

eslintbot · 2016-08-19T09:53:45Z

LGTM

eslintbot · 2016-08-19T10:35:46Z

LGTM

jrencz · 2016-08-19T10:35:49Z

@platinumazure I added the tests you suggested. It turns out my implementation (which is combined from no-new-symbol and radix) did covers all cases. Please review now.

There's one case which you didn't mention and i'm not sure if it can be covered:

Symbol = function () {};
Symbol();

If symbol is reassigned in the same scope as it's called but before it's called (you'r point 3 was about redefining after it's called). Symbol call in this example is a call of noop function assigned to Symbol identifier. It raises a warning although it shouldn't but I'n not sure if it's possible to detect if Symbol was reassigned. I think it may be checked in the same function scope but definitely not in general (in global scope).

platinumazure · 2016-08-19T15:07:14Z

@mysticatea Is the issue which @jrencz raised here possible to catch via escope?

mysticatea · 2016-08-19T15:15:38Z

I think the current implementation is good.
Several rules are using the same way (variable && variable.defs.length === 0) to detect redeclarations, but we have received no complaint about it.

We need dynamic analysis to detect completely the timing of the overwrites, but ESLint is a static analyzer 😄 .

mysticatea · 2016-08-19T15:17:10Z

lib/rules/symbol-description.js

+        }
+
+        return {
+            "Program:exit": () => {


I feel this : and => are extra.

"Program:exit"() {

(I'm sorry, very nit-picking.)

Should object-shorthand (which we have on in the codebase I think?) catch this? I guess it might not due to this working differently...

It depends on whether you treat it as a method that should use the object's this or a regular function that may only react to the this from the outer environment in the place of its definition.

Both could be used in the same project based on various circumstances.

platinumazure · 2016-08-19T15:21:20Z

@mysticatea I'll defer to your opinion. If you think the current implementation is good and that we probably can't do better, feel free to merge (if enough people have looked at it, in your view, and enough time has passed). Thanks!

mysticatea · 2016-08-19T15:32:40Z

Oops, sorry, I had been asked about re-assignments, not redeclarations. Hmm, interesting.

variable && variable.references.some(r => r.isWrite())

This is a check of re-assignment (though it doesn't detect timing...). I think fine that we add the check. But if we don't add the check, it will be warned by no-undef or no-global-assign, so it's not bad. I don't have strong opinions here.

jrencz · 2016-08-19T16:05:18Z

But if we don't add the check, it will be warned by no-undef or no-global-assign, so it's not bad.

I think it sticks to the policy that each rule should overlap with others as little as possible. Let's stick to what we have now then.

I'm going to squash it now and It looks like it's ready to be merged (at least from my point of view)

eslintbot · 2016-08-19T16:11:05Z

LGTM

nzakas · 2016-08-19T18:20:07Z

We don't need to check for reassignment, only that Symbol is a global reference. If people do weird things to that reference, it's on them. :)

nzakas · 2016-08-19T18:21:57Z

Lgtm, just waiting another day for others to review.

platinumazure · 2016-08-22T21:39:00Z

LGTM.

vitorbal · 2016-08-23T08:41:19Z

LGTM

ilyavolodin · 2016-08-23T12:38:46Z

LGTM. Thanks @jrencz for working on this!

jquerybot added the CLA: Valid label Aug 2, 2016

jrencz force-pushed the symbol-description branch from b53b42b to 57f7be5 Compare August 2, 2016 08:34

mysticatea added the do not merge This pull request should not be merged yet label Aug 2, 2016

nzakas removed the do not merge This pull request should not be merged yet label Aug 2, 2016

nzakas reviewed Aug 2, 2016
View reviewed changes

platinumazure reviewed Aug 2, 2016
View reviewed changes

jrencz mentioned this pull request Aug 2, 2016

Docs: Remove incorrect "constructor" statemant from no-new-symbol docs #6830

Merged

nzakas reviewed Aug 3, 2016
View reviewed changes

ilyavolodin reviewed Aug 9, 2016
View reviewed changes

nzakas reviewed Aug 10, 2016
View reviewed changes

jrencz force-pushed the symbol-description branch from d9ef644 to e0f7a71 Compare August 19, 2016 09:53

mysticatea reviewed Aug 19, 2016
View reviewed changes

New: symbol-description rule (fixes eslint#6778)

d0f0496

jrencz force-pushed the symbol-description branch from eb52c28 to d0f0496 Compare August 19, 2016 16:11

ilyavolodin merged commit 0d268f1 into eslint:master Aug 23, 2016

jrencz deleted the symbol-description branch August 23, 2016 15:40

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

		@@ -0,0 +1,94 @@
		# Require/Disallow Symbol Description (symbol-description)

		The `Symbol` constructor may have optional description:


		## When Not To Use It

		This rule should not be used in ES3/5 environments and in case you don't want to enforce either presence or omission of `description`.


		## Rule Details

		This rule allows enforcing or disallowing symbol descriptions.

		@@ -0,0 +1,61 @@
		# Require Symbol Description (symbol-description)

Uh oh!

New: symbol-description rule (fixes #6778) #6825

New: symbol-description rule (fixes #6778) #6825

Uh oh!

Conversation

jrencz commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Aug 2, 2016

Uh oh!

eslintbot commented Aug 2, 2016

Uh oh!

eslintbot commented Aug 2, 2016

Uh oh!

mysticatea commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferrante commented Aug 2, 2016

Uh oh!

nzakas commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nzakas commented Aug 2, 2016

Uh oh!

platinumazure Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinumazure commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eslintbot commented Aug 2, 2016

Uh oh!

jrencz commented Aug 2, 2016

Uh oh!

platinumazure commented Aug 2, 2016

Uh oh!

eslintbot commented Aug 2, 2016

Uh oh!

eslintbot commented Aug 2, 2016

Uh oh!

jrencz commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eslintbot commented Aug 3, 2016

Uh oh!

eslintbot commented Aug 3, 2016

Uh oh!

eslintbot commented Aug 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilyavolodin commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinumazure commented Aug 18, 2016

Uh oh!

jrencz commented Aug 19, 2016

Uh oh!

eslintbot commented Aug 19, 2016

Uh oh!

eslintbot commented Aug 19, 2016

Uh oh!

jrencz commented Aug 19, 2016

Uh oh!

New: `symbol-description` rule (fixes #6778) #6825

New: `symbol-description` rule (fixes #6778) #6825

jrencz commented Aug 2, 2016 •

edited

Loading

mysticatea commented Aug 2, 2016 •

edited

Loading

platinumazure Aug 2, 2016 •

edited

Loading

platinumazure commented Aug 2, 2016 •

edited

Loading

mysticatea Aug 19, 2016 •

edited

Loading

mysticatea commented Aug 19, 2016 •

edited

Loading