Skip to content

Commit

Permalink
Fix for legacy named arguments (bazelbuild#692)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vladmos
vladmos authored Aug 8, 2019
1 parent 9a7671b commit ac35884
Show file tree
Hide file tree
Showing 4 changed files with 312 additions and 9 deletions.
12 changes: 12 additions & 0 deletions WARNINGS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Warning categories supported by buildifier's linter:
* [git-repository](#git-repository)
* [http-archive](#http-archive)
* [integer-division](#integer-division)
* [keyword-positional-params](#keyword-positional-params)
* [load](#load)
* [load-on-top](#load-on-top)
* [module-docstring](#module-docstring)
Expand Down Expand Up @@ -400,6 +401,17 @@ d //= e

--------------------------------------------------------------------------------

## <a name="keyword-positional-params"></a>Keyword parameter should be positional

* Category_name: `keyword-positional-params`
* Automatic fix: yes

Some parameters for builtin functions in Starlark are keyword for legacy reasons;
their names are not meaningful (e.g. `x`). Making them positional-only will improve
the readability.

--------------------------------------------------------------------------------

## <a name="load"></a>Loaded symbol is unused

* Category name: `load`
Expand Down
1 change: 1 addition & 0 deletions warn/warn.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
"git-repository": nativeGitRepositoryWarning,
"http-archive": nativeHTTPArchiveWarning,
"integer-division": integerDivisionWarning,
"keyword-positional-params": keywordPositionalParametersWarning,
"load": unusedLoadWarning,
"load-on-top": loadOnTopWarning,
"module-docstring": moduleDocstringWarning,
Expand Down
152 changes: 152 additions & 0 deletions warn/warn_bazel_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -832,3 +832,155 @@ func ruleImplReturnWarning(f *build.File) []*LinterFinding {

return findings
}

type signature struct {
Positional []string // These parameters are typePositional-only
Keyword []string // These parameters are typeKeyword-only
}

var signatures = map[string]signature{
"all": {[]string{"elements"}, []string{}},
"any": {[]string{"elements"}, []string{}},
"tuple": {[]string{"x"}, []string{}},
"list": {[]string{"x"}, []string{}},
"len": {[]string{"x"}, []string{}},
"str": {[]string{"x"}, []string{}},
"repr": {[]string{"x"}, []string{}},
"bool": {[]string{"x"}, []string{}},
"int": {[]string{"x"}, []string{}},
"dir": {[]string{"x"}, []string{}},
"type": {[]string{"x"}, []string{}},
"hasattr": {[]string{"x", "name"}, []string{}},
"getattr": {[]string{"x", "name", "default"}, []string{}},
"select": {[]string{"x"}, []string{}},
"glob": {[]string{"include"}, []string{"exclude", "exclude_directories"}},
}

// functionName returns the name of the given function if it's a direct function call (e.g.
// `foo(...)` or `native.foo(...)`, but not `foo.bar(...)` or `x[3](...)`
func functionName(call *build.CallExpr) (string, bool) {
if ident, ok := call.X.(*build.Ident); ok {
return ident.Name, true
}
// Also check for `native.<name>`
dot, ok := call.X.(*build.DotExpr)
if !ok {
return "", false
}
if ident, ok := dot.X.(*build.Ident); !ok || ident.Name != "native" {
return "", false
}
return dot.Name, true
}

const (
typePositional int = iota
typeKeyword
typeArgs
typeKwargs
)

// paramType returns the type of the param. If it's a typeKeyword param, also returns its name
func paramType(param build.Expr) (int, string) {
switch param := param.(type) {
case *build.AssignExpr:
if param.Op == "=" {
ident, ok := param.LHS.(*build.Ident)
if ok {
return typeKeyword, ident.Name
}
return typeKeyword, ""
}
case *build.UnaryExpr:
switch param.Op {
case "*":
return typeArgs, ""
case "**":
return typeKwargs, ""
}
}
return typePositional, ""
}

// keywordPositionalParametersWarning checks for deprecated typeKeyword parameters of builtins
func keywordPositionalParametersWarning(f *build.File) []*LinterFinding {
var findings []*LinterFinding

// Check for legacy typeKeyword parameters
build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
call, ok := (*expr).(*build.CallExpr)
if !ok || len(call.List) == 0 {
return
}
function, ok := functionName(call)
if !ok {
return
}

// Findings and replacements for the current call expression
var callFindings []*LinterFinding
var callReplacements []LinterReplacement

signature, ok := signatures[function]
if !ok {
return
}

var paramTypes []int // types of the parameters (typeKeyword or not) after the replacements has been applied.
for i, parameter := range call.List {
pType, name := paramType(parameter)
paramTypes = append(paramTypes, pType)

if pType == typeKeyword && i < len(signature.Positional) && signature.Positional[i] == name {
// The parameter should be typePositional
callFindings = append(callFindings, makeLinterFinding(
parameter,
fmt.Sprintf(`Keyword parameter %q for %q should be positional.`, signature.Positional[i], function),
))
callReplacements = append(callReplacements, LinterReplacement{&call.List[i], makePositional(parameter)})
paramTypes[i] = typePositional
}

if pType == typePositional && i >= len(signature.Positional) && i < len(signature.Positional)+len(signature.Keyword) {
// The parameter should be typeKeyword
keyword := signature.Keyword[i-len(signature.Positional)]
callFindings = append(callFindings, makeLinterFinding(
parameter,
fmt.Sprintf(`Parameter at the position %d for %q should be keyword (%s = ...).`, i+1, function, keyword),
))
callReplacements = append(callReplacements, LinterReplacement{&call.List[i], makeKeyword(parameter, keyword)})
paramTypes[i] = typeKeyword
}
}

if len(callFindings) == 0 {
return
}

// Only apply the replacements if the signature is correct after they have been applied
// (i.e. the order of the parameters is typePositional, typeKeyword, typeArgs, typeKwargs)
// Otherwise the signature will be not correct, probably it was incorrect initially.
// All the replacements should be applied to the first finding for the current node.

if sort.IntsAreSorted(paramTypes) {
// It's possible that the parameter list had `ForceCompact` set to true because it only contained
// positional arguments, and now it has keyword arguments as well. Reset the flag to let the
// printer decide how the function call should be formatted.
for _, t := range paramTypes {
if t == typeKeyword {
// There's at least one keyword argument
newCall := *call
newCall.ForceCompact = false
callFindings[0].Replacement = append(callFindings[0].Replacement, LinterReplacement{expr, &newCall})
break
}
}
// Attach all the parameter replacements to the first finding
callFindings[0].Replacement = append(callFindings[0].Replacement, callReplacements...)
}

findings = append(findings, callFindings...)
})

return findings
}
156 changes: 147 additions & 9 deletions warn/warn_bazel_api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ java_test()
}

func TestNativePyWarning(t *testing.T) {
checkFindingsAndFix(t, "native-py", `
checkFindingsAndFix(t, "native-py", `
"""My file"""
def macro():
Expand All @@ -604,14 +604,14 @@ def macro():
py_test()
`, tables.PyLoadPath),
[]string{
fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
},
scopeBzl|scopeBuild)
[]string{
fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
},
scopeBzl|scopeBuild)
}

func TestNativeProtoWarning(t *testing.T) {
Expand Down Expand Up @@ -650,3 +650,141 @@ def macro():
},
scopeBzl|scopeBuild)
}

func TestKeywordParameters(t *testing.T) {
checkFindingsAndFix(t, "keyword-positional-params", `
foo(key = value)
all(elements = [True, False])
any(elements = [True, False])
tuple(x = [1, 2, 3])
list(x = [1, 2, 3])
len(x = [1, 2, 3])
str(x = foo)
repr(x = foo)
bool(x = 3)
int(x = "3")
int(x = "13", base = 8)
dir(x = foo)
type(x = foo)
select(x = {})
`, `
foo(key = value)
all([True, False])
any([True, False])
tuple([1, 2, 3])
list([1, 2, 3])
len([1, 2, 3])
str(foo)
repr(foo)
bool(3)
int("3")
int("13", base = 8)
dir(foo)
type(foo)
select({})
`, []string{
`:2: Keyword parameter "elements" for "all" should be positional.`,
`:3: Keyword parameter "elements" for "any" should be positional.`,
`:4: Keyword parameter "x" for "tuple" should be positional.`,
`:5: Keyword parameter "x" for "list" should be positional.`,
`:6: Keyword parameter "x" for "len" should be positional.`,
`:7: Keyword parameter "x" for "str" should be positional.`,
`:8: Keyword parameter "x" for "repr" should be positional.`,
`:9: Keyword parameter "x" for "bool" should be positional.`,
`:10: Keyword parameter "x" for "int" should be positional.`,
`:11: Keyword parameter "x" for "int" should be positional.`,
`:12: Keyword parameter "x" for "dir" should be positional.`,
`:13: Keyword parameter "x" for "type" should be positional.`,
`:14: Keyword parameter "x" for "select" should be positional.`,
}, scopeEverywhere)

checkFindingsAndFix(t, "keyword-positional-params", `
hasattr(
x = foo,
name = "bar",
)
getattr(
x = foo,
name = "bar",
)
getattr(
x = foo,
name = "bar",
default = "baz",
)
`, `
hasattr(
foo,
"bar",
)
getattr(
foo,
"bar",
)
getattr(
foo,
"bar",
"baz",
)
`, []string{
`:2: Keyword parameter "x" for "hasattr" should be positional.`,
`:3: Keyword parameter "name" for "hasattr" should be positional.`,
`:6: Keyword parameter "x" for "getattr" should be positional.`,
`:7: Keyword parameter "name" for "getattr" should be positional.`,
`:10: Keyword parameter "x" for "getattr" should be positional.`,
`:11: Keyword parameter "name" for "getattr" should be positional.`,
`:12: Keyword parameter "default" for "getattr" should be positional.`,
}, scopeEverywhere)

checkFindingsAndFix(t, "keyword-positional-params", `
glob(["*.cc"], ["test*"])
glob(["*.cc"])
glob(include = [], exclude = [])
glob([], exclude = [])
glob([], [], 1)
glob([], [], 1, 2)
glob(*args, [])
`, `
glob(["*.cc"], exclude = ["test*"])
glob(["*.cc"])
glob([], exclude = [])
glob([], exclude = [])
glob([], exclude = [], exclude_directories = 1)
glob([], [], 1, 2)
glob(*args, [])
`, []string{
`:1: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
`:3: Keyword parameter "include" for "glob" should be positional.`,
`:5: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
`:5: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
`:6: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
`:6: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
`:7: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
}, scopeEverywhere)

checkFindingsAndFix(t, "keyword-positional-params", `
native.glob(["*.cc"], ["test*"])
native.glob(["*.cc"])
native.glob(include = [], exclude = [])
native.glob([], exclude = [])
native.glob([], [], 1)
native.glob([], [], 1, 2)
native.glob(*args, [])
`, `
native.glob(["*.cc"], exclude = ["test*"])
native.glob(["*.cc"])
native.glob([], exclude = [])
native.glob([], exclude = [])
native.glob([], exclude = [], exclude_directories = 1)
native.glob([], [], 1, 2)
native.glob(*args, [])
`, []string{
`:1: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
`:3: Keyword parameter "include" for "glob" should be positional.`,
`:5: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
`:5: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
`:6: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
`:6: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
`:7: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
}, scopeEverywhere)
}

0 comments on commit ac35884

Please sign in to comment.