Merge pull request #236 from PlasmaFAIR/magic-number-in-array-size

Add rule `magic-number-in-array-size`

docs/rules.md

@@ -77,3 +77,9 @@
 | OB011 | [common-block](rules/common-block.md) | common blocks are obsolescent, prefer modules or derived types | <span title='Rule is stable' style='opacity: 0.6'>✔️</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
 | OB021 | [entry-statement](rules/entry-statement.md) | entry statements are obsolescent, use module procedures with generic interface | <span title='Rule is stable' style='opacity: 0.6'>✔️</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
 
+### 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` | <span title='Rule is in preview'>🧪</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
+

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
+```

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

fortitude/src/registry.rs

@@ -71,6 +71,9 @@ pub enum Category {
     /// Obsolescent features
     #[prefix = "OB"]
     Obsolescent,
+    /// Readability.
+    #[prefix = "R"]
+    Readability,
 }
 
 pub trait RuleNamespace: Sized {

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))]

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<Vec<Diagnostic>> {
+        // 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::<i32>()
+                    .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"]
+    }
+}

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(())
+    }
+}

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
+   |