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

RSDK-9755 - CLI lint enforce createCommandWithT usage #4803

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@
/cli/app.go @viamrobotics/sdk-netcode
/cli/CONTRIBUTING.md @viamrobotics/sdk-netcode
/cli/STYLEGUIDE.md @viamrobotics/sdk-netcode
/cli/internal/ @viamrobotics/sdk-netcode

6 changes: 6 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ jobs:
sudo apt-get update && sudo apt-get install -y python3-venv
sudo --preserve-env=MONGODB_TEST_OUTPUT_URI,GITHUB_SHA,GITHUB_RUN_ID,GITHUB_RUN_NUMBER,GITHUB_RUN_ATTEMPT,GITHUB_X_PR_BASE_SHA,GITHUB_X_PR_BASE_REF,GITHUB_X_HEAD_REF,GITHUB_X_HEAD_SHA,GITHUB_REPOSITORY -Hu testbot bash -lc 'make test-go'

- name: Run CLI linter tests
Copy link
Member

Choose a reason for hiding this comment

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

Maybe lint before running unit tests?

Unit tests take forever, and linting should be fast

run: |
cd cli/internal/cli_lint
go build .
./cli_lint ../../

- name: Upload test.json
if: always()
uses: actions/upload-artifact@v4
Expand Down
1 change: 1 addition & 0 deletions cli/internal/cli_lint/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cli_lint
101 changes: 101 additions & 0 deletions cli/internal/cli_lint/cli_linter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Package main is the CLI-specific linter itself.
package main

import (
"go/ast"
"go/types"
"strings"

"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/singlechecker"
)

var enforceCreateCommandWithT = &analysis.Analyzer{
Name: "createcommandwitht",
Doc: "Use CreateCommandWithT",
Run: enforceCreateCommandWithTRun,
}

func enforceCreateCommandWithTRun(pass *analysis.Pass) (interface{}, error) {
var commandType types.Type

for _, pkg := range pass.Pkg.Imports() {
// we want to differentiate the upstream `cli` package and our own `cli` package
if pkg.Name() == "cli" && !strings.Contains(pkg.Path(), "viam") {
commandType = pkg.Scope().Lookup("Command").Type()
commandType = types.NewPointer(commandType) // actual `Commands` in the app are pointers
}
}
if commandType == nil { // no CLI imports so we can skip
return nil, nil
}

for _, file := range pass.Files {
ast.Inspect(file, func(node ast.Node) bool {
composite, isComposite := node.(*ast.CompositeLit)

// we aren't looking at a struct, so no need to evaluate further
if !isComposite {
return true
}
compositeType := pass.TypesInfo.TypeOf(composite)

// we aren't looking at a CLI command, so no need to evaluate further
if compositeType.String() != commandType.String() {
return true
}

for _, elt := range composite.Elts {
keyValue, isKeyValue := elt.(*ast.KeyValueExpr)

// we aren't looking at assignment of a struct param, so no need to evaluate further
if !isKeyValue {
return true
}
key := keyValue.Key.(*ast.Ident)

// "Action", "Before", and "After" are the three types of CLI actions for which
// `createCommandWithT` was designed
if key.Name == "Action" || key.Name == "Before" ||
key.Name == "After" {
callExpr, isCallExpr := keyValue.Value.(*ast.CallExpr)

// `Action` was assigned a literal value, rather than a function call.
if !isCallExpr {
pass.Report(analysis.Diagnostic{
Pos: keyValue.Pos(),
End: keyValue.End(),
Message: "must use createCommandWithT when constructing a CLI action",
})
return true
}

callExprFunc, isCallExprFunc := callExpr.Fun.(*ast.IndexExpr)
if !isCallExprFunc { // this should never happen, but just for explicitness
return true
}
funcIdent, isFuncIdent := callExprFunc.X.(*ast.Ident)
if !isFuncIdent { // as above, this should never happen
return true
}

if funcIdent.Name != "createCommandWithT" {
// some other func was used to generate the action
pass.Report(analysis.Diagnostic{
Pos: funcIdent.Pos(),
End: funcIdent.End(),
Message: "must use createCommandWithT when constructing a CLI action",
})
}
}
}
return true
})
}

return nil, nil
}

func main() {
singlechecker.Main(enforceCreateCommandWithT)
}
Loading