Rule 1121 (assignments in sub-expressions) should exclude "while" conditions

Rules and Languages Report False-positive / False-negative...
,
1

I understand the motivation behind rule 1121, and it has identified a number of places where my code would be clearer by separating a complex line into two simpler ones. The only thing I take exception to is this very common pattern:

let item;
while(item = getNext()) {
  doThingWith(item);
  // ...
}

To comply with this rule, the code needs to be rewritten to

let item = getNext();
while (item) {
  doThingWith(item);
  // ...
  item = getNext();
}

This doesn’t seem so bad, but it gets ugly when your broken-out assignment increases in complexity, like item = parent.child.method("event name", arg1, arg2.sub3). I would argue that reducing duplication by writing the assignment once should take precedence over reducing ambiguity by breaking the assignment out of the conditional clause.

While researching this post, I read the linked reference from the original rule. It has many more examples and helped me understand the rule better. It sounds like while( (item = getNext()) !== undefined) should be compliant, because of this guidance:

As an exception to this guideline, it is permitted to use the assignment operator in conditional expressions when the assignment is not the controlling expression (that is, the assignment is a subexpression)

So, item=getNext() is a subexpression, and (...) !== undefined is the (legal) controlling expression. This is directly counter to the docs for typescript:S1121 which gives a noncompliant example of if ((str = cont.substring(pos1, pos2)) != ''). That’s what the CERT page says you should do!

I’m going to try rewriting my while-loop to match the CERT guidance, and if it still gets flagged I’ll come back here and update my post (and probably file an issue on GH/Jira as well).

2

Update: I changed my code to while ((match = QUOTE_RX.exec(str)) !== null) which is exactly the same construct used in the CERT “compliant solution”, while ((line = reader.readLine()) != null), and the rule still flags it. That means the TS implementation of this rule does not match the official guidance.

Should I open a bug report on GH? Should I edit this post to make it clear that the current behavior is buggy, or maybe make a new post here?

3

It looks like one of the mods removed the Java and CSharp tags – is that because this works as it’s supposed to in those languages?

4

Hello @Thw0rted,

Thank you for your feedback. Indeed, I agree there should be an exception for while conditions on this rule, as described in the reference you linked.

However, I think S1121 is a bit broader than the reference you linked, as it tries to discourage assignments in sub-expressions in general (whereas the reference discourages assignments in conditional expressions specifically). Therefore, I think the recommendation to extract assignments that are part of sub-expressions in a if conditional expression to a dedicated statement is still valid.

I created https://github.com/SonarSource/SonarJS/issues/2169 to track this.

5

I think I understand your clarification. The linked reference gives this example as compliant:

public void f(boolean a, boolean b) {
  if ((a = b) == true) {
    /* ... */
  }
}

and you’re saying that this should still be flagged by the rule. For what it’s worth I agree – unlike a while loop, I can’t think of a case where refactoring to put the assignment on a separate line would make the code more complicated.

6

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.