Repository: PlasmaFAIR/fortitude (language: python)

Issue:
Recommend `implicit none (type, external)`
This is a Fortran 2018 feature for preventing implicit procedures. The big compilers all support it: https://godbolt.org/z/hn36GjEqh
Notably `nvfortran` doesn't, so this would be an issue for CUDA projects.

Use case is something like

```f90
program foo
  implicit none

  call unknown_func() ! This compiles! (but may not link)
end program foo
```

```f90
program foo
  implicit none (external)

  call unknown_func() ! This won't compile
end program foo
```

This probably wants to be a separate rule to `implicit-typing` because not all compilers support it
Bugfix: `DiagnosticMessages` were created on pre-transformed file
When applying fixes, any remaining violations were converted to
messages using the _pre-transformed_ file, and so it was possible for
locations to be wrong


Discussion / hints:
That's a cool feature, would have saved me a few headaches when encountering some baffling linker errors. Would probably be an easy (unsafe) fix too.


Fix patch:
diff --git a/fortitude/src/check.rs b/fortitude/src/check.rs
index fbb23ee..99da6d4 100644
--- a/fortitude/src/check.rs
+++ b/fortitude/src/check.rs
@@ -493,7 +493,7 @@ pub(crate) fn check_and_fix_file<'a>(
         return Ok(FixerResult {
             result: violations
                 .into_iter()
-                .map(|v| DiagnosticMessage::from_ruff(file, v))
+                .map(|v| DiagnosticMessage::from_ruff(&transformed, v))
                 .collect_vec(),
             transformed,
             fixed,
diff --git a/fortitude/src/rules/mod.rs b/fortitude/src/rules/mod.rs
index 4b7c435..f376f9e 100644
--- a/fortitude/src/rules/mod.rs
+++ b/fortitude/src/rules/mod.rs
@@ -86,6 +86,7 @@ pub fn code_to_rule(category: Category, code: &str) -> Option<(RuleGroup, Rule)>
         (Typing, "001") => (RuleGroup::Stable, Ast, typing::implicit_typing::ImplicitTyping),
         (Typing, "002") => (RuleGroup::Stable, Ast, typing::implicit_typing::InterfaceImplicitTyping),
         (Typing, "003") => (RuleGroup::Stable, Ast, typing::implicit_typing::SuperfluousImplicitNone),
+        (Typing, "004") => (RuleGroup::Preview, Ast, typing::implicit_typing::ImplicitExternalProcedures),
         (Typing, "011") => (RuleGroup::Stable, Ast, typing::literal_kinds::LiteralKind),
         (Typing, "012") => (RuleGroup::Stable, Ast, typing::literal_kinds::LiteralKindSuffix),
         (Typing, "021") => (RuleGroup::Stable, Ast, typing::star_kinds::StarKind),
diff --git a/fortitude/src/rules/typing/implicit_typing.rs b/fortitude/src/rules/typing/implicit_typing.rs
index 31eee6b..40f0ea4 100644
--- a/fortitude/src/rules/typing/implicit_typing.rs
+++ b/fortitude/src/rules/typing/implicit_typing.rs
@@ -2,9 +2,10 @@
 use crate::ast::FortitudeNode;
 use crate::settings::Settings;
 use crate::{AstRule, FromAstNode};
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix, Violation};
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_source_file::SourceFile;
+use ruff_text_size::TextSize;
 use tree_sitter::Node;
 
 fn implicit_statement_is_none(node: &Node) -> bool {
@@ -148,3 +149,67 @@ impl AstRule for SuperfluousImplicitNone {
         vec!["implicit_statement"]
     }
 }
+
+/// ## What it does
+/// Checks if `implicit none` is missing `external`
+///
+/// ## Why is this bad?
+/// `implicit none` disables implicit types of variables but still allows
+/// implicit interfaces for procedures. Fortran 2018 added the ability to also
+/// forbid implicit interfaces through `implicit none (external)`, enabling the
+/// compiler to check the number and type of arguments and return values.
+///
+/// `implicit none` is equivalent to `implicit none (type)`, so the full
+/// statement should be `implicit none (type, external)`.
+#[violation]
+pub struct ImplicitExternalProcedures {}
+
+impl Violation for ImplicitExternalProcedures {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("'implicit none' missing 'external'")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some("Add `(external)` to 'implicit none'".to_string())
+    }
+}
+
+impl AstRule for ImplicitExternalProcedures {
+    fn check(_settings: &Settings, node: &Node, src: &SourceFile) -> Option<Vec<Diagnostic>> {
+        if !implicit_statement_is_none(node) {
+            return None;
+        }
+
+        let text = node.to_text(src.source_text())?.to_lowercase();
+
+        if !text.contains("external") {
+            let edit = if let Some(type_node) = node
+                .children(&mut node.walk())
+                .find(|child| child.to_text(src.source_text()).unwrap().to_lowercase() == "type")
+            {
+                // Seems unlikely someone would have `implicit none (type)`
+                // without `external` -- is that a sign they _explicitly_ don't
+                // want it? That's probably still unwise though
+                Edit::insertion(
+                    ", external".to_string(),
+                    TextSize::try_from(type_node.end_byte()).unwrap(),
+                )
+            } else {
+                Edit::insertion(
+                    " (type, external)".to_string(),
+                    TextSize::try_from(node.end_byte()).unwrap(),
+                )
+            };
+            let fix = Fix::unsafe_edit(edit);
+
+            some_vec!(Diagnostic::from_node(Self {}, node).with_fix(fix))
+        } else {
+            None
+        }
+    }
+
+    fn entrypoints() -> Vec<&'static str> {
+        vec!["implicit_statement"]
+    }
+}
diff --git a/fortitude/src/rules/typing/mod.rs b/fortitude/src/rules/typing/mod.rs
index bd7a9d9..104a5b4 100644
--- a/fortitude/src/rules/typing/mod.rs
+++ b/fortitude/src/rules/typing/mod.rs
@@ -21,6 +21,7 @@ mod tests {
     #[test_case(Rule::ImplicitTyping, Path::new("T001.f90"))]
     #[test_case(Rule::InterfaceImplicitTyping, Path::new("T002.f90"))]
     #[test_case(Rule::SuperfluousImplicitNone, Path::new("T003.f90"))]
+    #[test_case(Rule::ImplicitExternalProcedures, Path::new("T004.f90"))]
     #[test_case(Rule::LiteralKind, Path::new("T011.f90"))]
     #[test_case(Rule::LiteralKindSuffix, Path::new("T012.f90"))]
     #[test_case(Rule::StarKind, Path::new("T021.f90"))]


Test patch:
diff --git a/fortitude/resources/test/fixtures/typing/T004.f90 b/fortitude/resources/test/fixtures/typing/T004.f90
new file mode 100644
index 0000000..d71920a
--- /dev/null
+++ b/fortitude/resources/test/fixtures/typing/T004.f90
@@ -0,0 +1,11 @@
+module a
+  implicit none
+end module a
+
+module b
+  implicit none (type)
+end module b
+
+module c
+  implicit none (type, external)
+end module c
diff --git a/fortitude/src/rules/typing/snapshots/fortitude__rules__typing__tests__implicit-external-procedures_T004.f90.snap b/fortitude/src/rules/typing/snapshots/fortitude__rules__typing__tests__implicit-external-procedures_T004.f90.snap
new file mode 100644
index 0000000..6b189cd
--- /dev/null
+++ b/fortitude/src/rules/typing/snapshots/fortitude__rules__typing__tests__implicit-external-procedures_T004.f90.snap
@@ -0,0 +1,40 @@
+---
+source: fortitude/src/rules/typing/mod.rs
+expression: diagnostics
+snapshot_kind: text
+---
+./resources/test/fixtures/typing/T004.f90:2:3: T004 [*] 'implicit none' missing 'external'
+  |
+1 | module a
+2 |   implicit none
+  |   ^^^^^^^^^^^^^ T004
+3 | end module a
+  |
+  = help: Add `(external)` to 'implicit none'
+
+ℹ Unsafe fix
+1 1 | module a
+2   |-  implicit none
+  2 |+  implicit none (type, external)
+3 3 | end module a
+4 4 | 
+5 5 | module b
+
+./resources/test/fixtures/typing/T004.f90:6:3: T004 [*] 'implicit none' missing 'external'
+  |
+5 | module b
+6 |   implicit none (type)
+  |   ^^^^^^^^^^^^^^^^^^^^ T004
+7 | end module b
+  |
+  = help: Add `(external)` to 'implicit none'
+
+ℹ Unsafe fix
+3 3 | end module a
+4 4 | 
+5 5 | module b
+6   |-  implicit none (type)
+  6 |+  implicit none (type, external)
+7 7 | end module b
+8 8 | 
+9 9 | module c
diff --git a/fortitude/tests/check.rs b/fortitude/tests/check.rs
index 7ddee51..d2897a7 100644
--- a/fortitude/tests/check.rs
+++ b/fortitude/tests/check.rs
@@ -412,23 +412,34 @@ end program foo
     success: false
     exit_code: 1
     ----- stdout -----
+    [TEMP_FILE] T004 'implicit none' missing 'external'
+      |
+    2 | program foo
+    3 |   implicit none
+      |   ^^^^^^^^^^^^^ T004
+    4 |   real :: i
+    5 |   i = 4.0
+      |
+      = help: Add `(external)` to 'implicit none'
+
     [TEMP_FILE] P021 real has implicit kind
       |
     2 | program foo
     3 |   implicit none
-    4 |   real i
+    4 |   real :: i
       |   ^^^^ P021
     5 |   i = 4.0
     6 | contains
       |
 
     fortitude: 1 files scanned.
-    Number of errors: 3 (2 fixed, 1 remaining)
+    Number of errors: 4 (2 fixed, 2 remaining)
 
     For more information about specific rules, run:
 
         fortitude explain X001,Y002,...
 
+    No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
     ");
@@ -450,3 +461,71 @@ end program foo
 
     Ok(())
 }
+
+#[test]
+fn apply_unsafe_fixes() -> anyhow::Result<()> {
+    let tempdir = TempDir::new()?;
+    let test_file = tempdir.path().join("test.f90");
+    fs::write(
+        &test_file,
+        r#"
+program foo
+  implicit none
+  real i
+  i = 4.0
+contains
+  subroutine bar
+    implicit none
+  end subroutine bar
+end program foo
+"#,
+    )?;
+    apply_common_filters!();
+    assert_cmd_snapshot!(Command::cargo_bin(BIN_NAME)?
+                         .arg("check")
+                         .arg("--preview")
+                         .arg("--fix")
+                         .arg("--unsafe-fixes")
+                         .arg(&test_file),
+                         @r"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    [TEMP_FILE] P021 real has implicit kind
+      |
+    2 | program foo
+    3 |   implicit none (type, external)
+    4 |   real :: i
+      |   ^^^^ P021
+    5 |   i = 4.0
+    6 | contains
+      |
+
+    fortitude: 1 files scanned.
+    Number of errors: 4 (3 fixed, 1 remaining)
+
+    For more information about specific rules, run:
+
+        fortitude explain X001,Y002,...
+
+
+    ----- stderr -----
+    ");
+
+    let expected = r#"
+program foo
+  implicit none (type, external)
+  real :: i
+  i = 4.0
+contains
+  subroutine bar
+  end subroutine bar
+end program foo
+"#
+    .to_string();
+
+    let transformed = fs::read_to_string(&test_file)?;
+    assert_eq!(transformed, expected);
+
+    Ok(())
+}