Skip to content

Implement unused variable checker on HIR #4055

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Implement unused variable checker on HIR #4055

wants to merge 7 commits into from

Conversation

sakupan102
Copy link
Contributor

@sakupan102 sakupan102 commented Aug 10, 2025 •
edited
Loading

This PR closes #4086, #4085

@sakupan102 sakupan102 changed the title Implement unused variable checker on HIR. Implement unused variable checker on HIR Aug 10, 2025
@sakupan102
Implement unused variable checker on HIR.
f27c7a7 
gcc/rust/ChangeLog:

	* Make-lang.in: New file.
	* checks/lints/unused-var/rust-unused-var-checker.cc (UnusedVarChecker): Implement unused
	variable checker.
	* checks/lints/unused-var/rust-unused-var-checker.h (UnusedVarChecker): Implement unused
	variable checker.
	* checks/lints/unused-var/rust-unused-var-collector.cc (UnusedVarCollector): Implement
	unused variable collector.
	* checks/lints/unused-var/rust-unused-var-collector.h (UnusedVarCollector): Implement
	unused variable collector.
	* checks/lints/unused-var/rust-unused-var-context.cc (UnusedVarContext): Implement
	unused variable context.
	* checks/lints/unused-var/rust-unused-var-context.h (UnusedVarContext): Implement unused
	variable context.

Signed-off-by: Ryutaro Okada <[email protected]>
CohenArthur
CohenArthur reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

I think this is a really good base and the code looks good to me! I think it is important to add testcases for features like these, so it would be good if you could add a flag to enable the lints you've written and then add testcases to the testsuite

gcc/rust/checks/lints/unused-var/rust-unused-var-context.cc Outdated
Comment on lines 24 to 31
UnusedVarContext *
UnusedVarContext::get ()
{
static UnusedVarContext *instance;
if (instance == nullptr)
instance = new UnusedVarContext ();
return instance;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine but we should also check if we can do without a global variable

gcc/rust/checks/lints/unused-var/rust-unused-var-collector.cc Outdated
UnusedVarCollector::visit (HIR::LetStmt &stmt)
{
HIR::Pattern &pattern = stmt.get_pattern ();
collect_variable (pattern);
Copy link
Member

Choose a reason for hiding this comment

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

can't we visit instead here? mabye I'm missing something but I think all patterns should create variables to check for, so we could just do visit(pattern) and then create visitors for all our kinds of patterns instead of manually dispatching like in collect_variable

CohenArthur
CohenArthur approved these changes Aug 26, 2025
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

Thank you @sakupan102, this is really good work!!! Just a couple nitpicks and we should merge this. Well done :)

gcc/rust/checks/lints/unused-var/rust-unused-var-collector.h Outdated
Comment on lines 54 to 57
if (auto id = nr_context.lookup (ast_node_id))
def_id = *id;
else
return;
Copy link
Member

Choose a reason for hiding this comment

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

so if we fail to lookup the variable we silently return and do nothing - I think it would be worth crashing if we encounter this instead, as it should never happen

Suggested change
if (auto id = nr_context.lookup (ast_node_id))
def_id = *id;
else
return;
NodeId def_id = nr_context.lookup (ast_node_id).value();

gcc/rust/checks/lints/unused-var/rust-unused-var-collector.h Outdated
Comment on lines 60 to 61
if (!hir_id)
return;
Copy link
Member

Choose a reason for hiding this comment

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

likewise

gcc/testsuite/rust/unused-var/unused_var.exp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need this file, instead...

gcc/testsuite/rust/unused-var/template_function_0.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

... You can move this file to the regular gcc/testsuite/rust/compile folder, and add the following line to the top of the file:

// { dg-additional-options "-frust-unused-check-2.0" }

gcc/testsuite/rust/unused-var/static_item_0.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here :)

@sakupan102
Copy link
Contributor Author

@CohenArthur
I made the suggested changes based on your review. If everything looks good, I'll squash the commits.

@sakupan102 sakupan102 requested a review from CohenArthur August 27, 2025 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CohenArthur CohenArthur Awaiting requested review from CohenArthur

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unused check for template function
2 participants
@sakupan102 @CohenArthur