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: `no-unmodified-loop-condition` #4523

Closed
mysticatea opened this Issue Nov 23, 2015 · 17 comments

Comments

Projects
None yet
6 participants
@mysticatea
Copy link
Member

commented Nov 23, 2015

In a loop condition, if there is a variable which is not modified in the loop body, probably it's mistake.
So this rule reports unmodified variables in a loop condition.

The following patterns are considered problems:

let node = ...;
while (node) {
    doSomething(node);
}

for (let j = 0; j < xs.length; ++i) {
}

The following patterns are considered not problems:

let node = ...;
while (node) {
    doSomething(node);

    node = node.parent;
}

for (let j = 0; j < xs.length; ++j) {
}
@eslintbot

This comment has been minimized.

Copy link

commented Nov 23, 2015

@mysticatea Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 23, 2015

@mysticatea mysticatea added rule evaluating and removed triage labels Nov 23, 2015

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

What about if you're checking an object property and passing the object to
a function that could modify it? The modification of that property would
not be detectable via static analysis, yet it may well be modified by this
other function. (Note that I'm talking specifically about property changes-
I'm well aware that reassignments to a parameter don't have any side effect
in the calling function.)
On Nov 22, 2015 10:15 PM, "Toru Nagashima" notifications@github.com wrote:

In a loop condition, if there is a variable which is not modified in the
loop body, probably it's mistake.
So this rule reports unmodified variables in a loop condition.

The following patterns are considered problems:

let node = ...;while (node) {
doSomething(node);
}
for (let j = 0; j < xs.length; ++i) {
}

The following patterns are considered not problems:

let node = ...;while (node) {
doSomething(node);

node = node.parent;

}
for (let j = 0; j < xs.length; ++j) {
}


Reply to this email directly or view it on GitHub
#4523.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

Agree with @platinumazure Your first example in the invalid section has a good chance of modifying node inside doSomething function (if node is a complex type)

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

Maybe it could work if the loop condition checks an identifier only?
On Nov 22, 2015 10:38 PM, "Ilya Volodin" notifications@github.com wrote:

Agree with @platinumazure https://github.com/platinumazure Your first
example in the invalid section has a good chance of modifying node inside
doSomething function (if node is a complex type)


Reply to this email directly or view it on GitHub
#4523 (comment).

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

I think it can be done they way @mysticatea proposes with an additional option to clear any condition variable that is passed as a parameter of a function call inside the body of the loop.

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2015

I'm thinking this rule does not support object properties in this time.
It just checks escope.Reference and there are write references in the loop.

And the example is not doSomething(&node) of C :)
doSomething() cannot modify the node variable.

@Slayer95

This comment has been minimized.

Copy link

commented Nov 23, 2015

doSomething() cannot modify the node variable.

It can:

let node = ...;
while (node) {
    doSomething(node);
}

function doSomething (n) {
    node = false;
}
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Nov 23, 2015

@Slayer95 We wouldn't be able to catch that though static analysis anyways. The example I had in mind was more like:

let node = {a:10};
while (node) {
  doSomething(node);
}
function doSomething(node) {
  node = null;
}
@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2015

@Slayer95 Ah, I see. That pattern is analyzed correctly by escope, so we can catch it with escope.Reference simply.

@ilyavolodin Parameters in JS is not pointers of C :)

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 24, 2015

Can you tell if the variable is modified inside the loop vs. Outside the loop?

let node = ...;
while (node) {
    doSomething(node);
}

node = node.parent;
@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2015

@nzakas There are not write references in the while-loop, so the node identifier at line 2 is warned. It's a cause of an infinity loop.


If a write reference exist in a local function, I will check whether or not the local function is called in the loop. I can track that by escope.Reference, I guess.

let node = ...;
while (node) { // 4. This `node` identifier is not warned.
    doSomething(node); // 3. Called.
}

node = node.parent;

function doSomething(n) { // 2. Checks this function is called in a loop?
    node = node.parent; // 1. A write reference of node.
}
@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 27, 2015

Ah, I see. Won't this overlap with no-constant-condition?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 27, 2015

@nzakas I wouldn't think so. no-constant-condition implies a condition like true, using a constant, but this rule would use a condition like node (as in, variable node is not falsy). The expression node is not a constant expression, but this rule would try to catch no modifications. Likewise, constant conditions would imply no loop variable at all, no? The only potential overlap I could see is const variables, since those could be adjudged "constant" for no-constant-condition and also non-modified variables for this proposed rule. I imagine it would be fairly easy to make a decision of which rule should cover that case.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 27, 2015

@platinumazure ah, good call

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2015

@platinumazure Thank you!

no-constant-condition is covering constant values in IfStatements, ConditionalExpressions, and loop conditions.
This proposal is covering variables in loop conditions.
As @platinumazure mentioned, only const variables are overlapped.

I'm OK that this is an option of no-constant-condition.
But I think new rule is better.

@nzakas nzakas added accepted feature and removed evaluating labels Nov 30, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 30, 2015

Yup, sounds good!

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2015

Thank you!
I'm working on this.

@mysticatea mysticatea self-assigned this Dec 4, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 15, 2015

@nzakas nzakas closed this in #4706 Dec 15, 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.