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

Separate inconsistent_or_missing_returns_checker into two checkers #1111

Open
Raine-Yang-UofT opened this issue Nov 4, 2024 · 0 comments
Open

Comments

@Raine-Yang-UofT
Copy link
Contributor

The inconsistent_or_missing_returns_checker handles two error messages inconsistent-returns and missing-return-statement as the logic to detect the two errors are similar. However, for readability and expandability concern, it could be a better design to separate this checker into two, one specifically for each error message.

For example, during the integration of z3 option into this checker, we only want missing-return-statement to be affected by z3 option. However, as the checking for two messages are tightly coupled, we have to implement a messy and suboptimal solution in our current codebase:

# At Line 85
# ignore unfeasible edges for missing return if z3 option is on
if self.linter.config.z3 and (
    not block.is_feasible
    or not any(
        edge.is_feasible for edge in block.successors if edge.target is end
   )
):

We could have directly filters out blocks whose edge connecting to the end node is unfeasible, which replaces statement or not any(edge.is_feasible for edge in block.successors if edge.target is end) with the following modification:

Change

# At Line 60
# get the blocks connected to the end of cfg
end = node.cfg.end
end_blocks = [edge.source for edge in end.predecessors]

to

# At Line 60
# get the blocks connected to the end of cfg
end = node.cfg.end
end_blocks = [edge.source for edge in end.predecessors if edge.is_feasible]

However, this change would cause errors in inconsistent-returns in our current checker.

In addition, in our current implementation, we need to maintain unit tests for z3 options for inconsistent-returns even though z3 is not used for this message. This is to ensure that modifications on missing-return-statement z3 option would also cause accidental changes to inconsistent-returns. Refactoring the checker would also us to remove the redundant test cases.

@Raine-Yang-UofT Raine-Yang-UofT changed the title Separate inconsistent_or_missing_returns_checker into two separate checkers Separate inconsistent_or_missing_returns_checker into two checkers Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant