Repository: bazelbuild/buildtools
Pull request #837 (2020-05-19 17:59:20)

Implement a warning for the word 'skylark' in comments and docstrings


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 819bbe9..ae8bc89 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -53,6 +53,8 @@ Warning categories supported by buildifier's linter:
   * [`return-value`](#return-value)
   * [`rule-impl-return`](#rule-impl-return)
   * [`same-origin-load`](#same-origin-load)
+  * [`skylark-comment`](#skylark-comment)
+  * [`skylark-docstring`](#skylark-docstring)
   * [`string-iteration`](#string-iteration)
   * [`uninitialized`](#uninitialized)
   * [`unnamed-macro`](#unnamed-macro)
@@ -880,6 +882,16 @@ load(":f.bzl", "s1", "s2")
 
 --------------------------------------------------------------------------------
 
+## <a name="skylark-comment"></a><a name="skylark-docstring"></a>"Skylark" is an outdated name of the language, please use "starlark" instead
+
+  * Category name: `skylark-comment`, `skylark-docstring`
+  * Automatic fix: yes
+
+The configuration language for Bazel is called "Starlark" now, the name "Skylark" is
+outdated and shouldn't be used.
+
+--------------------------------------------------------------------------------
+
 ## <a name="string-iteration"></a>String iteration is deprecated
 
   * Category name: `string-iteration`
diff --git a/build/syntax.go b/build/syntax.go
index c8b21d5..e68d84f 100644
--- a/build/syntax.go
+++ b/build/syntax.go
@@ -54,6 +54,11 @@ type Expr interface {
 	// This method would normally be named 'Comments' but that
 	// would interfere with embedding a type of the same name.
 	Comment() *Comments
+
+	// Copy returns a non-deep copy of the node. Can be useful if
+	// the actual node type is hidden by the Expr interface and
+	// not relevant.
+	Copy() Expr
 }
 
 // A Comment represents a single # comment.
@@ -62,6 +67,10 @@ type Comment struct {
 	Token string // without trailing newline
 }
 
+func (c Comment) Span() (start, end Position) {
+	return c.Start, c.Start.add(c.Token)
+}
+
 // Comments collects the comments associated with an expression.
 type Comments struct {
 	Before []Comment // whole-line comments before this expression
@@ -128,6 +137,11 @@ func (f *File) Span() (start, end Position) {
 	return start, end
 }
 
+func (f *File) Copy() Expr {
+	n := *f
+	return &n
+}
+
 // A CommentBlock represents a top-level block of comments separate
 // from any rule.
 type CommentBlock struct {
@@ -139,6 +153,11 @@ func (x *CommentBlock) Span() (start, end Position) {
 	return x.Start, x.Start
 }
 
+func (x *CommentBlock) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An Ident represents an identifier.
 type Ident struct {
 	Comments
@@ -150,6 +169,21 @@ func (x *Ident) Span() (start, end Position) {
 	return x.NamePos, x.NamePos.add(x.Name)
 }
 
+func (x *Ident) Copy() Expr {
+	n := *x
+	return &n
+}
+
+func (x *Ident) asString() *StringExpr {
+	_, end := x.Span()
+	return &StringExpr{
+		Comments: x.Comments,
+		Start:    x.NamePos,
+		Value:    x.Name,
+		End:      end,
+	}
+}
+
 // BranchStmt represents a `pass`, `break`, or `continue` statement.
 type BranchStmt struct {
 	Comments
@@ -161,14 +195,9 @@ func (x *BranchStmt) Span() (start, end Position) {
 	return x.TokenPos, x.TokenPos.add(x.Token)
 }
 
-func (x *Ident) asString() *StringExpr {
-	_, end := x.Span()
-	return &StringExpr{
-		Comments: x.Comments,
-		Start:    x.NamePos,
-		Value:    x.Name,
-		End:      end,
-	}
+func (x *BranchStmt) Copy() Expr {
+	n := *x
+	return &n
 }
 
 // A LiteralExpr represents a literal number.
@@ -182,6 +211,11 @@ func (x *LiteralExpr) Span() (start, end Position) {
 	return x.Start, x.Start.add(x.Token)
 }
 
+func (x *LiteralExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A StringExpr represents a single literal string.
 type StringExpr struct {
 	Comments
@@ -201,6 +235,11 @@ func (x *StringExpr) Span() (start, end Position) {
 	return x.Start, x.End
 }
 
+func (x *StringExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An End represents the end of a parenthesized or bracketed expression.
 // It is a place to hang comments.
 type End struct {
@@ -212,6 +251,11 @@ func (x *End) Span() (start, end Position) {
 	return x.Pos, x.Pos.add(")")
 }
 
+func (x *End) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A CallExpr represents a function call expression: X(List).
 type CallExpr struct {
 	Comments
@@ -228,6 +272,11 @@ func (x *CallExpr) Span() (start, end Position) {
 	return start, x.End.Pos.add(")")
 }
 
+func (x *CallExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A DotExpr represents a field selector: X.Name.
 type DotExpr struct {
 	Comments
@@ -242,6 +291,11 @@ func (x *DotExpr) Span() (start, end Position) {
 	return start, x.NamePos.add(x.Name)
 }
 
+func (x *DotExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A Comprehension represents a list comprehension expression: [X for ... if ...].
 type Comprehension struct {
 	Comments
@@ -257,6 +311,11 @@ func (x *Comprehension) Span() (start, end Position) {
 	return x.Lbrack, x.End.Pos.add("]")
 }
 
+func (x *Comprehension) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A ForClause represents a for clause in a list comprehension: for Var in Expr.
 type ForClause struct {
 	Comments
@@ -271,6 +330,11 @@ func (x *ForClause) Span() (start, end Position) {
 	return x.For, end
 }
 
+func (x *ForClause) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An IfClause represents an if clause in a list comprehension: if Cond.
 type IfClause struct {
 	Comments
@@ -283,6 +347,11 @@ func (x *IfClause) Span() (start, end Position) {
 	return x.If, end
 }
 
+func (x *IfClause) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A KeyValueExpr represents a dictionary entry: Key: Value.
 type KeyValueExpr struct {
 	Comments
@@ -297,6 +366,11 @@ func (x *KeyValueExpr) Span() (start, end Position) {
 	return start, end
 }
 
+func (x *KeyValueExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A DictExpr represents a dictionary literal: { List }.
 type DictExpr struct {
 	Comments
@@ -310,6 +384,11 @@ func (x *DictExpr) Span() (start, end Position) {
 	return x.Start, x.End.Pos.add("}")
 }
 
+func (x *DictExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A ListExpr represents a list literal: [ List ].
 type ListExpr struct {
 	Comments
@@ -323,6 +402,11 @@ func (x *ListExpr) Span() (start, end Position) {
 	return x.Start, x.End.Pos.add("]")
 }
 
+func (x *ListExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A SetExpr represents a set literal: { List }.
 type SetExpr struct {
 	Comments
@@ -336,6 +420,11 @@ func (x *SetExpr) Span() (start, end Position) {
 	return x.Start, x.End.Pos.add("}")
 }
 
+func (x *SetExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A TupleExpr represents a tuple literal: (List)
 type TupleExpr struct {
 	Comments
@@ -356,6 +445,11 @@ func (x *TupleExpr) Span() (start, end Position) {
 	return start, end
 }
 
+func (x *TupleExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A UnaryExpr represents a unary expression: Op X.
 type UnaryExpr struct {
 	Comments
@@ -372,6 +466,11 @@ func (x *UnaryExpr) Span() (start, end Position) {
 	return x.OpStart, end
 }
 
+func (x *UnaryExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A BinaryExpr represents a binary expression: X Op Y.
 type BinaryExpr struct {
 	Comments
@@ -388,6 +487,11 @@ func (x *BinaryExpr) Span() (start, end Position) {
 	return start, end
 }
 
+func (x *BinaryExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An AssignExpr represents a binary expression with `=`: LHS = RHS.
 type AssignExpr struct {
 	Comments
@@ -404,6 +508,11 @@ func (x *AssignExpr) Span() (start, end Position) {
 	return start, end
 }
 
+func (x *AssignExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A ParenExpr represents a parenthesized expression: (X).
 type ParenExpr struct {
 	Comments
@@ -417,6 +526,11 @@ func (x *ParenExpr) Span() (start, end Position) {
 	return x.Start, x.End.Pos.add(")")
 }
 
+func (x *ParenExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A SliceExpr represents a slice expression: expr[from:to] or expr[from:to:step] .
 type SliceExpr struct {
 	Comments
@@ -435,6 +549,11 @@ func (x *SliceExpr) Span() (start, end Position) {
 	return start, x.End.add("]")
 }
 
+func (x *SliceExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An IndexExpr represents an index expression: X[Y].
 type IndexExpr struct {
 	Comments
@@ -449,6 +568,11 @@ func (x *IndexExpr) Span() (start, end Position) {
 	return start, x.End.add("]")
 }
 
+func (x *IndexExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A Function represents the common parts of LambdaExpr and DefStmt
 type Function struct {
 	Comments
@@ -462,6 +586,11 @@ func (x *Function) Span() (start, end Position) {
 	return x.StartPos, end
 }
 
+func (x *Function) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A LambdaExpr represents a lambda expression: lambda Var: Expr.
 type LambdaExpr struct {
 	Comments
@@ -472,6 +601,11 @@ func (x *LambdaExpr) Span() (start, end Position) {
 	return x.Function.Span()
 }
 
+func (x *LambdaExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // ConditionalExpr represents the conditional: X if TEST else ELSE.
 type ConditionalExpr struct {
 	Comments
@@ -490,6 +624,11 @@ func (x *ConditionalExpr) Span() (start, end Position) {
 	return start, end
 }
 
+func (x *ConditionalExpr) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A LoadStmt loads another module and binds names from it:
 // load(Module, "x", y="foo").
 //
@@ -512,6 +651,11 @@ func (x *LoadStmt) Span() (start, end Position) {
 	return x.Load, x.Rparen.Pos.add(")")
 }
 
+func (x *LoadStmt) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A DefStmt represents a function definition expression: def foo(List):.
 type DefStmt struct {
 	Comments
@@ -526,6 +670,11 @@ func (x *DefStmt) Span() (start, end Position) {
 	return x.Function.Span()
 }
 
+func (x *DefStmt) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // HeaderSpan returns the span of the function header `def f(...):`
 func (x *DefStmt) HeaderSpan() (start, end Position) {
 	return x.Function.StartPos, x.ColonPos
@@ -546,6 +695,11 @@ func (x *ReturnStmt) Span() (start, end Position) {
 	return x.Return, end
 }
 
+func (x *ReturnStmt) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // A ForStmt represents a for loop block: for x in range(10):.
 type ForStmt struct {
 	Comments
@@ -561,6 +715,11 @@ func (x *ForStmt) Span() (start, end Position) {
 	return x.For, end
 }
 
+func (x *ForStmt) Copy() Expr {
+	n := *x
+	return &n
+}
+
 // An IfStmt represents an if-else block: if x: ... else: ... .
 // `elif`s are treated as a chain of `IfStmt`s.
 type IfStmt struct {
@@ -580,3 +739,8 @@ func (x *IfStmt) Span() (start, end Position) {
 	end = stmtsEnd(body)
 	return x.If, end
 }
+
+func (x *IfStmt) Copy() Expr {
+	n := *x
+	return &n
+}
diff --git a/warn/warn.go b/warn/warn.go
index e76e6aa..bc16ed2 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -149,6 +149,8 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"rule-impl-return":          ruleImplReturnWarning,
 	"return-value":              missingReturnValueWarning,
 	"same-origin-load":          sameOriginLoadWarning,
+	"skylark-comment":           skylarkCommentWarning,
+	"skylark-docstring":         skylarkDocstringWarning,
 	"string-iteration":          stringIterationWarning,
 	"uninitialized":             uninitializedVariableWarning,
 	"unreachable":               unreachableStatementWarning,
@@ -159,7 +161,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 // MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
 var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding{
 	"deprecated-function": deprecatedFunctionWarning,
-	"unnamed-macro": unnamedMacroWarning,
+	"unnamed-macro":       unnamedMacroWarning,
 }
 
 // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index 6da01d6..45d7e42 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -4,6 +4,7 @@ package warn
 
 import (
 	"fmt"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -372,3 +373,105 @@ func unsortedDictItemsWarning(f *build.File) []*LinterFinding {
 	})
 	return findings
 }
+
+// skylarkToStarlark converts a string "skylark" in different cases to "starlark"
+// trying to preserve the same case style, e.g. capitalized "Skylark" becomes "Starlark".
+func skylarkToStarlark(s string) string {
+	switch {
+	case s == "SKYLARK":
+		return "STARLARK"
+	case strings.HasPrefix(s, "S"):
+		return "Starlark"
+	default:
+		return "starlark"
+	}
+}
+
+func replaceSkylark(s string) (newString string, changed bool) {
+	skylarkRegex := regexp.MustCompile("(?i)skylark")
+	newString = skylarkRegex.ReplaceAllStringFunc(s, skylarkToStarlark)
+	return newString, newString != s
+}
+
+func skylarkCommentWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
+	msg := `"Skylark" is an outdated name of the language, please use "starlark" instead.`
+
+	// Check comments
+	build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
+		comments := (*expr).Comment()
+		newComments := build.Comments{
+			Before: append([]build.Comment{}, comments.Before...),
+			Suffix: append([]build.Comment{}, comments.Suffix...),
+			After:  append([]build.Comment{}, comments.After...),
+		}
+		isModified := false
+		var start, end build.Position
+
+		for _, block := range []*[]build.Comment{&newComments.Before, &newComments.Suffix, &newComments.After} {
+			for i, comment := range *block {
+				newValue, changed := replaceSkylark(comment.Token)
+				(*block)[i] = build.Comment{
+					Start: comment.Start,
+					Token: newValue,
+				}
+				if changed {
+					isModified = true
+					start, end = comment.Span()
+				}
+			}
+		}
+		if !isModified {
+			return
+		}
+		newExpr := (*expr).Copy()
+		newExpr.Comment().Before = newComments.Before
+		newExpr.Comment().Suffix = newComments.Suffix
+		newExpr.Comment().After = newComments.After
+		finding := makeLinterFinding(*expr, msg, LinterReplacement{expr, newExpr})
+		finding.Start = start
+		finding.End = end
+		findings = append(findings, finding)
+	})
+
+	return findings
+}
+
+func checkSkylarkDocstring(stmts []build.Expr) *LinterFinding {
+	msg := `"Skylark" is an outdated name of the language, please use "starlark" instead.`
+
+	doc, ok := getDocstring(stmts)
+	if !ok {
+		return nil
+	}
+	docString := (*doc).(*build.StringExpr)
+	newValue, updated := replaceSkylark(docString.Value)
+	if !updated {
+		return nil
+	}
+	newDocString := *docString
+	newDocString.Value = newValue
+	return makeLinterFinding(docString, msg, LinterReplacement{doc, &newDocString})
+}
+
+func skylarkDocstringWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
+
+	// File docstring
+	if finding := checkSkylarkDocstring(f.Stmt); finding != nil {
+		findings = append(findings, finding)
+	}
+
+	// Function docstrings
+	for _, stmt := range f.Stmt {
+		def, ok := stmt.(*build.DefStmt)
+		if !ok {
+			continue
+		}
+		if finding := checkSkylarkDocstring(def.Body); finding != nil {
+			findings = append(findings, finding)
+		}
+	}
+
+	return findings
+}
diff --git a/warn/warn_deprecated.go b/warn/warn_deprecated.go
index 3813978..2634fe0 100644
--- a/warn/warn_deprecated.go
+++ b/warn/warn_deprecated.go
@@ -21,7 +21,7 @@ func checkDeprecatedFunction(stmt build.Expr, loadedSymbols *map[string]*build.I
 	if !ok {
 		return nil
 	}
-	str, ok := docstring.(*build.StringExpr)
+	str, ok := (*docstring).(*build.StringExpr)
 	if !ok {
 		return nil
 	}
diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go
index 499cf61..37acac2 100644
--- a/warn/warn_docstring.go
+++ b/warn/warn_docstring.go
@@ -14,15 +14,15 @@ const FunctionLengthDocstringThreshold = 5
 
 // getDocstring returns a docstring of the statements and true if it exists.
 // Otherwise it returns the first non-comment statement and false.
-func getDocstring(stmts []build.Expr) (build.Expr, bool) {
-	for _, stmt := range stmts {
-		switch stmt := stmt.(type) {
+func getDocstring(stmts []build.Expr) (*build.Expr, bool) {
+	for i, stmt := range stmts {
+		switch stmt.(type) {
 		case *build.CommentBlock:
 			continue
 		case *build.StringExpr:
-			return stmt, true
+			return &stmts[i], true
 		default:
-			return stmt, false
+			return &stmts[i], false
 		}
 	}
 	return nil, false
@@ -33,13 +33,13 @@ func moduleDocstringWarning(f *build.File) []*LinterFinding {
 		return nil
 	}
 	if stmt, ok := getDocstring(f.Stmt); stmt != nil && !ok {
-		start, _ := stmt.Span()
+		start, _ := (*stmt).Span()
 		end := build.Position{
 			Line:     start.Line,
 			LineRune: start.LineRune + 1,
 			Byte:     start.Byte + 1,
 		}
-		finding := makeLinterFinding(stmt, `The file has no module docstring.
+		finding := makeLinterFinding(*stmt, `The file has no module docstring.
 A module docstring is a string literal (not a comment) which should be the first statement of a file (it may follow comment lines).`)
 		finding.End = end
 		return []*LinterFinding{finding}
@@ -247,11 +247,11 @@ func functionDocstringHeaderWarning(f *build.File) []*LinterFinding {
 			continue
 		}
 
-		info := parseFunctionDocstring(doc.(*build.StringExpr))
+		info := parseFunctionDocstring((*doc).(*build.StringExpr))
 
 		if !info.hasHeader {
 			message := fmt.Sprintf("The docstring for the function %q should start with a one-line summary.", def.Name)
-			findings = append(findings, makeLinterFinding(doc, message))
+			findings = append(findings, makeLinterFinding(*doc, message))
 		}
 	}
 	return findings
@@ -271,13 +271,13 @@ func functionDocstringArgsWarning(f *build.File) []*LinterFinding {
 			continue
 		}
 
-		info := parseFunctionDocstring(doc.(*build.StringExpr))
+		info := parseFunctionDocstring((*doc).(*build.StringExpr))
 
 		if info.argumentsPos.LineRune > 0 {
 			argumentsEnd := info.argumentsPos
 			argumentsEnd.LineRune += len("Arguments:")
 			argumentsEnd.Byte += len("Arguments:")
-			finding := makeLinterFinding(doc, `Prefer "Args:" to "Arguments:" when documenting function arguments.`)
+			finding := makeLinterFinding(*doc, `Prefer "Args:" to "Arguments:" when documenting function arguments.`)
 			finding.Start = info.argumentsPos
 			finding.End = argumentsEnd
 			findings = append(findings, finding)
@@ -335,7 +335,7 @@ one (preferably two) space more than "Args:", for example:
         """`, plural, def.Name, notDocumentedArguments[0], notDocumentedArguments[0])
 			}
 
-			findings = append(findings, makeLinterFinding(doc, message))
+			findings = append(findings, makeLinterFinding(*doc, message))
 		}
 
 		// Check whether all documented arguments actually exist in the function signature.
@@ -353,7 +353,7 @@ one (preferably two) space more than "Args:", for example:
 			}
 			posEnd := pos
 			posEnd.LineRune += len(name)
-			finding := makeLinterFinding(doc, msg)
+			finding := makeLinterFinding(*doc, msg)
 			finding.Start = pos
 			finding.End = posEnd
 			findings = append(findings, finding)
@@ -376,12 +376,12 @@ func functionDocstringReturnWarning(f *build.File) []*LinterFinding {
 			continue
 		}
 
-		info := parseFunctionDocstring(doc.(*build.StringExpr))
+		info := parseFunctionDocstring((*doc).(*build.StringExpr))
 
 		// Check whether the return value is documented
 		if isDocstringRequired(def) && hasReturnValues(def) && !info.returns {
 			message := fmt.Sprintf("Return value of %q is not documented.", def.Name)
-			findings = append(findings, makeLinterFinding(doc, message))
+			findings = append(findings, makeLinterFinding(*doc, message))
 		}
 	}
 	return findings


Test patch:
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index b210539..57cadc4 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -455,3 +455,46 @@ d.update(
 		[]string{},
 		scopeEverywhere)
 }
+
+func TestSkylark(t *testing.T) {
+	checkFindingsAndFix(t, "skylark-comment", `
+# Skyline
+foo()
+# SkyLark
+
+# Implemented in skylark
+# Skylark
+bar() # SKYLARK
+`, `
+# Skyline
+foo()
+# Starlark
+
+# Implemented in starlark
+# Starlark
+bar() # STARLARK
+`,
+		[]string{
+			`:3: "Skylark" is an outdated name of the language, please use "starlark" instead.`,
+			`:7: "Skylark" is an outdated name of the language, please use "starlark" instead.`,
+		},
+		scopeEverywhere)
+
+	checkFindingsAndFix(t, "skylark-docstring", `
+# Some file
+
+"""
+This is a docstring describing a skylark file
+"""
+`, `
+# Some file
+
+"""
+This is a docstring describing a starlark file
+"""
+`,
+		[]string{
+			`:3: "Skylark" is an outdated name of the language, please use "starlark" instead.`,
+		},
+		scopeEverywhere)
+}