Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rule to find missing intrinsic specifiers in use statements #253

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
| ---- | ---- | ------- | ------: |
| M001 | [procedure-not-in-module](rules/procedure-not-in-module.md) | {procedure} not contained within (sub)module or program | <span title='Rule is stable' style='opacity: 0.6'>✔️</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
| M011 | [use-all](rules/use-all.md) | 'use' statement missing 'only' clause | <span title='Rule is stable' style='opacity: 0.6'>✔️</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
| M012 | [missing-intrinsic](rules/missing-intrinsic.md) | 'use' for intrinsic module missing 'intrinsic' modifier | <span title='Rule is in preview'>🧪</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
| M021 | [missing-accessibility-statement](rules/missing-accessibility-statement.md) | module '{}' missing default accessibility statement | <span title='Rule is in preview'>🧪</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |
| M022 | [default-public-accessibility](rules/default-public-accessibility.md) | module '{}' has default `public` accessibility | <span title='Rule is in preview'>🧪</span> <span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>🛠️</span> |

Expand Down
23 changes: 23 additions & 0 deletions docs/rules/missing-intrinsic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# missing-intrinsic (M012)
This rule is unstable and in [preview](../preview.md). The `--preview` flag is required for use.

## What it does
Checks whether `use` statements for intrinic modules specify `intrinsic` or
`non_intrinsic`.

## Why is this bad?
The compiler will default to using a non-intrinsic module, if there is one,
so not specifying the `intrinsic` modifier on intrinsic modules may lead to
the compiler version being shadowed by a different module with the same name.

## Example
```f90
! Not recommended
use :: iso_fortran_env, only: int32, real64

! Better
use, intrinsic :: iso_fortran_env, only: int32, real64
```

This ensures the compiler will use the built-in module instead of a different
module with the same name.
8 changes: 8 additions & 0 deletions fortitude/resources/test/fixtures/modules/M012.f90
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
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
use :: my_other_module
end module my_module
1 change: 1 addition & 0 deletions fortitude/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +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::MissingIntrinsic),
(Modules, "021") => (RuleGroup::Preview, Ast, modules::accessibility_statements::MissingAccessibilityStatement),
(Modules, "022") => (RuleGroup::Preview, Ast, modules::accessibility_statements::DefaultPublicAccessibility),

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

#[test_case(Rule::ProcedureNotInModule, Path::new("M001.f90"))]
#[test_case(Rule::UseAll, Path::new("M011.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
80 changes: 79 additions & 1 deletion fortitude/src/rules/modules/use_statements.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use crate::ast::FortitudeNode;
use crate::settings::Settings;
use crate::{AstRule, FromAstNode};
use ruff_diagnostics::{Diagnostic, 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;
use tree_sitter::Node;

// TODO Check that 'used' entity is actually used somewhere
Expand Down Expand Up @@ -49,3 +50,80 @@ impl AstRule for UseAll {
vec!["use_statement"]
}
}

/// ## What it does
/// Checks whether `use` statements for intrinic modules specify `intrinsic` or
/// `non_intrinsic`.
///
/// ## Why is this bad?
/// The compiler will default to using a non-intrinsic module, if there is one,
/// so not specifying the `intrinsic` modifier on intrinsic modules may lead to
/// the compiler version being shadowed by a different module with the same name.
///
/// ## Example
/// ```f90
/// ! Not recommended
/// use :: iso_fortran_env, only: int32, real64
///
/// ! Better
/// use, intrinsic :: iso_fortran_env, only: int32, real64
/// ```
///
/// This ensures the compiler will use the built-in module instead of a different
/// module with the same name.
#[violation]
pub struct MissingIntrinsic {}

const INTRINSIC_MODULES: &[&str] = &[
"iso_fortran_env",
"iso_c_binding",
"ieee_exceptions",
"ieee_artimetic",
"ieee_features",
];

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

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

impl AstRule for MissingIntrinsic {
fn check(_settings: &Settings, node: &Node, _src: &SourceFile) -> Option<Vec<Diagnostic>> {
let module_name = node
.child_with_name("module_name")?
.to_text(_src.source_text())?
.to_lowercase();

if INTRINSIC_MODULES.iter().any(|&m| m == module_name)
&& node
.children(&mut node.walk())
.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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let fix = Fix::safe_edit(Edit::insertion(intrinsic.to_string(), start_pos));
let fix = Fix::unsafe_edit(Edit::insertion(intrinsic.to_string(), start_pos));


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

fn entrypoints() -> Vec<&'static str> {
vec!["use_statement"]
}
}
Loading