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

Merged
merged 3 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 | [use-intrinsic](rules/use-intrinsic.md) | 'use' for intrinsic module missing 'intrinsic' modifier | <span title='Rule is in preview'>🧪</span> <span title='Automatic fix available'>🛠️</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
25 changes: 25 additions & 0 deletions docs/rules/use-intrinsic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# use-intrinsic (M012)
Fix is always available.

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.
6 changes: 6 additions & 0 deletions fortitude/resources/test/fixtures/modules/M012.f90
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module my_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::UseIntrinsic),
(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::UseIntrinsic, 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,22 @@
---
source: fortitude/src/rules/modules/mod.rs
expression: diagnostics
snapshot_kind: text
---
./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, only: real32
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ M012
3 | use, intrinsic :: iso_c_binding
4 | use, non_intrinsic :: iso_c_binding
|
= help: Add 'intrinsic'

ℹ Safe fix
1 1 | module my_module
2 |- use :: iso_fortran_env, only: real32
2 |+ use, intrinsic :: iso_fortran_env, only: real32
3 3 | use, intrinsic :: iso_c_binding
4 4 | use, non_intrinsic :: iso_c_binding
5 5 | use :: my_other_module
74 changes: 73 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::{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;

// TODO Check that 'used' entity is actually used somewhere
Expand Down Expand Up @@ -49,3 +50,74 @@ 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 UseIntrinsic {}

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

impl AlwaysFixableViolation for UseIntrinsic {
imciner2 marked this conversation as resolved.
Show resolved Hide resolved
#[derive_message_formats]
fn message(&self) -> String {
format!("'use' for intrinsic module missing 'intrinsic' modifier")
}

fn fix_title(&self) -> String {
"Add 'intrinsic'".to_string()
}
imciner2 marked this conversation as resolved.
Show resolved Hide resolved
}

impl AstRule for UseIntrinsic {
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 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));

return some_vec![Diagnostic::from_node(UseIntrinsic {}, node).with_fix(fix)];
imciner2 marked this conversation as resolved.
Show resolved Hide resolved
}
None
}

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