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/RCS1031 and RCS1208 #1058

Closed

Conversation

jamesHargreaves12
Copy link
Contributor

@jamesHargreaves12 jamesHargreaves12 commented Mar 21, 2023

This PR extends the local variable check from #1039 to also check variables assigned during pattern matching for RCS1031.

Slightly embarrassingly, I didn't check all the ReduceIfNestingAnalysis code paths in #1039 and so I didn't realise that the AnalyzeDataFlow can only handle an argument of type ExpressionSyntax or StatementSyntax. If you pass it anything else then it will throw a run time error. This PR fixes this issue and adds a number of new tests to enforce.

It is currently not trivial to test the Loop / Switch sections of ReduceIfNestingAnalysis.AnalyzeCore via the analyzer as it passes through ReduceIfNestingOptions.None. However, I manually removed the guard clause on lines 63 and 90 and confirmed that it was working correctly.

@josefpihrt
Copy link
Collaborator

josefpihrt commented Apr 3, 2023

@jamesHargreaves12 What was the reason for closing this PR?

@jamesHargreaves12
Copy link
Contributor Author

jamesHargreaves12 commented Apr 3, 2023

@jamesHargreaves12 What was the reason for closing this PR?

Yesterday while testing something else I found another change that made sense to be included within this PR but did not have time to add unit tests and create a PR for it. So I closed the PR to avoid you spending any time reviewing it until I had a chance to include those changes. The PR with the updates is here #1062

@josefpihrt
Copy link
Collaborator

@jamesHargreaves12 👍

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.

2 participants