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

Interface Analyzer #8923

Merged
merged 10 commits into from May 24, 2021
Merged

Interface Analyzer #8923

merged 10 commits into from May 24, 2021

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented May 22, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

  • This PR adds in a static analysis tool, which parses the AST for the selected interfaces and then checks if the appropriate
    equality checks are being compared.
  • Fixes across prysm for issues found by the static analyzer.

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas added the Ready For Review A pull request ready for code review label May 22, 2021
@nisdas nisdas requested a review from a team as a code owner May 22, 2021 10:54
@rkapka
Copy link
Contributor

rkapka commented May 23, 2021

Why do you add a new check instead of replacing the existing one? After all IsNIl does include a nilness check on the receiver.

@nisdas
Copy link
Member Author

nisdas commented May 23, 2021

@rkapka because the interface itself could be nil, so running any receiver methods would create a panic

@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #8923 (2e5ec37) into develop (09d7efe) will decrease coverage by 0.01%.
The diff coverage is 39.39%.

@@             Coverage Diff             @@
##           develop    #8923      +/-   ##
===========================================
- Coverage    60.89%   60.87%   -0.02%     
===========================================
  Files          528      528              
  Lines        37336    37337       +1     
===========================================
- Hits         22735    22729       -6     
  Misses       11344    11344              
- Partials      3257     3264       +7     

}
if xIsIface && yIsNil {
pass.Reportf(identX.Pos(), "A nilness check is being performed on an interface"+
", this check needs another accompanying check on the underlying object for the interface")
Copy link
Member

Choose a reason for hiding this comment

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

Little confusing. What does "another accompanying check" mean? Better to be specific

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Nice job!

@terencechain terencechain merged commit 76cdbac into develop May 24, 2021
@nisdas nisdas deleted the interfaceAnalyzer branch May 24, 2021 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants