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

fix(sem): avoid no-return false positives #1452

Merged
merged 3 commits into from
Sep 7, 2024

Conversation

saem
Copy link
Collaborator

@saem saem commented Sep 7, 2024

Summary

no-return analysis now no longer confuses void branches for true
no-returns.

Details

No-return analysis now dives into the trees of if/try/block/case
statements in order to determine if they are in fact no-return
scenarios. This is only done for the statement variety.

The updated analysis besides being more correct, in that it avoids false
positive no-return identification, it still allows for a number of false
negatives. Chiefly that unstructured exits ( return/break/raise ) are
only considered in the trailing position of statement lists and not part
way through. This is considered acceptable as there is an overall
improvement in accuracy and it should cover most code.

This includes a fix for an NPE crash in discard-or-use error reporting.

@saem saem added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler labels Sep 7, 2024
@saem saem added the compiler/msgs Compiler output and diagnostic subsystem: error and warnig reporting, information, debugging label Sep 7, 2024
@saem saem requested a review from zerbina September 7, 2024 21:41
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

Looks good to me. Using recursion might make the analysis a tad easier to understand/read, but endsInNoReturn will be obsoleted by a proper void type anyway.

@zerbina
Copy link
Collaborator

zerbina commented Sep 7, 2024

/merge

Copy link

github-actions bot commented Sep 7, 2024

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • Found via Loony CI and reduced thanks to @alaviss

@chore-runner chore-runner bot added this pull request to the merge queue Sep 7, 2024
@saem
Copy link
Collaborator Author

saem commented Sep 7, 2024

Thanks for the review.

Looks good to me. Using recursion might make the analysis a tad easier to understand/read, but endsInNoReturn will be obsoleted by a proper void type anyway.

I did think about it, but already felt it's a bit pricey (cost to benefit) as is and didn't want to push it any further than I already have.

Merged via the queue into nim-works:devel with commit 961aaa7 Sep 7, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/msgs Compiler output and diagnostic subsystem: error and warnig reporting, information, debugging compiler/sem Related to semantic-analysis system of the compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants