Fixed nilaway issues (#50405)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**Related issue:** Resolves #50404 

- Refactored `ListHostSoftware` and `ModifyAppConfig` functions beeing
too big for nilaway
- Added a hard check to make sure all our funcitons/packages are being
analyzed by nilaway

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files)
for more information.

## Testing

- [x] Added/updated automated tests

- [x] QA'd all new/changed functionality manually


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Improvements**
* Improved software inventory filtering for self-service and macOS
applications, producing more accurate results.
* Improved application configuration updates so saved settings and
related system changes are processed more reliably.
* **Quality**
* Added automated checks to identify overly complex functions and help
maintain code quality.
* Updated static analysis tooling and expanded validation coverage with
new tests.
* **Documentation**
* Added a changelog entry describing the latest reliability and
maintainability improvements.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Victor Lyuboslavsky
2026-08-04 15:41:18 -05:00
committed by GitHub
parent c49d3d8191
commit bd601fff84
8 changed files with 310 additions and 81 deletions
+1 -1
View File
@@ -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"
+8 -1
View File
@@ -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
+1
View File
@@ -0,0 +1 @@
- Split `ListHostSoftware` and `ModifyAppConfig` into smaller helpers so that those packages can be checked by nilaway linter.
+82 -47
View File
@@ -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
}
}
+52 -32
View File
@@ -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 {
+80
View File
@@ -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) }
@@ -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)
}
}
@@ -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
}
}