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

[Linter] Unnecessary while loop #16876

Merged

Conversation

tx-tomcat
Copy link
Contributor

@tx-tomcat tx-tomcat commented Mar 26, 2024

Description

This linter encourages replacing while(true) loops with the more idiomatic loop construct. Here's a breakdown of how it works:
It checks each expression in the AST.
If the expression is a While loop, it examines the condition.
If the condition is always true (using the is_condition_always_true function), it reports a diagnostic suggesting to use loop instead.

The is_condition_always_true function checks if the condition is a boolean literal with the value true.

Test plan

Added more use case including false positive, false negative case

Release notes

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI: Move will now lint against while (true), which should be replaced by loop
  • Rust SDK:

Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
multisig-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
sui-kiosk ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 9:45pm
sui-typescript-docs 🛑 Canceled (Inspect) Aug 14, 2024 9:45pm

Copy link

vercel bot commented May 31, 2024

@tx-tomcat is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

Comment on lines 61 to 69
fn is_condition_always_true(condition: &UnannotatedExp_) -> bool {
if let UnannotatedExp_::Value(val) = condition {
if let Value_::Bool(b) = &val.value {
return *b;
}
}
false
}
fn report_while_true_to_loop(env: &mut CompilationEnv, loc: Loc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking things up into functions like this is typically not preferred when they called exactly once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored.Only is_condition_always_true remain

@@ -0,0 +1,54 @@
module 0x42::loop_test {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this file as previously discussed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored

@tx-tomcat tx-tomcat force-pushed the custom_rules/unnecessary_while_loop branch from 0c69463 to e583142 Compare August 13, 2024 09:43
@tnowacki tnowacki merged commit c4343a2 into MystenLabs:main Aug 14, 2024
41 of 44 checks passed
suiwombat pushed a commit that referenced this pull request Sep 16, 2024
# Description
This linter encourages replacing `while(true)` loops with the more
idiomatic loop construct. Here's a breakdown of how it works:
It checks each expression in the AST.
If the expression is a While loop, it examines the condition.
If the condition is always true (using the `is_condition_always_true`
function), it reports a diagnostic suggesting to use loop instead.

The `is_condition_always_true` function checks if the condition is a
boolean literal with the value true.

## Test plan
Added more use case including false positive, false negative case

## Release notes

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [X] CLI: Move will now lint against `while (true)`, which should be
replaced by `loop`
- [ ] Rust SDK:

---------

Co-authored-by: jamedzung <[email protected]>
Co-authored-by: Todd Nowacki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants