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

Add static analysis to enforce usage of InitWithReset #5654

Merged
merged 13 commits into from
Apr 28, 2020

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Apr 27, 2020

What type of PR is this?

Feature

What does this PR do? Why is it needed?

#5650 introduce a reset function for featureconfig. This PR adds build time static analysis to enforce usage of the reset method.

Which issues(s) does this PR fix?

N/A - Minor follow up to #5650

Other notes for review

Sorry for these other random commits in the branch. Also fixes a few gaps missed by #5650

@prestonvanloon prestonvanloon requested a review from a team as a code owner April 27, 2020 20:43
@prestonvanloon prestonvanloon added the Ready For Review A pull request ready for code review label Apr 27, 2020
@prestonvanloon
Copy link
Member Author

Oops, need to fix a few usages!

Copy link
Contributor

@farazdagi farazdagi 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 (AST parsing especially).

What I've found running the thing locally:

  • it seems featureconfig analyzer catches the false positive (fails when trying to build: on file connmgr_test.go which doesn't have any featureconfig references).
  • analyzer.go needs go fmt applied
  • Couple of more places featureconfig.Init was used:
diff --git a/shared/featureconfig/config.go b/shared/featureconfig/config.go
index b111f18e0..549bdfaf8 100644
--- a/shared/featureconfig/config.go
+++ b/shared/featureconfig/config.go
@@ -12,7 +12,8 @@ The process for implementing new features using this package is as follows:
        cfg := &featureconfig.Flags{
                VerifyAttestationSigs: true,
        }
-       featureconfig.Init(cfg)
+       resetCfg := featureconfig.InitWithReset(cfg)
+       defer resetCfg()
        6. Add the string for the flags that should be running within E2E to E2EValidatorFlags
        and E2EBeaconChainFlags.
 */
diff --git a/tools/extractor/main.go b/tools/extractor/main.go
index 6d9f05a9f..b2eb8cafc 100644
--- a/tools/extractor/main.go
+++ b/tools/extractor/main.go
@@ -19,13 +19,9 @@ var (
        state = flag.Uint("state", 0, "Extract state at this slot.")
 )

-func init() {
-       fc := featureconfig.Get()
-       fc.WriteSSZStateTransitions = true
-       featureconfig.Init(fc)
-}
-
 func main() {
+       resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{WriteSSZStateTransitions: true})
+       defer resetCfg()
        flag.Parse()
        fmt.Println("Starting process...")
        d, err := db.NewDB(*datadir, cache.NewStateSummaryCache())

@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #5654 into master will increase coverage by 21.16%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #5654       +/-   ##
===========================================
+ Coverage   32.26%   53.42%   +21.16%     
===========================================
  Files         180      309      +129     
  Lines       16052    25514     +9462     
===========================================
+ Hits         5179    13631     +8452     
+ Misses      10165     9858      -307     
- Partials      708     2025     +1317     

@prestonvanloon
Copy link
Member Author

@farazdagi the static analysis check was only for test code, so that is why you don't see it complaining about those non-test code files.

I fixed the issue with false positives. It should be all good to go now.

Copy link
Contributor

@farazdagi farazdagi left a comment

Choose a reason for hiding this comment

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

lgtm

@prylabs-bulldozer prylabs-bulldozer bot merged commit e30349d into master Apr 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the sa-fc-init branch April 28, 2020 01:13
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.

2 participants