Relax cascade_invocations lint for assignments?

[jamesderlin]

I had some code:

_imageStream?.removeListener(_onImageUpdated);
_imageStream = newImageStream;
_imageStream.addListener(_onImageUpdated);

I have the cascade_invocations lint enabled because using the cascade operator still doesn't quite come naturally to me. The lint suggested changing it to:

_imageStream?.removeListener(_onImageUpdated);
_imageStream = newImageStream..addListener(_onImageUpdated);

However, using the cascade operator here changes the order of operations, and it led to a bug where the callback fired synchronously and failed assertions because _imageStream had not yet been set to newImageStream.

Perhaps the cascade_invocations lint should be relaxed if the expression is being used as the RHS of an assignment? (Or possibly if the cascaded expression would be the RHS of an assignment and if we can somehow determine that the expression is doing something non-trivial that could depend on the LHS of the assignment.)

(I suppose something else to consider is whether foo = bar..baz() could be treated as (foo = bar)..baz() instead of as foo = (bar..baz), but I admit that that probably would be unexpected, particularly if the cascaded expression all fits on one line.)

[pq]

Good catch. Thanks @jamesderlin!

Perhaps the cascade_invocations lint should be relaxed if the expression is being used as the RHS of an assignment?

This sounds reasonable to me.

@bwilkerson: what do you think?

[bwilkerson]

I think this is a hard question. :-)

The problem is that we can't, in general, tell whether the variable being assigned can be referenced from within the code being invoked after the assignment (except in the case where it's being assigned to a local variable). If we could, I'd recommend that we fix the lint to do the work so that the user doesn't need to.

But given that we can't, the question becomes, will enough users run into the kind of issue that @jamesderlin did that we should limit the lint, or is it a small enough percentage that we should recommend adding an //ignore comment when it does arise?

Limiting the lint would cause it to not be able to catch cases like

_list = _getDefaultList();
_list.add(a);
_list.add(b);

which seems like one of the cases the lint was designed to catch.

[srawlins]

Duplicate of #57631