diff --git a/.custom-gcl.yml b/.custom-gcl.yml index c34d5e5c15..dbeb9c0118 100644 --- a/.custom-gcl.yml +++ b/.custom-gcl.yml @@ -5,7 +5,7 @@ version: v2.11.3 plugins: - module: "go.uber.org/nilaway" import: "go.uber.org/nilaway/cmd/gclplugin" - version: v0.0.0-20260528182042-490362de4fb6 # fixed version for reproducible builds - latest as of 2026-06-01 + version: v0.0.0-20260803001828-dc48a6814e08 # fixed version for reproducible builds - latest as of 2026-08-03 - module: "github.com/fleetdm/fleet/v4/tools/ci/setboolcheck" import: "github.com/fleetdm/fleet/v4/tools/ci/setboolcheck/cmd/gclplugin" path: "tools/ci/setboolcheck" diff --git a/Makefile b/Makefile index d12a460d4a..a991d9683d 100644 --- a/Makefile +++ b/Makefile @@ -254,7 +254,7 @@ lint-js: .help-short--lint-go: @echo "Run the Go linters" -lint-go: check-no-testing-in-prod +lint-go: check-no-testing-in-prod check-nilaway-func-size golangci-lint run --allow-serial-runners --timeout 15m ifndef SKIP_INCREMENTAL $(MAKE) lint-go-incremental @@ -265,6 +265,13 @@ endif check-no-testing-in-prod: go run ./tools/check-no-testing-in-prod +.help-short--check-nilaway-func-size: + @echo "Fail if any function has too many CFG blocks for nilaway to analyze." +# Deliberately not part of the incremental lint: nilaway reports this failure at a synthetic $GOROOT +# position that --new-from-rev always filters out, so the gate has to run over the whole repo. +check-nilaway-func-size: + go run ./tools/check-nilaway-func-size ./... + .help-short--lint-go-incremental: @echo "Run the incremental Go linters" lint-go-incremental: custom-gcl diff --git a/changes/50404-nilaway-function-size-lint b/changes/50404-nilaway-function-size-lint new file mode 100644 index 0000000000..f2f55a6f9b --- /dev/null +++ b/changes/50404-nilaway-function-size-lint @@ -0,0 +1 @@ +- Split `ListHostSoftware` and `ModifyAppConfig` into smaller helpers so that those packages can be checked by nilaway linter. diff --git a/server/datastore/mysql/software.go b/server/datastore/mysql/software.go index ae8df95ace..b9b9bd04e9 100644 --- a/server/datastore/mysql/software.go +++ b/server/datastore/mysql/software.go @@ -5808,6 +5808,83 @@ func filterOutOfScopeFailedHostSoftwareInstalls( } } +// filterSelfServiceOutOfScopeHostSoftware drops self-service titles that a scope filter excluded from the inventory maps. Self +// service impacts inventory: when a software title is excluded because of a filter, it should be excluded from the inventory as +// well, because we cannot "reinstall" it on the self service page. Only titles flagged self-service are considered; the rest are +// left alone. Callers apply this only when opts.SelfServiceOnly is set. Maps are mutated in place. +func filterSelfServiceOutOfScopeHostSoftware( + bySoftwareTitleID map[uint]*hostSoftware, + byVPPAdamID map[string]*hostSoftware, + byInHouseID map[uint]*hostSoftware, + filteredBySoftwareTitleID map[uint]*hostSoftware, + filteredByVPPAdamID map[string]*hostSoftware, + filteredByInHouseID map[uint]*hostSoftware, +) { + for _, software := range bySoftwareTitleID { + if software.PackageSelfService != nil && *software.PackageSelfService { + if filteredBySoftwareTitleID[software.ID] == nil { + // remove the software title from bySoftwareTitleID + delete(bySoftwareTitleID, software.ID) + } + } + } + for vppAppAdamID, software := range byVPPAdamID { + if software.VPPAppSelfService != nil && *software.VPPAppSelfService { + if filteredByVPPAdamID[vppAppAdamID] == nil { + // remove the software title from byVPPAdamID + delete(byVPPAdamID, vppAppAdamID) + } + } + } + for inHouseID, software := range byInHouseID { + if software.InHouseAppSelfService != nil && *software.InHouseAppSelfService { + if filteredByInHouseID[inHouseID] == nil { + // remove the software title from byInHouseID + delete(byInHouseID, inHouseID) + } + } + } +} + +// filterHostSoftwareToMacOSApplications drops every title the host isn't reporting at the top level of the macOS /Applications +// folder. Callers apply this only for macOS hosts with opts.MacOSApplicationsOnly set. Pruning the in-memory maps (rather than +// the SQL) keeps the count and main queries consistent and applies uniformly across software, VPP, and in-house apps. Maps are +// mutated in place. +func (ds *Datastore) filterHostSoftwareToMacOSApplications( + ctx context.Context, + hostID uint, + bySoftwareTitleID map[uint]*hostSoftware, + bySoftwareID map[uint]*hostSoftware, + byVPPAdamID map[string]*hostSoftware, + byInHouseID map[uint]*hostSoftware, +) error { + qualifyingTitleIDs, err := ds.macOSTopLevelApplicationTitleIDs(ctx, hostID) + if err != nil { + return ctxerr.Wrap(ctx, err, "filter macos applications") + } + for titleID := range bySoftwareTitleID { + if _, ok := qualifyingTitleIDs[titleID]; !ok { + delete(bySoftwareTitleID, titleID) + } + } + for softwareID, s := range bySoftwareID { + if _, ok := qualifyingTitleIDs[s.ID]; !ok { + delete(bySoftwareID, softwareID) + } + } + for adamID, s := range byVPPAdamID { + if _, ok := qualifyingTitleIDs[s.ID]; !ok { + delete(byVPPAdamID, adamID) + } + } + for inHouseID, s := range byInHouseID { + if _, ok := qualifyingTitleIDs[s.ID]; !ok { + delete(byInHouseID, inHouseID) + } + } + return nil +} + // mergeInstallDataByInstaller records the most recent install for a title's specific installer, // keyed by (title id, installer id). Keeping install data per installer (rather than collapsing to // one row per title) is what lets ListHostSoftware later surface the install belonging to the @@ -6900,30 +6977,8 @@ func (ds *Datastore) ListHostSoftware(ctx context.Context, host *fleet.Host, opt // self service impacts inventory, when a software title is excluded because of a filter, // it should be excluded from the inventory as well, because we cannot "reinstall" it on the self service page if opts.SelfServiceOnly { - for _, software := range bySoftwareTitleID { - if software.PackageSelfService != nil && *software.PackageSelfService { - if filteredBySoftwareTitleID[software.ID] == nil { - // remove the software title from bySoftwareTitleID - delete(bySoftwareTitleID, software.ID) - } - } - } - for vppAppAdamID, software := range byVPPAdamID { - if software.VPPAppSelfService != nil && *software.VPPAppSelfService { - if filteredByVPPAdamID[vppAppAdamID] == nil { - // remove the software title from byVPPAdamID - delete(byVPPAdamID, vppAppAdamID) - } - } - } - for inHouseID, software := range byInHouseID { - if software.InHouseAppSelfService != nil && *software.InHouseAppSelfService { - if filteredByInHouseID[inHouseID] == nil { - // remove the software title from byInHouseID - delete(byInHouseID, inHouseID) - } - } - } + filterSelfServiceOutOfScopeHostSoftware(bySoftwareTitleID, byVPPAdamID, byInHouseID, + filteredBySoftwareTitleID, filteredByVPPAdamID, filteredByInHouseID) } // since these host installed vpp apps/in-house apps are already added in bySoftwareTitleID, @@ -6946,29 +7001,9 @@ func (ds *Datastore) ListHostSoftware(ctx context.Context, host *fleet.Host, opt // than the SQL) keeps the count and main queries consistent and applies // uniformly across software, VPP, and in-house apps. if opts.MacOSApplicationsOnly && fleet.IsMacOSPlatform(host.Platform) { - qualifyingTitleIDs, err := ds.macOSTopLevelApplicationTitleIDs(ctx, host.ID) - if err != nil { - return nil, nil, ctxerr.Wrap(ctx, err, "filter macos applications") - } - for titleID := range bySoftwareTitleID { - if _, ok := qualifyingTitleIDs[titleID]; !ok { - delete(bySoftwareTitleID, titleID) - } - } - for softwareID, s := range bySoftwareID { - if _, ok := qualifyingTitleIDs[s.ID]; !ok { - delete(bySoftwareID, softwareID) - } - } - for adamID, s := range byVPPAdamID { - if _, ok := qualifyingTitleIDs[s.ID]; !ok { - delete(byVPPAdamID, adamID) - } - } - for inHouseID, s := range byInHouseID { - if _, ok := qualifyingTitleIDs[s.ID]; !ok { - delete(byInHouseID, inHouseID) - } + if err := ds.filterHostSoftwareToMacOSApplications(ctx, host.ID, bySoftwareTitleID, bySoftwareID, + byVPPAdamID, byInHouseID); err != nil { + return nil, nil, err } } diff --git a/server/service/appconfig.go b/server/service/appconfig.go index 440def49ec..d0f617fb07 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -1412,11 +1412,31 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle } obfuscatedAppConfig.Obfuscate() - // if the agent options changed, create the corresponding activity newAgentOptions := "" if obfuscatedAppConfig.AgentOptions != nil { newAgentOptions = string(*obfuscatedAppConfig.AgentOptions) } + + if err := svc.processSavedAppConfigChanges(ctx, oldAppConfig, appConfig, lic, oldAgentOptions, newAgentOptions, + conditionalAccessNoTeamUpdated); err != nil { + return nil, err + } + + return obfuscatedAppConfig, nil +} + +// processSavedAppConfigChanges runs the side effects of a completed app config change: it creates the activities for the settings +// that were modified and reconciles the downstream state that depends on them (OS updates, disk encryption, DEP profiles, host +// name templates, Windows MDM profile cleanup). It runs after SaveAppConfig has committed, so returning an error here leaves the +// new configuration persisted. +func (svc *Service) processSavedAppConfigChanges( + ctx context.Context, + oldAppConfig, appConfig *fleet.AppConfig, + lic *fleet.LicenseInfo, + oldAgentOptions, newAgentOptions string, + conditionalAccessNoTeamUpdated bool, +) error { + // if the agent options changed, create the corresponding activity if oldAgentOptions != newAgentOptions { if err := svc.NewActivity( ctx, @@ -1425,7 +1445,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle Global: true, }, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for app config agent options modification") + return ctxerr.Wrap(ctx, err, "create activity for app config agent options modification") } } @@ -1436,24 +1456,24 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle oldAppConfig.MDM.MacOSUpdates, appConfig.MDM.MacOSUpdates, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "process macOS OS updates config change") + return ctxerr.Wrap(ctx, err, "process macOS OS updates config change") } if err := svc.processAppleOSUpdateSettings(ctx, lic, fleet.IOS, oldAppConfig.MDM.IOSUpdates, appConfig.MDM.IOSUpdates, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "process iOS OS updates config change") + return ctxerr.Wrap(ctx, err, "process iOS OS updates config change") } if err := svc.processAppleOSUpdateSettings(ctx, lic, fleet.IPadOS, oldAppConfig.MDM.IPadOSUpdates, appConfig.MDM.IPadOSUpdates, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "process iPadOS OS updates config change") + return ctxerr.Wrap(ctx, err, "process iPadOS OS updates config change") } if appConfig.YaraRules != nil { if err := svc.ds.ApplyYaraRules(ctx, appConfig.YaraRules); err != nil { - return nil, ctxerr.Wrap(ctx, err, "save yara rules for app config") + return ctxerr.Wrap(ctx, err, "save yara rules for app config") } } @@ -1470,10 +1490,10 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle if deadline != nil { if err := svc.EnterpriseOverrides.MDMWindowsEnableOSUpdates(ctx, nil, appConfig.MDM.WindowsUpdates); err != nil { - return nil, ctxerr.Wrap(ctx, err, "enable no-team windows OS updates") + return ctxerr.Wrap(ctx, err, "enable no-team windows OS updates") } } else if err := svc.EnterpriseOverrides.MDMWindowsDisableOSUpdates(ctx, nil); err != nil { - return nil, ctxerr.Wrap(ctx, err, "disable no-team windows OS updates") + return ctxerr.Wrap(ctx, err, "disable no-team windows OS updates") } if err := svc.NewActivity( @@ -1484,7 +1504,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle GracePeriodDays: grace, }, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for app config macos min version modification") + return ctxerr.Wrap(ctx, err, "create activity for app config windows updates modification") } } @@ -1494,16 +1514,16 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle if appConfig.MDM.EnableDiskEncryption.Value { act = fleet.ActivityTypeEnabledMacosDiskEncryption{} if err := svc.EnterpriseOverrides.MDMAppleEnableFileVaultAndEscrow(ctx, nil); err != nil { - return nil, ctxerr.Wrap(ctx, err, "enable no-team filevault and escrow") + return ctxerr.Wrap(ctx, err, "enable no-team filevault and escrow") } } else { act = fleet.ActivityTypeDisabledMacosDiskEncryption{} if err := svc.EnterpriseOverrides.MDMAppleDisableFileVaultAndEscrow(ctx, nil); err != nil { - return nil, ctxerr.Wrap(ctx, err, "disable no-team filevault and escrow") + return ctxerr.Wrap(ctx, err, "disable no-team filevault and escrow") } } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for app config macos disk encryption") + return ctxerr.Wrap(ctx, err, "create activity for app config macos disk encryption") } } } @@ -1516,7 +1536,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle // inert because the enforcement cron skips an empty template. if lic.IsPremium() && oldAppConfig.MDM.HostNameTemplate.Value != appConfig.MDM.HostNameTemplate.Value { if err := svc.EnterpriseOverrides.ApplyHostNameTemplateChange(ctx, nil, appConfig.MDM.HostNameTemplate.Value); err != nil { - return nil, ctxerr.Wrap(ctx, err, "reconcile no-team host name template") + return ctxerr.Wrap(ctx, err, "reconcile no-team host name template") } } @@ -1530,7 +1550,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle act = fleet.ActivityTypeDisabledRecoveryLockPasswords{} } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for app config recovery lock password") + return ctxerr.Wrap(ctx, err, "create activity for app config recovery lock password") } } } @@ -1544,7 +1564,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle act = fleet.ActivityTypeDisabledMacosSetupEndUserAuth{} } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for macos enable end user auth change") + return ctxerr.Wrap(ctx, err, "create activity for macos enable end user auth change") } } @@ -1556,7 +1576,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle act = fleet.ActivityTypeDisabledManagedLocalAccount{Platform: "darwin"} } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for macos enable managed local account change") + return ctxerr.Wrap(ctx, err, "create activity for macos enable managed local account change") } } @@ -1568,7 +1588,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle act = fleet.ActivityTypeDisabledManagedLocalAccount{Platform: "windows"} } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for windows enable managed local account change") + return ctxerr.Wrap(ctx, err, "create activity for windows enable managed local account change") } } @@ -1580,25 +1600,25 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle if appleMDMUrlChanged && appConfig.MDM.AppleServerURL != "" { parsedURL, err := url.Parse(appConfig.MDM.AppleServerURL) if err != nil { - return nil, fleet.NewInvalidArgumentError("mdmAppleServerURL", "must be a valid URL") + return fleet.NewInvalidArgumentError("mdmAppleServerURL", "must be a valid URL") } scheme := strings.ToLower(parsedURL.Scheme) if scheme == "" { - return nil, fleet.NewInvalidArgumentError("mdmAppleServerURL", "must include a URL scheme (e.g. https://)") + return fleet.NewInvalidArgumentError("mdmAppleServerURL", "must include a URL scheme (e.g. https://)") } if scheme != "http" && scheme != "https" { - return nil, fleet.NewInvalidArgumentError("mdmAppleServerURL", "URL scheme must be http or https") + return fleet.NewInvalidArgumentError("mdmAppleServerURL", "URL scheme must be http or https") } if parsedURL.Hostname() == "" { - return nil, fleet.NewInvalidArgumentError("mdmAppleServerURL", "must include a host") + return fleet.NewInvalidArgumentError("mdmAppleServerURL", "must include a host") } } if (mdmEnableEndUserAuthChanged || mdmSSOSettingsChanged || serverURLChanged || appleMDMUrlChanged) && lic.IsPremium() { if err := svc.EnterpriseOverrides.MDMAppleSyncDEPProfiles(ctx); err != nil { - return nil, ctxerr.Wrap(ctx, err, "sync DEP profiles") + return ctxerr.Wrap(ctx, err, "sync DEP profiles") } } @@ -1612,11 +1632,11 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle // Clean up all pending Windows MDM profile rows since hosts can no longer receive MDM commands. if err := svc.ds.CleanupAllHostMDMProfilesForPlatform(ctx, "windows"); err != nil { - return nil, ctxerr.Wrap(ctx, err, "cleaning up Windows host MDM profiles") + return ctxerr.Wrap(ctx, err, "cleaning up Windows host MDM profiles") } } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrapf(ctx, err, "create activity %s", act.ActivityName()) + return ctxerr.Wrapf(ctx, err, "create activity %s", act.ActivityName()) } } @@ -1628,7 +1648,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle act = fleet.ActivityTypeDisabledWindowsMDMMigration{} } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { - return nil, ctxerr.Wrapf(ctx, err, "create activity %s", act.ActivityName()) + return ctxerr.Wrapf(ctx, err, "create activity %s", act.ActivityName()) } } @@ -1643,7 +1663,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle TeamName: "", }, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for enabling conditional access") + return ctxerr.Wrap(ctx, err, "create activity for enabling conditional access") } } else { if err := svc.NewActivity( @@ -1654,7 +1674,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle TeamName: "", }, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for disabling conditional access") + return ctxerr.Wrap(ctx, err, "create activity for disabling conditional access") } } } @@ -1684,7 +1704,7 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle authz.UserFromContext(ctx), fleet.ActivityTypeAddedConditionalAccessOkta{}, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for adding/editing Okta conditional access") + return ctxerr.Wrap(ctx, err, "create activity for adding/editing Okta conditional access") } } else if oldOktaConfigured && !newOktaConfigured { // Okta configuration was deleted @@ -1693,13 +1713,13 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle authz.UserFromContext(ctx), fleet.ActivityTypeDeletedConditionalAccessOkta{}, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for deleting Okta conditional access") + return ctxerr.Wrap(ctx, err, "create activity for deleting Okta conditional access") } } if oktaBypassChanged { if err := svc.ds.ConditionalAccessClearBypasses(ctx); err != nil { - return nil, ctxerr.Wrap(ctx, err, "clearing existing conditional access bypasses") + return ctxerr.Wrap(ctx, err, "clearing existing conditional access bypasses") } if err := svc.NewActivity( @@ -1709,11 +1729,11 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle BypassDisabled: appConfig.ConditionalAccess.BypassDisabled.Value, }, ); err != nil { - return nil, ctxerr.Wrap(ctx, err, "create activity for updating conditional access bypass") + return ctxerr.Wrap(ctx, err, "create activity for updating conditional access bypass") } } - return obfuscatedAppConfig, nil + return nil } func validateFleetDesktopSettings(newAppConfig fleet.AppConfig, lic *fleet.LicenseInfo) *fleet.InvalidArgumentError { diff --git a/tools/check-nilaway-func-size/main.go b/tools/check-nilaway-func-size/main.go new file mode 100644 index 0000000000..aae7590ad2 --- /dev/null +++ b/tools/check-nilaway-func-size/main.go @@ -0,0 +1,80 @@ +// Command check-nilaway-func-size fails the build if any Go function is too large for nilaway to analyze. +// +// Background: nilaway skips any function whose control-flow graph exceeds a fixed block count (_maxFuncSizeInCFGBlocks, currently +// 500, in go.uber.org/nilaway/assertion/function/analyzer.go). A skipped function is not merely unanalyzed on its own. nilaway's +// accumulation analyzer bails out for the whole package as soon as the assertion analyzer reports any error, so a single +// oversized function costs every other function in that package its nil-panic analysis, and costs dependent packages the +// inference facts that package would have exported. +// +// This tool consumes the same golang.org/x/tools/go/analysis/passes/ctrlflow CFGs that nilaway consumes, so its block counts are +// identical to nilaway's by construction rather than an approximation of them. +// +// Usage: +// +// go run ./tools/check-nilaway-func-size ./... +// +// Wired into make lint-go (see Makefile). +package main + +import ( + "fmt" + "go/ast" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/ctrlflow" + "golang.org/x/tools/go/analysis/singlechecker" +) + +// defaultMaxCFGBlocks mirrors nilaway's own limit. Nothing passes -max in practice; it exists so the limit can be lowered to buy +// headroom (flagging functions before they actually lose analysis) and so the tests can use a small fixture. Raising it above +// nilaway's limit accomplishes nothing, since nilaway stops analyzing past that point regardless. +const defaultMaxCFGBlocks = 500 + +var maxCFGBlocks int + +var analyzer = &analysis.Analyzer{ + Name: "nilawayfuncsize", + Doc: "reports functions with too many CFG blocks for nilaway to analyze", + Requires: []*analysis.Analyzer{ctrlflow.Analyzer}, + Run: run, +} + +func init() { + analyzer.Flags.IntVar(&maxCFGBlocks, "max", defaultMaxCFGBlocks, + fmt.Sprintf("maximum CFG blocks allowed per function (nilaway's own limit is %d)", defaultMaxCFGBlocks)) +} + +func run(pass *analysis.Pass) (any, error) { + cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs) + + for _, file := range pass.Files { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + // ctrlflow.CFGs.FuncDecl looks the declaration up by its type object and would panic on a name that has none, as in a blank "func + // _()" declaration. nilaway never sizes those either, so skip them. + if pass.TypesInfo.Defs[fn.Name] == nil { + continue + } + // Only function declarations are gated. nilaway size-checks function literals solely when its experimental-anonymous-function + // flag is set, and .golangci-incremental.yml does not set it, so an oversized closure costs us nothing today. + graph := cfgs.FuncDecl(fn) + if graph == nil { + // ctrlflow builds no CFG for functions in its hard-coded known-intrinsic list (log.Fatal and friends). nilaway skips those as well. + continue + } + if len(graph.Blocks) > maxCFGBlocks { + pass.Reportf(fn.Pos(), + "%s has %d CFG blocks, over the limit of %d. nilaway skips functions this large, and drops "+ + "nil-panic analysis for every other function in the package with it. Split it into helpers. ", + fn.Name.Name, len(graph.Blocks), maxCFGBlocks) + } + } + } + + return nil, nil +} + +func main() { singlechecker.Main(analyzer) } diff --git a/tools/check-nilaway-func-size/main_test.go b/tools/check-nilaway-func-size/main_test.go new file mode 100644 index 0000000000..9059dd54be --- /dev/null +++ b/tools/check-nilaway-func-size/main_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "strconv" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +// TestAnalyzer runs the analyzer against testdata with a deliberately tiny limit, so the fixture does not need a genuinely +// 500-block function to exercise the reporting path. +func TestAnalyzer(t *testing.T) { + const testMax = 5 + + if err := analyzer.Flags.Set("max", strconv.Itoa(testMax)); err != nil { + t.Fatalf("set max flag: %s", err) + } + t.Cleanup(func() { + if err := analyzer.Flags.Set("max", strconv.Itoa(defaultMaxCFGBlocks)); err != nil { + t.Fatalf("restore max flag: %s", err) + } + }) + + analysistest.Run(t, analysistest.TestData(), analyzer, "example") +} + +// TestDefaultMatchesNilaway guards the constant against drifting above nilaway's own limit, where the gate would stop meaning +// anything. +func TestDefaultMatchesNilaway(t *testing.T) { + const nilawayMaxFuncSizeInCFGBlocks = 500 + + if defaultMaxCFGBlocks > nilawayMaxFuncSizeInCFGBlocks { + t.Errorf("defaultMaxCFGBlocks = %d, must not exceed nilaway's limit of %d", + defaultMaxCFGBlocks, nilawayMaxFuncSizeInCFGBlocks) + } +} diff --git a/tools/check-nilaway-func-size/testdata/src/example/example.go b/tools/check-nilaway-func-size/testdata/src/example/example.go new file mode 100644 index 0000000000..fcff634f85 --- /dev/null +++ b/tools/check-nilaway-func-size/testdata/src/example/example.go @@ -0,0 +1,50 @@ +package example + +// tooBig branches enough times to exceed the threshold the test sets. +func tooBig(a, b, c, d int) int { // want "tooBig has \\d+ CFG blocks, over the limit of 5" + if a > 0 { + a++ + } + if b > 0 { + b++ + } + if c > 0 { + c++ + } + if d > 0 { + d++ + } + return a + b + c + d +} + +// smallEnough stays under the threshold and must not be reported. +func smallEnough(a int) int { + if a > 0 { + return a + } + return -a +} + +// closuresAreNotGated keeps its own CFG small while nesting a heavily branching function literal. +// nilaway only size-checks literals under its experimental-anonymous-function flag, which Fleet does +// not enable, so this must not be reported. +func closuresAreNotGated() func(int) int { + return func(n int) int { + if n > 1 { + n++ + } + if n > 2 { + n++ + } + if n > 3 { + n++ + } + if n > 4 { + n++ + } + if n > 5 { + n++ + } + return n + } +}