-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: Support linting complex udim2 constructors that can be simplified #601
Open
Quenty
wants to merge
6
commits into
Kampfkarren:main
Choose a base branch
from
Quenty:users/quenty/roblox_complex_udim2_new
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
58ec788
feat: Support linting complex udim2 constructors that can be simplified
Quenty 8e9d516
fix: Address PR comments and refactor code to support UDim2.new(a, 0,…
Quenty c7d286d
Merge branch 'main' into users/quenty/roblox_complex_udim2_new
Quenty 1ba6d91
refactor: Rename to ManualFromScaleOrFromOffsetLint
Quenty a154640
Merge remote-tracking branch 'kampfkarren/main' into users/quenty/rob…
Quenty a87e682
docs: Update changelog.md
Quenty File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# roblox_manual_fromscale_or_fromoffset | ||
## What it does | ||
Checks for uses of `UDim2.new` where the arguments could be simplified to `UDim2.fromScale` or `UDim2.fromOffset`. | ||
|
||
## Why this is bad | ||
This reduces readability of `UDim2.new()` construction. | ||
|
||
## Example | ||
```lua | ||
UDim2.new(1, 0, 1, 0) | ||
``` | ||
|
||
## Remarks | ||
This lint is only active if you are using the Roblox standard library. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
156 changes: 156 additions & 0 deletions
156
selene-lib/src/lints/roblox_manual_fromscale_or_fromoffset.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,156 @@ | ||
use super::*; | ||
use crate::ast_util::range; | ||
use std::convert::Infallible; | ||
|
||
use full_moon::{ | ||
ast::{self, Ast}, | ||
visitors::Visitor, | ||
}; | ||
|
||
pub struct ManualFromScaleOrFromOffsetLint; | ||
|
||
fn create_diagnostic(args: &UDim2ComplexArgs) -> Diagnostic { | ||
let code = "roblox_manual_fromscale_or_fromoffset"; | ||
let primary_label = Label::new(args.call_range); | ||
|
||
match args.complexity_type { | ||
UDim2ConstructorType::OffsetOnly => Diagnostic::new_complete( | ||
code, | ||
"this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset" | ||
.to_owned(), | ||
primary_label, | ||
vec![format!( | ||
"try: UDim2.fromOffset({}, {})", | ||
args.arg_0, args.arg_1 | ||
)], | ||
Vec::new(), | ||
), | ||
UDim2ConstructorType::ScaleOnly => Diagnostic::new_complete( | ||
code, | ||
"this UDim2.new call only sets scale, and can be simplified using UDim2.fromScale" | ||
.to_owned(), | ||
primary_label, | ||
vec![format!( | ||
"try: UDim2.fromScale({}, {})", | ||
args.arg_0, args.arg_1 | ||
)], | ||
Vec::new(), | ||
), | ||
} | ||
} | ||
|
||
impl Lint for ManualFromScaleOrFromOffsetLint { | ||
type Config = (); | ||
type Error = Infallible; | ||
|
||
const SEVERITY: Severity = Severity::Warning; | ||
const LINT_TYPE: LintType = LintType::Style; | ||
|
||
fn new(_: Self::Config) -> Result<Self, Self::Error> { | ||
Ok(ManualFromScaleOrFromOffsetLint) | ||
} | ||
|
||
fn pass(&self, ast: &Ast, context: &Context, _: &AstContext) -> Vec<Diagnostic> { | ||
if !context.is_roblox() { | ||
return Vec::new(); | ||
} | ||
|
||
let mut visitor = UDim2NewVisitor::default(); | ||
|
||
visitor.visit_ast(ast); | ||
|
||
visitor.args.iter().map(create_diagnostic).collect() | ||
} | ||
} | ||
|
||
#[derive(Default)] | ||
struct UDim2NewVisitor { | ||
args: Vec<UDim2ComplexArgs>, | ||
} | ||
|
||
#[derive(PartialEq)] | ||
enum UDim2ConstructorType { | ||
ScaleOnly, | ||
OffsetOnly, | ||
} | ||
|
||
struct UDim2ComplexArgs { | ||
complexity_type: UDim2ConstructorType, | ||
call_range: (usize, usize), | ||
arg_0: String, | ||
arg_1: String, | ||
} | ||
|
||
impl Visitor for UDim2NewVisitor { | ||
fn visit_function_call(&mut self, call: &ast::FunctionCall) { | ||
if_chain::if_chain! { | ||
if let ast::Prefix::Name(token) = call.prefix(); | ||
if token.token().to_string() == "UDim2"; | ||
let mut suffixes = call.suffixes().collect::<Vec<_>>(); | ||
|
||
if suffixes.len() == 2; // .new and () | ||
let call_suffix = suffixes.pop().unwrap(); | ||
let index_suffix = suffixes.pop().unwrap(); | ||
|
||
if let ast::Suffix::Index(ast::Index::Dot { name, .. }) = index_suffix; | ||
if name.token().to_string() == "new"; | ||
|
||
if let ast::Suffix::Call(ast::Call::AnonymousCall( | ||
ast::FunctionArgs::Parentheses { arguments, .. } | ||
)) = call_suffix; | ||
|
||
then { | ||
if arguments.len() != 4 { | ||
return; | ||
} | ||
|
||
let mut iter = arguments.iter(); | ||
let x_scale = iter.next().unwrap().to_string(); | ||
let x_offset = iter.next().unwrap().to_string(); | ||
let y_scale = iter.next().unwrap().to_string(); | ||
let y_offset = iter.next().unwrap().to_string(); | ||
|
||
let only_offset = x_scale.parse::<f32>() == Ok(0.0) && y_scale.parse::<f32>() == Ok(0.0); | ||
let only_scale = x_offset.parse::<f32>() == Ok(0.0) && y_offset.parse::<f32>() == Ok(0.0); | ||
|
||
if only_offset && only_scale | ||
{ | ||
// Skip linting if all are zero | ||
return; | ||
} | ||
else if only_offset | ||
{ | ||
self.args.push(UDim2ComplexArgs { | ||
call_range: range(call), | ||
arg_0: x_offset, | ||
arg_1: y_offset, | ||
complexity_type: UDim2ConstructorType::OffsetOnly, | ||
}); | ||
} | ||
else if only_scale | ||
{ | ||
self.args.push(UDim2ComplexArgs { | ||
call_range: range(call), | ||
arg_0: x_scale, | ||
arg_1: y_scale, | ||
complexity_type: UDim2ConstructorType::ScaleOnly, | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::{super::test_util::test_lint, *}; | ||
|
||
#[test] | ||
fn test_manual_fromscale_or_fromoffset() { | ||
test_lint( | ||
ManualFromScaleOrFromOffsetLint::new(()).unwrap(), | ||
"roblox_manual_fromscale_or_fromoffset", | ||
"roblox_manual_fromscale_or_fromoffset", | ||
); | ||
} | ||
} |
27 changes: 27 additions & 0 deletions
27
...sts/lints/roblox_manual_fromscale_or_fromoffset/roblox_manual_fromscale_or_fromoffset.lua
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
UDim2.new(0) | ||
UDim2.new(0.5) | ||
UDim2.new(1) | ||
UDim2.new(2) | ||
UDim2.new(a) | ||
UDim2.new(1, 1) | ||
UDim2.new(1, 2) | ||
UDim2.new(a, b) | ||
UDim2.new(1, a) | ||
UDim2.new(1, 1, 1) | ||
UDim2.new(a, b, c) | ||
UDim2.new(1, 1, 1, 1) | ||
UDim2.new(a, b, c, d) | ||
UDim2.fromOffset(1, 1) | ||
UDim2.fromScale(1, 1) | ||
UDim2.new() | ||
UDim2.new(1, 0, 1, 0) | ||
UDim2.new(0, 0, 1, 0) | ||
UDim2.new(0, 5, 1, 5) | ||
UDim2.new(0, 5, 0, 1) | ||
UDim2.new(0, 0, 0, 1) | ||
UDim2.new(0, 1, 0, 1) | ||
UDim2.new(0, a, 0, 1) | ||
UDim2.new(0, a, 0, b) | ||
UDim2.new(1, a, 0, b) | ||
UDim2.new(a, 0, 0, 0) | ||
UDim2.new(a, 0.0, 0, 0.0) |
2 changes: 2 additions & 0 deletions
2
...ints/roblox_manual_fromscale_or_fromoffset/roblox_manual_fromscale_or_fromoffset.std.toml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[selene] | ||
name = "roblox" |
72 changes: 72 additions & 0 deletions
72
.../lints/roblox_manual_fromscale_or_fromoffset/roblox_manual_fromscale_or_fromoffset.stderr
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets scale, and can be simplified using UDim2.fromScale | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:17:1 | ||
│ | ||
17 │ UDim2.new(1, 0, 1, 0) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromScale(1, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets scale, and can be simplified using UDim2.fromScale | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:18:1 | ||
│ | ||
18 │ UDim2.new(0, 0, 1, 0) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromScale(0, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:20:1 | ||
│ | ||
20 │ UDim2.new(0, 5, 0, 1) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromOffset(5, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:21:1 | ||
│ | ||
21 │ UDim2.new(0, 0, 0, 1) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromOffset(0, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:22:1 | ||
│ | ||
22 │ UDim2.new(0, 1, 0, 1) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromOffset(1, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:23:1 | ||
│ | ||
23 │ UDim2.new(0, a, 0, 1) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromOffset(a, 1) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets offset, and can be simplified using UDim2.fromOffset | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:24:1 | ||
│ | ||
24 │ UDim2.new(0, a, 0, b) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromOffset(a, b) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets scale, and can be simplified using UDim2.fromScale | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:26:1 | ||
│ | ||
26 │ UDim2.new(a, 0, 0, 0) | ||
│ ^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromScale(a, 0) | ||
|
||
error[roblox_manual_fromscale_or_fromoffset]: this UDim2.new call only sets scale, and can be simplified using UDim2.fromScale | ||
┌─ roblox_manual_fromscale_or_fromoffset.lua:27:1 | ||
│ | ||
27 │ UDim2.new(a, 0.0, 0, 0.0) | ||
│ ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
│ | ||
= try: UDim2.fromScale(a, 0) | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be alphabetical order. Same with
lints.rs
.