New rule: unused-parameter (#28)

* Adds rule superfluous-else (an extension of indent-error-flow)

* Fix superfluous-else rule struct namming.

* Adds superfuous-else rule to the rules table

* Adds confusing-naming rule

* adds multifile test

* [WIP] fix multiple file test

* draft solution for detecting confusing-names through multiple files

* [WIP] confusing-name multiple files

* clean-up

* draft working version

* cleaner version + more informative messages

* adds check on struct field names

* fix config.go

* clean master

* new ADS rule: newerr

* ADS-print working version

* ads-print final version

* ads-lost-err working version

* confusing-namming: fix tests

* unused-parameter: working version

* WIP adds scopes - still imprecise ( eg a:=a is not detected as use)

* w/scopes and more precise

* adds test on structs

* adds test w/ var shadowing

* more precise handling of for/switch statements

* fix check of +=, -=, *= and the like. Adds better support for slices and switchs

README.md

@@ -215,6 +215,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | `modifies-param`      |  n/a   | Warns on assignments to function parameters                      |    no    |  no   |
 | `confusing-results`   |  n/a   | Suggests to name potentially confusing function results          |    no    |  no   |
 | `deep-exit`           |  n/a   | Looks for program exits in funcs other than `main()` or `init()` |    no    |  no   |
+| `unused-parameter`    |  n/a   | Suggests to rename or remove unused function parameters          |    no    |  no   |
 
 ## Available Formatters
 

config.go

@@ -54,6 +54,7 @@ var allRules = append([]lint.Rule{
 	&rule.ModifiesParamRule{},
 	&rule.ConfusingResultsRule{},
 	&rule.DeepExitRule{},
+	&rule.UnusedParamRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{

fixtures/unused-param.go

@@ -0,0 +1,169 @@
+package fixtures
+
+import (
+	"fmt"
+	"go/ast"
+	"io/ioutil"
+	"os"
+	"runtime"
+	"testing"
+	"time"
+
+	"github.com/mgechev/revive/lint"
+)
+
+func f0(param int) {
+	param := param
+}
+
+func f1(param int) { // MATCH /parameter 'param' seems to be unused, consider removing or renaming it as _/
+	if param := fn(); predicate(param) {
+		// do stuff
+	}
+}
+
+func f2(param int) { // MATCH /parameter 'param' seems to be unused, consider removing or renaming it as _/
+	switch param := fn(); param {
+	default:
+
+	}
+}
+
+func f3(param myStruct) {
+	a := param.field
+}
+
+func f4(param myStruct, c int) { // MATCH /parameter 'c' seems to be unused, consider removing or renaming it as _/
+	param.field = "aString"
+	param.c = "sss"
+}
+
+func f5(a int, _ float) { // MATCH /parameter 'a' seems to be unused, consider removing or renaming it as _/
+	fmt.Printf("Hello, Golang\n")
+	{
+		if true {
+			a := 2
+			b := a
+		}
+	}
+}
+
+func f6(unused string) { // MATCH /parameter 'unused' seems to be unused, consider removing or renaming it as _/
+	switch unused := runtime.GOOS; unused {
+	case "darwin":
+		fmt.Println("OS X.")
+	case "linux":
+		fmt.Println("Linux.")
+	default:
+		fmt.Printf("%s.", unused)
+	}
+	for unused := 0; unused < 10; unused++ {
+		sum += unused
+	}
+	{
+		unused := 1
+	}
+}
+
+func f6bis(unused string) {
+	switch unused := runtime.GOOS; unused {
+	case "darwin":
+		fmt.Println("OS X.")
+	case "linux":
+		fmt.Println("Linux.")
+	default:
+		fmt.Printf("%s.", unused)
+	}
+	for unused := 0; unused < 10; unused++ {
+		sum += unused
+	}
+	{
+		unused := 1
+	}
+
+	fmt.Print(unused)
+}
+
+func f7(pl int) {
+	for i := 0; pl < i; i-- {
+
+	}
+}
+
+func getCompareFailCause(n *node, which int, prevValue string, prevIndex uint64) string {
+	switch which {
+	case CompareIndexNotMatch:
+		return fmt.Sprintf("[%v != %v]", prevIndex, n.ModifiedIndex)
+	case CompareValueNotMatch:
+		return fmt.Sprintf("[%v != %v]", prevValue, n.Value)
+	default:
+		return fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex)
+	}
+}
+
+func assertSuccess(t *testing.T, baseDir string, fi os.FileInfo, src []byte, rules []lint.Rule, config map[string]lint.RuleConfig) error { // MATCH /parameter 'src' seems to be unused, consider removing or renaming it as _/
+	l := lint.New(func(file string) ([]byte, error) {
+		return ioutil.ReadFile(baseDir + file)
+	})
+
+	ps, err := l.Lint([][]string{[]string{fi.Name()}}, rules, lint.Config{
+		Rules: config,
+	})
+	if err != nil {
+		return err
+	}
+
+	failures := ""
+	for p := range ps {
+		failures += p.Failure
+	}
+	if failures != "" {
+		t.Errorf("Expected the rule to pass but got the following failures: %s", failures)
+	}
+	return nil
+}
+
+func (w lintCyclomatic) Visit(n ast.Node) ast.Visitor { // MATCH /parameter 'n' seems to be unused, consider removing or renaming it as _/
+	f := w.file
+	for _, decl := range f.AST.Decls {
+		if fn, ok := decl.(*ast.FuncDecl); ok {
+			c := complexity(fn)
+			if c > w.complexity {
+				w.onFailure(lint.Failure{
+					Confidence: 1,
+					Category:   "maintenance",
+					Failure:    fmt.Sprintf("function %s has cyclomatic complexity %d", funcName(fn), c),
+					Node:       fn,
+				})
+			}
+		}
+	}
+	return nil
+}
+
+func ext۰time۰Sleep(fr *frame, args []value) value { // MATCH /parameter 'fr' seems to be unused, consider removing or renaming it as _/
+	time.Sleep(time.Duration(args[0].(int64)))
+	return nil
+}
+
+func (c *chanList) remove(id uint32) {
+	id -= c.offset
+}
+
+func (c *chanList) remove1(id uint32) {
+	id *= c.offset
+}
+
+func (c *chanList) remove2(id uint32) {
+	id /= c.offset
+}
+
+func (c *chanList) remove3(id uint32) {
+	id += c.offset
+}
+
+func encodeFixed64Rpc(dAtA []byte, offset int, v uint64, i int) int {
+	dAtA[offset+i] = uint8(v)
+
+	return 8
+}

rule/unused-param.go

@@ -0,0 +1,286 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// UnusedParamRule lints unused params in functions.
+type UnusedParamRule struct{}
+
+// Apply applies the rule to given file.
+func (r *UnusedParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := lintUnusedParamRule{onFailure: onFailure}
+
+	ast.Walk(w, file.AST)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *UnusedParamRule) Name() string {
+	return "unused-parameter"
+}
+
+type lintUnusedParamRule struct {
+	onFailure func(lint.Failure)
+}
+
+func (w lintUnusedParamRule) Visit(node ast.Node) ast.Visitor {
+	switch n := node.(type) {
+	case *ast.FuncDecl:
+		fv := newFuncVisitor(retrieveNamedParams(n.Type.Params.List))
+		if n.Body != nil {
+			ast.Walk(fv, n.Body)
+			checkUnusedParams(w, fv.params, n)
+		}
+		return nil
+	}
+
+	return w
+}
+
+type scope struct {
+	vars map[string]bool
+}
+
+func newScope() scope {
+	return scope{make(map[string]bool, 0)}
+}
+
+func (s *scope) addVars(exps []ast.Expr) {
+	for _, e := range exps {
+		if id, ok := e.(*ast.Ident); ok {
+			s.vars[id.Name] = true
+		}
+	}
+}
+
+type scopeStack struct {
+	stk []scope
+}
+
+func (s *scopeStack) openScope() {
+	s.stk = append(s.stk, newScope())
+}
+
+func (s *scopeStack) closeScope() {
+	if len(s.stk) > 0 {
+		s.stk = s.stk[:len(s.stk)-1]
+	}
+}
+
+func (s *scopeStack) currentScope() scope {
+	if len(s.stk) > 0 {
+		return s.stk[len(s.stk)-1]
+	}
+
+	panic("no current scope")
+}
+
+func newScopeStack() scopeStack {
+	return scopeStack{make([]scope, 0)}
+}
+
+type funcVisitor struct {
+	sStk   scopeStack
+	params map[string]bool
+}
+
+func newFuncVisitor(params map[string]bool) funcVisitor {
+	return funcVisitor{sStk: newScopeStack(), params: params}
+}
+
+func walkStmtList(v ast.Visitor, list []ast.Stmt) {
+	for _, s := range list {
+		ast.Walk(v, s)
+	}
+}
+
+func (v funcVisitor) Visit(node ast.Node) ast.Visitor {
+	varSelector := func(n ast.Node) bool {
+		id, ok := n.(*ast.Ident)
+		return ok && id.Obj != nil && id.Obj.Kind.String() == "var"
+	}
+	switch n := node.(type) {
+	case *ast.BlockStmt:
+		v.sStk.openScope()
+		walkStmtList(v, n.List)
+		v.sStk.closeScope()
+		return nil
+	case *ast.AssignStmt:
+		var uses []ast.Node
+		if isOpAssign(n.Tok) { // Case of id += expr
+			uses = append(uses, pickFromExpList(n.Lhs, varSelector, nil)...)
+		} else { // Case of id[expr] = expr
+			indexSelector := func(n ast.Node) bool {
+				_, ok := n.(*ast.IndexExpr)
+				return ok
+			}
+			f := func(n ast.Node) []ast.Node {
+				ie, ok := n.(*ast.IndexExpr)
+				if !ok { // not possible
+					return nil
+				}
+
+				return pick(ie.Index, varSelector, nil)
+			}
+
+			uses = append(uses, pickFromExpList(n.Lhs, indexSelector, f)...)
+		}
+
+		uses = append(uses, pickFromExpList(n.Rhs, varSelector, nil)...)
+
+		markParamListAsUsed(uses, v)
+		cs := v.sStk.currentScope()
+		cs.addVars(n.Lhs)
+	case *ast.Ident:
+		if n.Obj != nil {
+			if n.Obj.Kind.String() == "var" {
+				markParamAsUsed(n, v)
+			}
+		}
+	case *ast.ForStmt:
+		v.sStk.openScope()
+		if n.Init != nil {
+			ast.Walk(v, n.Init)
+		}
+		uses := pickFromExpList([]ast.Expr{n.Cond}, varSelector, nil)
+		markParamListAsUsed(uses, v)
+		ast.Walk(v, n.Body)
+		v.sStk.closeScope()
+		return nil
+	case *ast.SwitchStmt:
+		v.sStk.openScope()
+		if n.Init != nil {
+			ast.Walk(v, n.Init)
+		}
+		uses := pickFromExpList([]ast.Expr{n.Tag}, varSelector, nil)
+		markParamListAsUsed(uses, v)
+		// Analyze cases (they are not BlockStmt but a list of Stmt)
+		cases := n.Body.List
+		for _, c := range cases {
+			cc, ok := c.(*ast.CaseClause)
+			if !ok {
+				continue
+			}
+			uses := pickFromExpList(cc.List, varSelector, nil)
+			markParamListAsUsed(uses, v)
+			v.sStk.openScope()
+			for _, stmt := range cc.Body {
+				ast.Walk(v, stmt)
+			}
+			v.sStk.closeScope()
+		}
+
+		v.sStk.closeScope()
+		return nil
+	}
+
+	return v
+}
+
+func retrieveNamedParams(pl []*ast.Field) map[string]bool {
+	result := make(map[string]bool, len(pl))
+	for _, p := range pl {
+		for _, n := range p.Names {
+			if n.Name != "_" {
+				result[n.Name] = true
+			}
+		}
+	}
+	return result
+}
+
+func checkUnusedParams(w lintUnusedParamRule, params map[string]bool, n *ast.FuncDecl) {
+	for k, v := range params {
+		if v {
+			w.onFailure(lint.Failure{
+				Confidence: 0.8, // confidence is not 1.0 because of shadow variables
+				Node:       n,
+				Category:   "bad practice",
+				Failure:    fmt.Sprintf("parameter '%s' seems to be unused, consider removing or renaming it as _", k),
+			})
+		}
+	}
+
+}
+
+func markParamListAsUsed(ids []ast.Node, v funcVisitor) {
+	for _, id := range ids {
+		markParamAsUsed(id.(*ast.Ident), v)
+	}
+}
+
+func markParamAsUsed(id *ast.Ident, v funcVisitor) { // TODO: constraint parameters to receive just a list of params and a scope stack
+	for _, s := range v.sStk.stk {
+		if s.vars[id.Name] {
+			return
+		}
+	}
+
+	if v.params[id.Name] {
+		v.params[id.Name] = false
+	}
+}
+
+type picker struct {
+	fselect  func(n ast.Node) bool
+	onSelect func(n ast.Node)
+}
+
+func pick(n ast.Node, fselect func(n ast.Node) bool, f func(n ast.Node) []ast.Node) []ast.Node {
+	var result []ast.Node
+
+	if n == nil {
+		return result
+	}
+
+	if f == nil {
+		f = func(n ast.Node) []ast.Node { return []ast.Node{n} }
+	}
+
+	onSelect := func(n ast.Node) {
+		result = append(result, f(n)...)
+	}
+	p := picker{fselect: fselect, onSelect: onSelect}
+	ast.Walk(p, n)
+	return result
+}
+
+func pickFromExpList(l []ast.Expr, fselect func(n ast.Node) bool, f func(n ast.Node) []ast.Node) []ast.Node {
+	result := make([]ast.Node, 0)
+	for _, e := range l {
+		result = append(result, pick(e, fselect, f)...)
+	}
+	return result
+}
+
+func (p picker) Visit(node ast.Node) ast.Visitor {
+	if p.fselect == nil {
+		return nil
+	}
+
+	if p.fselect(node) {
+		p.onSelect(node)
+	}
+
+	return p
+}
+
+func isOpAssign(aTok token.Token) bool {
+	return aTok == token.ADD_ASSIGN || aTok == token.AND_ASSIGN ||
+		aTok == token.MUL_ASSIGN || aTok == token.OR_ASSIGN ||
+		aTok == token.QUO_ASSIGN || aTok == token.REM_ASSIGN ||
+		aTok == token.SHL_ASSIGN || aTok == token.SHR_ASSIGN ||
+		aTok == token.SUB_ASSIGN || aTok == token.XOR_ASSIGN
+}

test/unused-param_test.go

@@ -0,0 +1,11 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+func TestUnusedParam(t *testing.T) {
+	testRule(t, "unused-param", &rule.UnusedParamRule{})
+}