Repository: PlasmaFAIR/fortitude (language: python)

Issue:
Warn about modules without `private` statements
I think it's a good idea to have a private statement in a module, requiring the programmer to explicitly list the public entities, so I request that fortitude have an option to warn about a module such as the one below.

```
module m
implicit none
integer :: i
end module m
```

If such a warning is added, whether it should be turned on by default is a matter of taste.




Discussion / hints:
I fully agree with this! It's something I aim for in my own code, but I'm unsure if it should be part of the default rule set.

Following this, we could implement another rule to check for 'dead code' (types/procedures that aren't used anywhere), as it's likely a bug if something isn't used within a module and isn't exported. If everything in a module is public by default, it becomes much harder to catch that sort of error.

Fix patch:
diff --git a/fortitude/src/rules/mod.rs b/fortitude/src/rules/mod.rs
index 5116d01..3719b77 100644
--- a/fortitude/src/rules/mod.rs
+++ b/fortitude/src/rules/mod.rs
@@ -99,6 +99,8 @@ pub fn code_to_rule(category: Category, code: &str) -> Option<(RuleGroup, Rule)>
 
         (Modules, "001") => (RuleGroup::Stable, Ast, modules::external_functions::ExternalFunction),
         (Modules, "011") => (RuleGroup::Stable, Ast, modules::use_statements::UseAll),
+        (Modules, "021") => (RuleGroup::Preview, Ast, modules::accessibility_statements::MissingAccessibilityStatement),
+        (Modules, "022") => (RuleGroup::Preview, Ast, modules::accessibility_statements::DefaultPublicAccessibility),
 
         // Rules for testing fortitude
         // Couldn't get a separate `Testing` category working for some reason
diff --git a/fortitude/src/rules/modules/accessibility_statements.rs b/fortitude/src/rules/modules/accessibility_statements.rs
new file mode 100644
index 0000000..4fb5574
--- /dev/null
+++ b/fortitude/src/rules/modules/accessibility_statements.rs
@@ -0,0 +1,112 @@
+use crate::ast::FortitudeNode;
+use crate::settings::Settings;
+use crate::{AstRule, FromAstNode};
+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 missing `private` or `public` accessibility statements in modules
+///
+/// ## Why is this bad?
+/// The `private` statement makes all entities (variables, types, procedures)
+/// private by default, requiring an explicit `public` attribute to make them
+/// available. As well as improving encapsulation between modules, this also
+/// makes it possible to detect unused entities.
+///
+/// A `public` statement in a module does not change the default behaviour,
+/// and therefore all entities will be available from outside the module
+/// unless they are individually given a `private` attribute. This brings
+/// all of the same downsides as the default behaviour, but an explicit
+/// `public` statement makes it clear that the programmer is choosing
+/// this behaviour intentionally.  
+#[violation]
+pub struct MissingAccessibilityStatement {
+    name: String,
+}
+
+impl Violation for MissingAccessibilityStatement {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!(
+            "module '{}' missing default accessibility statement",
+            self.name
+        )
+    }
+}
+
+impl AstRule for MissingAccessibilityStatement {
+    fn check(_settings: &Settings, node: &Node, src: &SourceFile) -> Option<Vec<Diagnostic>> {
+        let module = node.parent()?;
+
+        let bare_private_statement = match module.child_with_name("private_statement") {
+            Some(statement) => statement.named_child(0).is_none(),
+            None => false,
+        };
+
+        let bare_public_statement = match module.child_with_name("public_statement") {
+            Some(statement) => statement.named_child(0).is_none(),
+            None => false,
+        };
+
+        // No statement whatsoever
+        if !bare_private_statement && !bare_public_statement {
+            let name = node.named_child(0)?.to_text(src.source_text())?.to_string();
+            return some_vec![Diagnostic::from_node(
+                MissingAccessibilityStatement { name },
+                node
+            )];
+        }
+
+        None
+    }
+
+    fn entrypoints() -> Vec<&'static str> {
+        vec!["module_statement"]
+    }
+}
+
+/// ## What it does
+/// Checks if the default accessibility in modules is set to `public`
+///
+/// ## Why is this bad?
+/// The `public` statement makes all entities (variables, types, procedures)
+/// public by default. This decreases encapsulation and makes it more likely to
+/// accidentally expose more than necessary. Public accessibility also makes
+/// it harder to detect unused entities, which can often be indicative of
+/// errors within the code.
+#[violation]
+pub struct DefaultPublicAccessibility {
+    name: String,
+}
+
+impl Violation for DefaultPublicAccessibility {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("module '{}' has default `public` accessibility", self.name)
+    }
+}
+
+impl AstRule for DefaultPublicAccessibility {
+    fn check(_settings: &Settings, node: &Node, src: &SourceFile) -> Option<Vec<Diagnostic>> {
+        let module = node.parent()?;
+
+        let public_statement = module.child_with_name("public_statement")?;
+
+        // Bare `public` statement`
+        if public_statement.named_child(0).is_none() {
+            let name = node.named_child(0)?.to_text(src.source_text())?.to_string();
+            return some_vec![Diagnostic::from_node(
+                DefaultPublicAccessibility { name },
+                node
+            )];
+        }
+
+        None
+    }
+
+    fn entrypoints() -> Vec<&'static str> {
+        vec!["module_statement"]
+    }
+}
diff --git a/fortitude/src/rules/modules/mod.rs b/fortitude/src/rules/modules/mod.rs
index d4ff743..b335e7b 100644
--- a/fortitude/src/rules/modules/mod.rs
+++ b/fortitude/src/rules/modules/mod.rs
@@ -1,3 +1,4 @@
+pub mod accessibility_statements;
 pub mod external_functions;
 pub mod use_statements;
 
@@ -20,6 +21,8 @@ mod tests {
 
     #[test_case(Rule::ExternalFunction, Path::new("M001.f90"))]
     #[test_case(Rule::UseAll, Path::new("M011.f90"))]
+    #[test_case(Rule::MissingAccessibilityStatement, Path::new("M021.f90"))]
+    #[test_case(Rule::DefaultPublicAccessibility, Path::new("M022.f90"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
         let diagnostics = test_path(


Test patch:
diff --git a/fortitude/resources/test/fixtures/modules/M021.f90 b/fortitude/resources/test/fixtures/modules/M021.f90
new file mode 100644
index 0000000..b61f60a
--- /dev/null
+++ b/fortitude/resources/test/fixtures/modules/M021.f90
@@ -0,0 +1,34 @@
+module good_module
+  ! This is good
+  private
+  public :: foo
+  integer :: foo
+end module good_module
+
+module ok_module
+  ! This is ok
+  public
+end module ok_module
+
+module ok_module2
+  ! This is ok too
+  public
+  private :: foo
+  integer :: foo
+end module ok_module2
+
+module bad_module
+  ! This is bad
+end module bad_module
+
+module only_some_private_module
+  ! This is bad
+  private :: foo
+  public :: bar
+  integer :: foo
+  integer :: bar
+end module only_some_private_module
+
+program test
+  ! Obviously fine
+end program test
diff --git a/fortitude/resources/test/fixtures/modules/M022.f90 b/fortitude/resources/test/fixtures/modules/M022.f90
new file mode 100644
index 0000000..e466105
--- /dev/null
+++ b/fortitude/resources/test/fixtures/modules/M022.f90
@@ -0,0 +1,23 @@
+module good_module
+  ! This is good
+  private
+  public :: foo
+  integer :: foo
+end module good_module
+
+module bad_module
+  ! This is bad
+  public
+end module bad_module
+
+module only_some_private_module
+  ! This is bad
+  private :: foo
+  public :: bar
+  integer :: foo
+  integer :: bar
+end module only_some_private_module
+
+program test
+  ! Obviously fine
+end program test
diff --git a/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__default-public-accessibility_M022.f90.snap b/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__default-public-accessibility_M022.f90.snap
new file mode 100644
index 0000000..fd35a98
--- /dev/null
+++ b/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__default-public-accessibility_M022.f90.snap
@@ -0,0 +1,14 @@
+---
+source: fortitude/src/rules/modules/mod.rs
+expression: diagnostics
+snapshot_kind: text
+---
+./resources/test/fixtures/modules/M022.f90:8:1: M022 module 'bad_module' has default `public` accessibility
+   |
+ 6 | end module good_module
+ 7 |
+ 8 | module bad_module
+   | ^^^^^^^^^^^^^^^^^ M022
+ 9 |   ! This is bad
+10 |   public
+   |
diff --git a/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__missing-accessibility-statement_M021.f90.snap b/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__missing-accessibility-statement_M021.f90.snap
new file mode 100644
index 0000000..40e9c41
--- /dev/null
+++ b/fortitude/src/rules/modules/snapshots/fortitude__rules__modules__tests__missing-accessibility-statement_M021.f90.snap
@@ -0,0 +1,24 @@
+---
+source: fortitude/src/rules/modules/mod.rs
+expression: diagnostics
+snapshot_kind: text
+---
+./resources/test/fixtures/modules/M021.f90:20:1: M021 module 'bad_module' missing default accessibility statement
+   |
+18 | end module ok_module2
+19 |
+20 | module bad_module
+   | ^^^^^^^^^^^^^^^^^ M021
+21 |   ! This is bad
+22 | end module bad_module
+   |
+
+./resources/test/fixtures/modules/M021.f90:24:1: M021 module 'only_some_private_module' missing default accessibility statement
+   |
+22 | end module bad_module
+23 |
+24 | module only_some_private_module
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ M021
+25 |   ! This is bad
+26 |   private :: foo
+   |