Merge remote-tracking branch 'upstream/master'

README.md

@@ -212,6 +212,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | `superfluous-else`    |  n/a   | Prevents redundant else statements (extends `indent-error-flow`) |    no    |  no   |
 | `confusing-naming`    |  n/a   | Warns on methods with names that differ only by capitalization   |    no    |  no   |
 | `get-return      `    |  n/a   | Warns on getters that do not yield any result                    |    no    |  no   |
+| `modifies-param`      |  n/a   | Warns on assignments to function parameters                      |    no    |  no   |
 
 ## Available Formatters
 

config.go

@@ -50,6 +50,7 @@ var allRules = append([]lint.Rule{
 	&rule.EmptyBlockRule{},
 	&rule.SuperfluousElseRule{},
 	&rule.GetReturnRule{},
+	&rule.ModifiesParamRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{

fixtures/modifies-param.go

@@ -0,0 +1,26 @@
+package fixtures
+
+import (
+	"go/ast"
+)
+
+func one(a int) {
+ a,b:= 1,2 // MATCH /parameter 'a' seems to be modified/
+ a++ // MATCH /parameter 'a' seems to be modified/
+}
+
+func two(b, c float32) {
+    if c>0.0 {
+        b = 1 // MATCH /parameter 'b' seems to be modified/
+    }
+}
+
+type foo struct {
+    a string
+}
+
+func three(s *foo) {
+    s.a = "foooooo"
+}
+
+

fixtures/superfluous-else.go

@@ -4,6 +4,7 @@
 package pkg
 
 import (
+	"os"
 	"fmt"
 	"log"
 )
@@ -71,3 +72,86 @@ func j(f func() bool) string {
 
 	return "ok"
 }
+
+func fatal1() string {
+		if f() {
+			a := 1
+			log.Fatal("x")
+		} else { // MATCH /if block ends with call to log.Fatal function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func fatal2() string {
+		if f() {
+			a := 1
+			log.Fatalf("x")
+		} else { // MATCH /if block ends with call to log.Fatalf function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func fatal3() string {
+		if f() {
+			a := 1
+			log.Fatalln("x")
+		} else { // MATCH /if block ends with call to log.Fatalln function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func exit1() string {
+		if f() {
+			a := 1
+			os.Exit(2)
+		} else { // MATCH /if block ends with call to os.Exit function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func Panic1() string {
+		if f() {
+			a := 1
+			log.Panic(2)
+		} else { // MATCH /if block ends with call to log.Panic function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func Panic2() string {
+		if f() {
+			a := 1
+			log.Panicf(2)
+		} else { // MATCH /if block ends with call to log.Panicf function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+func Panic3() string {
+		if f() {
+			a := 1
+			log.Panicln(2)
+		} else { // MATCH /if block ends with call to log.Panicln function, so drop this else and outdent its block/
+			log.Printf("non-positive")
+		}
+	return "ok"
+}
+
+// noreg_19 no-regression test for issue #19 (https://github.com/mgechev/revive/issues/19)
+func noreg_19(f func() bool, x int) string {
+	if err == author.ErrCourseNotFound {
+		break
+	} else if err == author.ErrCourseAccess {
+		// side effect
+	} else if err == author.AnotherError {
+		os.Exit(1) // "okay"
+	} else {
+		// side effect
+	}
+}

rule/modifies-param.go

@@ -0,0 +1,94 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// ModifiesParamRule lints given else constructs.
+type ModifiesParamRule struct{}
+
+// Apply applies the rule to given file.
+func (r *ModifiesParamRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := lintModifiesParamRule{onFailure: onFailure}
+	ast.Walk(w, file.AST)
+	return failures
+}
+
+// Name returns the rule name.
+func (r *ModifiesParamRule) Name() string {
+	return "modifies-parameter"
+}
+
+type lintModifiesParamRule struct {
+	params    map[string]bool
+	onFailure func(lint.Failure)
+}
+
+func retrieveParamNames(pl []*ast.Field) map[string]bool {
+	result := make(map[string]bool, len(pl))
+	for _, p := range pl {
+		for _, n := range p.Names {
+			result[n.Name] = true
+		}
+	}
+	return result
+}
+
+func checkAssign(lhs []ast.Expr, w lintModifiesParamRule) {
+	for _, e := range lhs {
+		id, ok := e.(*ast.Ident)
+		if ok {
+			if w.params[id.Name] {
+				w.onFailure(lint.Failure{
+					Confidence: 0.5,
+					Node:       id,
+					Category:   "bad practice",
+					URL:        "#modifies-parameter",
+					Failure:    fmt.Sprintf("parameter '%s' seems to be a modified", id.Name),
+				})
+			}
+		}
+	}
+}
+
+func (w lintModifiesParamRule) Visit(node ast.Node) ast.Visitor {
+	switch v := node.(type) {
+	case *ast.FuncDecl:
+		w.params = retrieveParamNames(v.Type.Params.List)
+	case *ast.IncDecStmt:
+		if id, ok := v.X.(*ast.Ident); ok {
+			checkParam(id, &w)
+		}
+	case *ast.AssignStmt:
+		lhs := v.Lhs
+		for _, e := range lhs {
+			id, ok := e.(*ast.Ident)
+			if ok {
+				checkParam(id, &w)
+			}
+		}
+	}
+
+	return w
+}
+
+func checkParam(id *ast.Ident, w *lintModifiesParamRule) {
+	if w.params[id.Name] {
+		w.onFailure(lint.Failure{
+			Confidence: 0.5, // confidence is low because of shadow variables
+			Node:       id,
+			Category:   "bad practice",
+			URL:        "#modifies-parameter",
+			Failure:    fmt.Sprintf("parameter '%s' seems to be modified", id),
+		})
+	}
+}

rule/superfluous-else.go

@@ -1,6 +1,7 @@
 package rule
 
 import (
+	"fmt"
 	"go/ast"
 	"go/token"
 
@@ -13,12 +14,23 @@ type SuperfluousElseRule struct{}
 // Apply applies the rule to given file.
 func (r *SuperfluousElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
-
 	onFailure := func(failure lint.Failure) {
 		failures = append(failures, failure)
 	}
 
-	w := lintSuperfluousElse{make(map[*ast.IfStmt]bool), onFailure}
+	var branchingFunctions = map[string]map[string]bool{
+		"os": map[string]bool{"Exit": true},
+		"log": map[string]bool{
+			"Fatal":   true,
+			"Fatalf":  true,
+			"Fatalln": true,
+			"Panic":   true,
+			"Panicf":  true,
+			"Panicln": true,
+		},
+	}
+
+	w := lintSuperfluousElse{make(map[*ast.IfStmt]bool), onFailure, branchingFunctions}
 	ast.Walk(w, file.AST)
 	return failures
 }
@@ -29,8 +41,9 @@ func (r *SuperfluousElseRule) Name() string {
 }
 
 type lintSuperfluousElse struct {
-	ignore    map[*ast.IfStmt]bool
-	onFailure func(lint.Failure)
+	ignore             map[*ast.IfStmt]bool
+	onFailure          func(lint.Failure)
+	branchingFunctions map[string]map[string]bool
 }
 
 func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor {
@@ -39,6 +52,9 @@ func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor {
 		return w
 	}
 	if w.ignore[ifStmt] {
+		if elseif, ok := ifStmt.Else.(*ast.IfStmt); ok {
+			w.ignore[elseif] = true
+		}
 		return w
 	}
 	if elseif, ok := ifStmt.Else.(*ast.IfStmt); ok {
@@ -64,18 +80,36 @@ func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor {
 	}
 
 	lastStmt := ifStmt.Body.List[len(ifStmt.Body.List)-1]
-	if stmt, ok := lastStmt.(*ast.BranchStmt); ok {
+	switch stmt := lastStmt.(type) {
+	case *ast.BranchStmt:
 		token := stmt.Tok.String()
 		if token != "fallthrough" {
-			w.onFailure(lint.Failure{
-				Confidence: 1,
-				Node:       ifStmt.Else,
-				Category:   "indent",
-				URL:        "#indent-error-flow",
-				Failure:    "if block ends with a " + token + " statement, so drop this else and outdent its block" + extra,
-			})
+			w.onFailure(newFailure(ifStmt.Else, "if block ends with a "+token+" statement, so drop this else and outdent its block"+extra))
+		}
+	case *ast.ExprStmt:
+		if ce, ok := stmt.X.(*ast.CallExpr); ok { // it's a function call
+			if fc, ok := ce.Fun.(*ast.SelectorExpr); ok {
+				if id, ok := fc.X.(*ast.Ident); ok {
+					fn := fc.Sel.Name
+					pkg := id.Name
+					if w.branchingFunctions[pkg][fn] { // it's a call to a branching function
+						w.onFailure(
+							newFailure(ifStmt.Else, fmt.Sprintf("if block ends with call to %s.%s function, so drop this else and outdent its block%s", pkg, fn, extra)))
+					}
+				}
+			}
 		}
 	}
 
 	return w
 }
+
+func newFailure(node ast.Node, msg string) lint.Failure {
+	return lint.Failure{
+		Confidence: 1,
+		Node:       node,
+		Category:   "indent",
+		URL:        "#indent-error-flow",
+		Failure:    msg,
+	}
+}

test/modifies-param_test.go

@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+// TestModifiesParam rule.
+func TestModifiesParam(t *testing.T) {
+	testRule(t, "modifies-param", &rule.ModifiesParamRule{})
+}