-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, many thanks for the contribution! ❤️
Overall this is looking great, but a few small changes will be needed before we can merge:
- The fix doesn't work in cases where a
::
separator isn't present, such asuse iso_fortran_env
. As a::
is optional for a plainuse
statement but necessary after addingintrinsic
, we need to add it if it's missing. I've added a suggestion below that I think fixes the issue, and we'll need to add a plainuse
statement to the tests too. - The fix has the potential to break a user's code if they're deliberately using their own module named
iso_fortran_env
(or one of the other intrinsic modules) for whatever reason, so unfortunately it'll have to be listed as an unsafe fix. - We're following Ruff's rule naming convention, so I think a better name for it would be
MissingIntrinsic
, as that would mean users could ignore this rule by adding the comment! allow(missing-intrinsic)
. With that said, I'm only just now realising that the ruleUseAll
in the same file would be better namedMissingOnly
, but as that would be a breaking change I'll leave it for another PR.
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)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following edit will handle the case where use
is used without a ::
separator, and it downgrades the fix to 'unsafe'. I'm unsure if checking child(1)
for ::
is the most robust solution though.
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)]; | |
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::unsafe_edit(Edit::insertion(intrinsic.to_string(), start_pos)); | |
return some_vec![Diagnostic::from_node(UseIntrinsic {}, node).with_fix(fix)]; |
"ieee_features", | ||
]; | ||
|
||
impl AlwaysFixableViolation for UseIntrinsic { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be changed to just Violation
fn fix_title(&self) -> String { | ||
"Add 'intrinsic'".to_string() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing to plain Violation
:
fn fix_title(&self) -> String { | |
"Add 'intrinsic'".to_string() | |
} | |
fn fix_title(&self) -> Option<String> { | |
Some("Add 'intrinsic'".to_string()) | |
} |
This adds a linting rule that checks
use
statements for if they refer to an intrinsic module defined in the standard, and if so that theuse
statement includes theintrinsic
modifier (or alternately, thenon_intrinsic
modifier if someone really wants to override a compiler intrinsic module). Including it is good practice since the compiler will prioritize modules outside the compiler first, so this makes the need for the compiler version explicit.