From da0febe69971d76852be25593516c0abf0a5d9bd Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Fri, 13 Dec 2024 17:05:24 +0000 Subject: [PATCH] Add rule `magic-number-in-array-size` --- docs/rules.md | 6 + docs/rules/magic-number-in-array-size.md | 26 +++ .../test/fixtures/readability/R001.f90 | 16 ++ fortitude/src/registry.rs | 3 + fortitude/src/rules/mod.rs | 3 + .../src/rules/readability/magic_numbers.rs | 96 +++++++++++ fortitude/src/rules/readability/mod.rs | 27 +++ ...__magic-number-in-array-size_R001.f90.snap | 154 ++++++++++++++++++ 8 files changed, 331 insertions(+) create mode 100644 docs/rules/magic-number-in-array-size.md create mode 100644 fortitude/resources/test/fixtures/readability/R001.f90 create mode 100644 fortitude/src/rules/readability/magic_numbers.rs create mode 100644 fortitude/src/rules/readability/mod.rs create mode 100644 fortitude/src/rules/readability/snapshots/fortitude__rules__readability__tests__magic-number-in-array-size_R001.f90.snap diff --git a/docs/rules.md b/docs/rules.md index e2a416d..23cf6a9 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -77,3 +77,9 @@ | OB011 | [common-block](rules/common-block.md) | common blocks are obsolescent, prefer modules or derived types | โœ”๏ธ | | OB021 | [entry-statement](rules/entry-statement.md) | entry statements are obsolescent, use module procedures with generic interface | โœ”๏ธ | +### Readability (R) + +| Code | Name | Message | | +| ---- | ---- | ------- | ------: | +| R001 | [magic-number-in-array-size](rules/magic-number-in-array-size.md) | Magic number in array size, consider replacing {value} with named `parameter` | ๐Ÿงช | + diff --git a/docs/rules/magic-number-in-array-size.md b/docs/rules/magic-number-in-array-size.md new file mode 100644 index 0000000..ae5f958 --- /dev/null +++ b/docs/rules/magic-number-in-array-size.md @@ -0,0 +1,26 @@ +# magic-number-in-array-size (R001) +This rule is unstable and in [preview](../preview.md). The `--preview` flag is required for use. + +## What it does +Checks for use of literals when specifying array sizes + +## Why is this bad? +Prefer named constants to literal integers when declaring arrays. This makes +it easier to find similarly sized arrays in the codebase, as well as ensuring +they are consistently sized when specified in different places. Named +parameters also make it easier for readers to understand your code. + +The values `0, 1, 2, 3, 4` are ignored by default. + +TODO: Add user settings + +## Examples +Instead of: +```f90 +integer, dimension(10) :: x, y +``` +prefer: +```f90 +integer, parameter :: NUM_SPLINE_POINTS = 10 +integer, dimension(NUM_SPLINE_POINTS) :: x, y +``` \ No newline at end of file diff --git a/fortitude/resources/test/fixtures/readability/R001.f90 b/fortitude/resources/test/fixtures/readability/R001.f90 new file mode 100644 index 0000000..9fa8dff --- /dev/null +++ b/fortitude/resources/test/fixtures/readability/R001.f90 @@ -0,0 +1,16 @@ +module test + implicit none + integer, parameter :: NUM_POINTS = 54 + integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + integer, dimension(57) :: E + integer, dimension(57, 64) :: F + integer, dimension(NUM_POINTS) :: G + integer :: H(NUM_POINTS) + integer, dimension(-NUM_POINTS:NUM_POINTS) :: I + integer, dimension(0:NUM_POINTS) :: J +contains + subroutine foo(L, M) + integer, dimension(8:9, 10, 11:12), intent(in) :: L + integer, intent(out) :: M(57) + end subroutine foo +end module test diff --git a/fortitude/src/registry.rs b/fortitude/src/registry.rs index 687da1a..2ba3fcd 100644 --- a/fortitude/src/registry.rs +++ b/fortitude/src/registry.rs @@ -71,6 +71,9 @@ pub enum Category { /// Obsolescent features #[prefix = "OB"] Obsolescent, + /// Readability. + #[prefix = "R"] + Readability, } pub trait RuleNamespace: Sized { diff --git a/fortitude/src/rules/mod.rs b/fortitude/src/rules/mod.rs index ffa283b..872410d 100644 --- a/fortitude/src/rules/mod.rs +++ b/fortitude/src/rules/mod.rs @@ -8,6 +8,7 @@ pub(crate) mod io; pub(crate) mod modules; pub(crate) mod obsolescent; pub(crate) mod precision; +pub(crate) mod readability; pub(crate) mod style; pub(crate) mod testing; pub(crate) mod typing; @@ -114,6 +115,8 @@ pub fn code_to_rule(category: Category, code: &str) -> Option<(RuleGroup, Rule)> (Io, "001") => (RuleGroup::Preview, Ast, io::missing_specifier::MissingActionSpecifier), + (Readability, "001") => (RuleGroup::Preview, Ast, readability::magic_numbers::MagicNumberInArraySize), + // Rules for testing fortitude // Couldn't get a separate `Testing` category working for some reason #[cfg(any(feature = "test-rules", test))] diff --git a/fortitude/src/rules/readability/magic_numbers.rs b/fortitude/src/rules/readability/magic_numbers.rs new file mode 100644 index 0000000..8c088ea --- /dev/null +++ b/fortitude/src/rules/readability/magic_numbers.rs @@ -0,0 +1,96 @@ +use crate::ast::FortitudeNode; +use crate::settings::Settings; +use crate::{AstRule, FromAstNode}; +use itertools::Itertools; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_source_file::SourceFile; +use tree_sitter::Node; + +/// ## What it does +/// Checks for use of literals when specifying array sizes +/// +/// ## Why is this bad? +/// Prefer named constants to literal integers when declaring arrays. This makes +/// it easier to find similarly sized arrays in the codebase, as well as ensuring +/// they are consistently sized when specified in different places. Named +/// parameters also make it easier for readers to understand your code. +/// +/// The values `0, 1, 2, 3, 4` are ignored by default. +/// +/// TODO: Add user settings +/// +/// ## Examples +/// Instead of: +/// ```f90 +/// integer, dimension(10) :: x, y +/// ``` +/// prefer: +/// ```f90 +/// integer, parameter :: NUM_SPLINE_POINTS = 10 +/// integer, dimension(NUM_SPLINE_POINTS) :: x, y +/// ``` +#[violation] +pub struct MagicNumberInArraySize { + value: i32, +} + +impl Violation for MagicNumberInArraySize { + #[derive_message_formats] + fn message(&self) -> String { + let Self { value } = self; + format!("Magic number in array size, consider replacing {value} with named `parameter`") + } +} + +const DEFAULT_ALLOWED_LITERALS: &[i32] = &[0, 1, 2, 3, 4]; + +impl AstRule for MagicNumberInArraySize { + fn check(_settings: &Settings, node: &Node, source: &SourceFile) -> Option> { + // We're either looking for `type, dimension(X) :: variable` or `type :: variable(X)` + let size = if node.kind() == "type_qualifier" { + if node.child(0)?.to_text(source.source_text())?.to_lowercase() != "dimension" { + return None; + } + node.child_with_name("argument_list")? + } else { + // sized_declarator + node.child_with_name("size")? + }; + + let violations: Vec<_> = size + .named_children(&mut size.walk()) + .filter_map(|child| match child.kind() { + // Need to return a vec here to match next arm + "number_literal" => Some(vec![child]), + // This is `X:Y`, pull out the lower and upper bound separately + "extent_specifier" => Some( + child + .named_children(&mut child.walk()) + .filter(|extent| extent.kind() == "number_literal") + .collect_vec(), + ), + _ => None, + }) + .flatten() + .filter_map(|literal| { + let value = literal + .to_text(source.source_text())? + .parse::() + .unwrap(); + if DEFAULT_ALLOWED_LITERALS.contains(&value) { + None + } else { + Some((literal, value)) + } + }) + .map(|(child, value)| Diagnostic::from_node(Self { value }, &child)) + .collect(); + + Some(violations) + } + + fn entrypoints() -> Vec<&'static str> { + vec!["sized_declarator", "type_qualifier"] + } +} diff --git a/fortitude/src/rules/readability/mod.rs b/fortitude/src/rules/readability/mod.rs new file mode 100644 index 0000000..d468fd7 --- /dev/null +++ b/fortitude/src/rules/readability/mod.rs @@ -0,0 +1,27 @@ +pub mod magic_numbers; + +#[cfg(test)] +mod tests { + use std::convert::AsRef; + use std::path::Path; + + use anyhow::Result; + use insta::assert_snapshot; + use test_case::test_case; + + use crate::registry::Rule; + use crate::settings::Settings; + use crate::test::test_path; + + #[test_case(Rule::MagicNumberInArraySize, Path::new("R001.f90"))] + fn rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("readability").join(path).as_path(), + &[rule_code], + &Settings::default(), + )?; + assert_snapshot!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/fortitude/src/rules/readability/snapshots/fortitude__rules__readability__tests__magic-number-in-array-size_R001.f90.snap b/fortitude/src/rules/readability/snapshots/fortitude__rules__readability__tests__magic-number-in-array-size_R001.f90.snap new file mode 100644 index 0000000..70a4517 --- /dev/null +++ b/fortitude/src/rules/readability/snapshots/fortitude__rules__readability__tests__magic-number-in-array-size_R001.f90.snap @@ -0,0 +1,154 @@ +--- +source: fortitude/src/rules/readability/mod.rs +expression: diagnostics +snapshot_kind: text +--- +./resources/test/fixtures/readability/R001.f90:4:16: R001 Magic number in array size, consider replacing 221 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^^^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:4:27: R001 Magic number in array size, consider replacing 221 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^^^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:4:37: R001 Magic number in array size, consider replacing 100 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^^^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:4:53: R001 Magic number in array size, consider replacing 33 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:4:56: R001 Magic number in array size, consider replacing 44 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:4:60: R001 Magic number in array size, consider replacing 5 with named `parameter` + | +2 | implicit none +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) + | ^ R001 +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | + +./resources/test/fixtures/readability/R001.f90:5:22: R001 Magic number in array size, consider replacing 57 with named `parameter` + | +3 | integer, parameter :: NUM_POINTS = 54 +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) +5 | integer, dimension(57) :: E + | ^^ R001 +6 | integer, dimension(57, 64) :: F +7 | integer, dimension(NUM_POINTS) :: G + | + +./resources/test/fixtures/readability/R001.f90:6:22: R001 Magic number in array size, consider replacing 57 with named `parameter` + | +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | ^^ R001 +7 | integer, dimension(NUM_POINTS) :: G +8 | integer :: H(NUM_POINTS) + | + +./resources/test/fixtures/readability/R001.f90:6:26: R001 Magic number in array size, consider replacing 64 with named `parameter` + | +4 | integer :: A(221), B(4, 221), C(1:100), D(1, 2:3, 33:44, 5, NUM_POINTS) +5 | integer, dimension(57) :: E +6 | integer, dimension(57, 64) :: F + | ^^ R001 +7 | integer, dimension(NUM_POINTS) :: G +8 | integer :: H(NUM_POINTS) + | + +./resources/test/fixtures/readability/R001.f90:13:24: R001 Magic number in array size, consider replacing 8 with named `parameter` + | +11 | contains +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L + | ^ R001 +14 | integer, intent(out) :: M(57) +15 | end subroutine foo + | + +./resources/test/fixtures/readability/R001.f90:13:26: R001 Magic number in array size, consider replacing 9 with named `parameter` + | +11 | contains +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L + | ^ R001 +14 | integer, intent(out) :: M(57) +15 | end subroutine foo + | + +./resources/test/fixtures/readability/R001.f90:13:29: R001 Magic number in array size, consider replacing 10 with named `parameter` + | +11 | contains +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L + | ^^ R001 +14 | integer, intent(out) :: M(57) +15 | end subroutine foo + | + +./resources/test/fixtures/readability/R001.f90:13:33: R001 Magic number in array size, consider replacing 11 with named `parameter` + | +11 | contains +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L + | ^^ R001 +14 | integer, intent(out) :: M(57) +15 | end subroutine foo + | + +./resources/test/fixtures/readability/R001.f90:13:36: R001 Magic number in array size, consider replacing 12 with named `parameter` + | +11 | contains +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L + | ^^ R001 +14 | integer, intent(out) :: M(57) +15 | end subroutine foo + | + +./resources/test/fixtures/readability/R001.f90:14:31: R001 Magic number in array size, consider replacing 57 with named `parameter` + | +12 | subroutine foo(L, M) +13 | integer, dimension(8:9, 10, 11:12), intent(in) :: L +14 | integer, intent(out) :: M(57) + | ^^ R001 +15 | end subroutine foo +16 | end module test + |