unnecessary_statements shouldn't trigger when the operator is overloaded

[gaetschwartz]

Say you some code that overloads an operator:

class Logger {
	void operator<<(String message) {/*...*/}
}

void main() {
	final l = Logger();

	l << "Hello world (づ｡◕‿‿◕｡)づ"; 
	// Unnecessary statement.
	// Try completing the statement or breaking it up. (unnecessary_statements)
}

The lint trigger when it shouldn't as the statement has a clear effect here.

[bwilkerson]

It's a difficult balance in this case between false positives (reporting code that is actually necessary) and false negatives (not reporting code that isn't necessary).

The reason the code above is a false positive is because there is a (presumed) side-effect that makes the removal of the code impact the behavior of the code. The fact that the operator was overridden doesn't mean that it has a side-effect, and the only way the lint could know for sure whether there is a side-effect would be to walk through the implementation of the overridden operator, and everything invoked by it, to find some code with a side-effect (such as writing to the file system). Doing that would be too expensive.

So we're left with the need to make some assumptions and base the analysis off those assumptions. In this case, the assumption is that most of the time operators don't have side-effects, so we should assume that none of them do. That assumption might be wrong, but one counterexample doesn't prove that it is.

There is an alternative. We could consider adding an annotation to allow users to tell us when our assumption has been violated (maybe something like @hasSideEffect). Then, rather than needing to look at the full call graph for an operator, we could just look for the use of that annotation. The question is whether that annotation would be useful enough for the community as a whole to make it worth adding.

[gaetschwartz]

To me, the lint should be as permissive as possible, I would rather have it not report unnecessary code, than having false positives. My idea was to ignore all binary expressions for which the operator is overloaded. I implemented what I think is a decent compromise in my PR dart-archive/linter#4469

[lrhn]

ignore all binary expressions for which the operator is overloaded

I'm not entirely sure what "overloaded" means here (Dart doesn't have overloading, so maybe "overridden").

I'm also fairly certain, no matter what it means, it's not a good signal for whether an operator has side effects or not.

The only operators which we know, for absolutely certain, has no side effects are the operators of num, int, double, bool or String.
Possibly ones called directly on an object creation expression where we have analyzed the implementation to have no side effects.

Every other operator is on a non-final/sealed class which means there might be an implementation somewhere which has a side-effecting implementation of the operator, and the first operand may be an instance of that class, which isn't visible from its static type.

Basically, you'd need to know the concrete implementation of every operator (or function) that this lint warns about, and know for certain (through built-in knowledge or member body analysis) that it doesn't do any side effects, in order to know whether it has side effects. That's the only way to get absolutely no false positives.

In practice it's much more useful to assume that operators do not have side effects, and warn about operators used in positions where the value isn't used. That will have false positives, and annotating the operator with some marker annotation saying that it does do stuff seems like a better trade-off than just not warning at all.
(Even if it requires the author of a side-effecting operator to do extra work, that they might not know about. As soon as someone tells them, they can add the annotation and release a new version of the package, and all is good from then on.)

[gaetschwartz]

Yes, by overloading I mean overriding (overloading is the word generally used to designate such operation afaik ?)

In practice it's much more useful to assume that operators do not have side effects, and warn about operators used in positions where the value isn't used.

Is it though ? I don't necessarily agree here, I think it's more useful to assume overridden operators do have a side-effect (which is often the case).

From a correctness standpoint I would rather have false negatives (linter not complaining when using an operator that is overridden by the lhs or an extension that's potentially pure) than false positives (linter complaining when using an operator that is overridden by the lhs or an extension that has side effects).

Here is what I would prefer the behavior to be like.

class Logger {
	void operator<<(String message) {/*...*/} // Could be pure or not
}

void main() {
	final l = Logger();

	l << "Hello world";	// Shouldn't complain
	1 + 2;				// Should complain
}

[lrhn]

The general use of "operator overloading" really just means "operators that work on multiple types", so you don't need different syntax for "plus of integers" and "plus of doubles". Dart allows by making them instance methods.

Overloading in an OO language like Dart usually means an object having two methods with the same name.
Dart does not support that kind of overloading.

Overriding means a subclass adding an implementation which overrides the implementation it inherited from a superclass.

What you suggest, I think, is the lint not triggering on any operator at all, except a few statically recognized ones from the SDK (which are no more or less overloading than any other operators, they're just easier to statically know.)
The list of operators it would trigger on is then very short, to the point where the lint isn't as useful any more.

[gaetschwartz]

That's not what I am suggesting. I'm suggesting that the lint triggers on all operators, except the ones for which the lhs, is a user-defined class and has overridden the operator (or an extension has). The rule wouldn't check the "nature" of the operator or have a hardcoded list of operators to check, it just checks if the operator that's in use is a "core one" (1+1) or a "user-defined one" (logger << "Message").

Edit
I think I actually mean overloaded and not overridden here, as the logger class here doesn't have a << operator at all, so I think overloading is the right word here, correct me if I'm wrong ?

[lrhn]

The question is which classes you would not consider user-defined, and which do declare operators?

If we limit ourselves to the platform libraries as non-user-defined, there really aren't that many.
When I say "a few statically recognized ones from the SDK", I refer the operators of the num subclasses, String, bool, BigInt and a few typed-data types like Float32x4. These are all sealed/final types, so we know that there are no user-written subclasses which override the implementation.
We can even allow the == operator of a few classes which are now declared final, but most classes are not final, so we can't know that a subclass doesn't override with a side-effecting implementation. If that can happen, we cannot know from the static type of the first operand which member is invoked.

I believe that to be all the non-[], []= and ==operators currently defined in the platform SDK libraries, and I'd count it as "a few", relatively.

There is nothing special about the SDK defined operators, other than them being defined in platform libraries, so recognizing everything else does mean checking the "nature" (origin) of the declaration, and/or having a list of all those platform types and their allowed operators.

Everything else would be user-defined.

The word I'd use here is "implemented". If a user-defined class or extension has an implementation of an operator, then that's a user-defined operator. It doesn't override or overload anything.

My worry is that if we restrict the lint to only those operators, then it doesn't do enough, and I'd want another, more heuristic, lint which considers non-side-effecting any operator, getter, literal, constructor invocation, method on any num, String or bool, method on Iterable other than forEach, any method which returns an Iterable or Stream, and probably a bunch more.

[srawlins]

I agree that an annotation should be used rather than a blanket exception for non-SDK operators. As most user-implemented operator + implementations probably have no side effects (and are not meant to be used as statements), it would be a huge loss of functionality to the rule to except all of them.

A better solution to the << operator I think would be an annotation like @hasSideEffects.

[lrhn]

The problem with an @hasSideEffects annotation is that it requires the operator author to add it, even if they don't care, or know, about the lint it affects. So you can't assume anyone to add it, which makes enabling the lint problematic.

It'll be annoying if you have to write // ignore: unnecessary_statement repeatedly in a method that uses a fancy

extension <T> on StreamController<T> { 
  void operator <<(T value) { this.add(value); }
}

and ignoring it for the file is error-prone. (Do we have ignore_for_method?).

No easy solution that doesn't require somehow telling the analyzer/linter that this particular operator, that I didn't write, does have side-effects.

[srawlins]

Yeah I think most linters are configurable, with a config file, to allow for exceptions like this. You can build up a list of methods, functions, etc. which are known to be pure, and a list of getters which are known to have side effects.