Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
* Mark rule as unsafe
* Handle case of use without ::
* Rename rule to follow naming conventions
  • Loading branch information
imciner2 committed Jan 6, 2025
1 parent b1de3b3 commit 5dadbd9
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 32 deletions.
2 changes: 2 additions & 0 deletions fortitude/resources/test/fixtures/modules/M012.f90
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
module my_module
use iso_fortran_env
use my_other_module
use :: iso_fortran_env, only: real32
use, intrinsic :: iso_c_binding
use, non_intrinsic :: iso_c_binding
Expand Down
2 changes: 1 addition & 1 deletion fortitude/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub fn code_to_rule(category: Category, code: &str) -> Option<(RuleGroup, Rule)>

(Modules, "001") => (RuleGroup::Stable, Ast, modules::external_functions::ProcedureNotInModule),
(Modules, "011") => (RuleGroup::Stable, Ast, modules::use_statements::UseAll),
(Modules, "012") => (RuleGroup::Preview, Ast, modules::use_statements::UseIntrinsic),
(Modules, "012") => (RuleGroup::Preview, Ast, modules::use_statements::MissingIntrinsic),
(Modules, "021") => (RuleGroup::Preview, Ast, modules::accessibility_statements::MissingAccessibilityStatement),
(Modules, "022") => (RuleGroup::Preview, Ast, modules::accessibility_statements::DefaultPublicAccessibility),

Expand Down
2 changes: 1 addition & 1 deletion fortitude/src/rules/modules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ mod tests {

#[test_case(Rule::ProcedureNotInModule, Path::new("M001.f90"))]
#[test_case(Rule::UseAll, Path::new("M011.f90"))]
#[test_case(Rule::UseIntrinsic, Path::new("M012.f90"))]
#[test_case(Rule::MissingIntrinsic, Path::new("M012.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<()> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
---
source: fortitude/src/rules/modules/mod.rs
expression: diagnostics
---
./resources/test/fixtures/modules/M012.f90:2:5: M012 [*] 'use' for intrinsic module missing 'intrinsic' modifier
|
1 | module my_module
2 | use iso_fortran_env
| ^^^^^^^^^^^^^^^^^^^ M012
3 | use my_other_module
4 | use :: iso_fortran_env, only: real32
|
= help: Add 'intrinsic'

Safe fix
1 1 | module my_module
2 |- use iso_fortran_env
2 |+ use, intrinsic :: iso_fortran_env
3 3 | use my_other_module
4 4 | use :: iso_fortran_env, only: real32
5 5 | use, intrinsic :: iso_c_binding

./resources/test/fixtures/modules/M012.f90:4:5: M012 [*] 'use' for intrinsic module missing 'intrinsic' modifier
|
2 | use iso_fortran_env
3 | use my_other_module
4 | use :: iso_fortran_env, only: real32
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ M012
5 | use, intrinsic :: iso_c_binding
6 | use, non_intrinsic :: iso_c_binding
|
= help: Add 'intrinsic'

Safe fix
1 1 | module my_module
2 2 | use iso_fortran_env
3 3 | use my_other_module
4 |- use :: iso_fortran_env, only: real32
4 |+ use, intrinsic :: iso_fortran_env, only: real32
5 5 | use, intrinsic :: iso_c_binding
6 6 | use, non_intrinsic :: iso_c_binding
7 7 | use :: my_other_module

This file was deleted.

22 changes: 14 additions & 8 deletions fortitude/src/rules/modules/use_statements.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::ast::FortitudeNode;
use crate::settings::Settings;
use crate::{AstRule, FromAstNode};
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, Violation};
use ruff_diagnostics::{Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_source_file::SourceFile;
use ruff_text_size::TextSize;
Expand Down Expand Up @@ -72,7 +72,7 @@ impl AstRule for UseAll {
/// This ensures the compiler will use the built-in module instead of a different
/// module with the same name.
#[violation]
pub struct UseIntrinsic {}
pub struct MissingIntrinsic {}

const INTRINSIC_MODULES: &[&str] = &[
"iso_fortran_env",
Expand All @@ -82,18 +82,18 @@ const INTRINSIC_MODULES: &[&str] = &[
"ieee_features",
];

impl AlwaysFixableViolation for UseIntrinsic {
impl Violation for MissingIntrinsic {
#[derive_message_formats]
fn message(&self) -> String {
format!("'use' for intrinsic module missing 'intrinsic' modifier")
}

fn fix_title(&self) -> String {
"Add 'intrinsic'".to_string()
fn fix_title(&self) -> Option<String> {
Some("Add 'intrinsic'".to_string())
}
}

impl AstRule for UseIntrinsic {
impl AstRule for MissingIntrinsic {
fn check(_settings: &Settings, node: &Node, _src: &SourceFile) -> Option<Vec<Diagnostic>> {
let module_name = node
.child_with_name("module_name")?
Expand All @@ -106,13 +106,19 @@ impl AstRule for UseIntrinsic {
.filter_map(|child| child.to_text(_src.source_text()))
.all(|child| child != "intrinsic" && child != "non_intrinsic")
{
let intrinsic = if node.child(1)?.kind() == "::" {
", intrinsic"
} else {
", intrinsic ::"
};

let use_field = node
.children(&mut node.walk())
.find(|&child| child.to_text(_src.source_text()) == Some("use"))?;
let start_pos = TextSize::try_from(use_field.end_byte()).unwrap();
let fix = Fix::safe_edit(Edit::insertion(", intrinsic".to_string(), start_pos));
let fix = Fix::safe_edit(Edit::insertion(intrinsic.to_string(), start_pos));

return some_vec![Diagnostic::from_node(UseIntrinsic {}, node).with_fix(fix)];
return some_vec![Diagnostic::from_node(MissingIntrinsic {}, node).with_fix(fix)];
}
None
}
Expand Down

0 comments on commit 5dadbd9

Please sign in to comment.