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

Avoid false positive for missing return #78

Merged

Conversation

xgouchet
Copy link
Contributor

@xgouchet xgouchet commented Oct 3, 2022

Given a function with throw statements in some branches, there is a false positive that considers those branches as invalid. A throw can be a valid exit strategy for a function when "something truly terrible happens".

function ok7() as integer
    a = 1
    if a > 0
        throw "something wrong"
    else
        return 0
    end if
end function

@xgouchet
Copy link
Contributor Author

xgouchet commented Oct 3, 2022

This also upgrades the Brighterscript dependency to 0.59.0 to use the isThrowStatement method.

@xgouchet xgouchet force-pushed the xgouchet/consistent_return branch 3 times, most recently from 60ac7a5 to 75ec235 Compare October 3, 2022 15:42
@xgouchet
Copy link
Contributor Author

xgouchet commented Oct 3, 2022

Seems like some tests are failing in CI with the 0.59.0 upgrade, I'll fix those asap 👍

@elsassph
Copy link
Collaborator

elsassph commented Oct 3, 2022

Thanks, the return tracking was implemented before throw was introduced!

@elsassph
Copy link
Collaborator

elsassph commented Oct 4, 2022

Can you check if it fixes #68

@elsassph elsassph closed this Oct 4, 2022
@elsassph elsassph reopened this Oct 4, 2022
@xgouchet
Copy link
Contributor Author

xgouchet commented Oct 4, 2022

Can you check if it fixes #68

No it doesn't but I'll add that to my PR

@elsassph
Copy link
Collaborator

elsassph commented Oct 5, 2022

Code looks good, but needs testing at scale.

@TwitchBronBron
Copy link
Member

I can test this on our app today.

@TwitchBronBron
Copy link
Member

TwitchBronBron commented Oct 13, 2022

Sorry for the delay. I finally got around to testing this, and it looks like it works well against our larger project! The only change I would suggest is bumping the peerDependency value up since this will fail for older brighterscript versions.

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

Successfully merging this pull request may close these issues.

None yet

3 participants