proposal: unnecessary_pattern_assignment

[srawlins]

unnecessary_pattern_assignment

Description

I found code like the following and could not figure out how it was legal syntax:

prefix.C() = preifx.C()..a = 5;

How could a constructor call be the target of an assignment? prefix.C() = ? Ah, it turns out this is a pattern assignment, and the pattern, prefix.C(), is an object pattern. I think this should be reported.

Details

Patterns are not my strong suit. But I think what we have is an object pattern in an assignment context, but the object pattern has no field subpatterns. I think this is precisely the conditions that we want to report on.

Kind

Unintentional code.

Bad Examples

C() = C()..a = 1;
var C() = C()..a = 1;

The second example here certainly seems less unintentional, with the var sitting there. But it's still bizarre unnecessary code.

Good Examples

C()..a = 1;
var x = 1;
C(a: x) = C(a: x)..a = 1;

Discussion

The same copy-paste error could result in an object pattern with subpattern fields, as in C(a: x) = C(a: x)..a = 1;, but in such a case, the code would have meaning. In that example, x would be assigned as part of the object-pattern-in-assignment-context. So I don't think it would be prudent to report such cases.

There may be a question of how common it would be to typo the constructor declaration with zero args. This came up in code that uses a builder pattern, such that the constructor takes no parameters.

[srawlins]

CC @munificent for comments. Patterns are not my strong suit.

[munificent]

I found code like the following and could not figure out how it was legal syntax:

prefix.C() = prefix.C()..a = 5;

Wow, that is bananas. I wonder what the author was thinking. Even the cascade is pointless.

Also, I just realized that this is valid syntax too:

() = ();

What have I done to the language?! 😱

Yes, I think it makes a lot of sense to have a lint that fires if you have a pattern assignment that doesn't assign to any variables. It's basically dead code (unless the RHS has type dynamic, in which case there is a cast from dynamic that could fail but... don't do that either).

[srawlins]

Wow, that is bananas. I wonder what the author was thinking. Even the cascade is pointless.

I really assume it is copy-paste or just missing that you typed it extra, and then no compiler complained, haha. There actually were multiple cascades; I just simplified it to the point of raising another question 🤣

[pq]

Oh wow. Crazy example!

It's basically dead code

If this is the case, can we flag it as dead code using a variation of the shared analyzer DEAD_CODE diagnostic and not have a new (opt-in) lint?

[srawlins]

I don't think so. The existing DEAD_CODE diagnostic is about unreachable code.

[pq]

Good point. That said, can we still make this an analyzer diagnostic? Is there any reason to make folks opt-in?

[bwilkerson]

I think we typically use the prefix unnecessary_ for code that is reachable but doesn't change the semantics. So there's at least a precedent for making this kind of thing a warning.

[srawlins]

I think that'd be great. There are some other rules that guard against "unintentional" code, like no_self_assignments, recursive_getters, and valid_regexps, that could be converted to Warnings.