Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: division_optimization #3930

Closed
2 of 5 tasks
srawlins opened this issue Dec 20, 2022 · 6 comments
Closed
2 of 5 tasks

proposal: division_optimization #3930

srawlins opened this issue Dec 20, 2022 · 6 comments
Assignees
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug

Comments

@srawlins
Copy link
Member

srawlins commented Dec 20, 2022

division_optimization

Description

This rule replaces the current HintCode named division_optimization. As part of removing the notion of "hints," dart-lang/sdk#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

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.
  • 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
Copy link
Member

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

@rakudrama
Copy link
Member

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
Copy link
Member Author

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 dart-lang/sdk#50796, acknowledging that it has always been the case that we can implement a better diagnostic and retire this division_optimization diagnostic.

@bwilkerson
Copy link
Member

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
Copy link
Member Author

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.

@pq pq added the P3 A lower priority bug or feature request label May 22, 2023
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
@srawlins srawlins self-assigned this Aug 2, 2024
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Aug 2, 2024
Fixes dart-lang/linter#3930

This allows us to remove (softly) the DIVISION_OPTIMIZATION HintCode,
which was not deemed a good candidate for a Warning.

Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
Change-Id: Iac48c94687d0b6b32c971e9f366ccb96adf34429
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/378543
Reviewed-by: Phil Quitslund <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
mockturtl added a commit to mockturtl/tidy that referenced this issue Aug 9, 2024
@srawlins
Copy link
Member Author

Complete with dart-lang/sdk@b946c47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint-proposal P3 A lower priority bug or feature request status-pending type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants