proposal: `division_optimization`

srawlins commented 1 year ago

`division_optimization`

Description

This rule replaces the current HintCode named division_optimization. As part of removing the notion of "hints," https://github.com/dart-lang/sdk/issues/50796, we want to move division_optimization to the linter.

Details

The operation 'x \~/ y' is more efficient than '(x / y).toInt()'. Try re-writing the expression to use the '~/' operator.

Kind

Efficiency?

Bad Examples

int divide(num x, num y) => [!(x / y).toInt()!];

Good Examples

int divide(num x, num y) => x ~/ y;

Discussion

Add any other motivation or useful context here.

Discussion checklist

[x] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
[x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
[ ] If there's any prior art (e.g., in other linters), please add references here.
[ ] If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
[ ] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

bwilkerson commented 1 year ago

@rakudrama Is this still valid advice for dart2js? Would we recommend this lint to our users?

rakudrama commented 1 year ago

I would not give this advice on the basis of efficiency.

The performance characteristics of web and native are probably quite different. For dart2js, which one is faster depends on the types and values of the inputs.

An optimizing compiler might recognize the less efficient form and convert it to something more like the more efficient form, and that might depend on what the compiler can tell about the values in play. That said, dart2js does not know about the combination. It knows some tricks for ~/, and when those tricks are applicable, I would expect ~/ to be slightly faster.

I would give the advice to prefer `~/` on the basis of clarity and as a form of education.

In many languages, / is a different operation on integers and doubles, and you choose which operation you want by converting an input. In Dart, / is the same operation regardless of whether the input is int or double (and ~/ is the other operation). This is slightly surprising to developers coming from languages with overloaded or otherwise type-determined operators. It is easy to discover that adding .toInt() gets the result they want.

(x / y).toInt() is worse than x ~/ y mainly because it its more verbose than necessary (exacerbated by the parentheses). Once a developer knows that ~/ exists, they would use it because it is simpler.

I would keep the lint; it would fire only when x or y are int values; and it would say something more along the lines of "Did you know truncating (integer) division is available via the '~/' operator?"

I would change the examples to have more typical of int names and scenarios. double is used much more often than num, and when did you last write a function called divide?

String s = ...
int middle = s.length ~/ 2;

It would be better to fix the front ends

I think the best way to address the education issue is for the front ends (analyzer, CFE) to give a better type error at the point of surprise. The developer reaches for .toInt() because of something like this message:

Error: The argument type 'double' can't be assigned to the parameter type 'int'.
String suffix(String s) => s.substring(s.length / 2);
                                                ^

It would be a better experience to add a suggestion to use ~/ as additional information to the main message.

Then the proposed lint would have less value and could be retired.

srawlins commented 1 year ago

Then the proposed lint would have less value and could be retired.

Just a note: this diagnostic is already implemented, in the analyzer as a Hint (default enabled for everyone). So we may still transfer the diagnostic to this codebase as part of https://github.com/dart-lang/sdk/issues/50796, acknowledging that it has always been the case that we can implement a better diagnostic and retire this division_optimization diagnostic.

bwilkerson commented 1 year ago

It would make more sense to me to not introduce the lint just to remove it later, but I could live with the other.

srawlins commented 1 year ago

Given that dart-lang/sdk#50942 has a lot of complexity, I'm in favor of not blocking the HintCode-removal on it, and instead, migrating division_optimization as it exists today from a Hint to a Lint.

dart-lang / linter