From b09bb31e0fa21cb401e08b777c95c84ceb584fe9 Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Tue, 6 Aug 2019 16:33:17 +0200 Subject: [PATCH 1/6] Fix for legacy named arguments --- WARNINGS.md | 12 +++++++ warn/warn.go | 1 + warn/warn_bazel_api.go | 52 +++++++++++++++++++++++++++ warn/warn_bazel_api_test.go | 72 ++++++++++++++++++++++++++++++++----- 4 files changed, 128 insertions(+), 9 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index b01963ec2..e5ccfdb76 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -24,6 +24,7 @@ Warning categories supported by buildifier's linter: * [git-repository](#git-repository) * [http-archive](#http-archive) * [integer-division](#integer-division) + * [keyword-parameters](#keyword-parameters) * [load](#load) * [load-on-top](#load-on-top) * [module-docstring](#module-docstring) @@ -400,6 +401,17 @@ d //= e -------------------------------------------------------------------------------- +## Keyword parameter should be positional + + * Category_name: `keyword-parameters` + * 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. + + -------------------------------------------------------------------------------- + ## Loaded symbol is unused * Category name: `load` diff --git a/warn/warn.go b/warn/warn.go index bcb0f3eb6..92143c12a 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -122,6 +122,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "git-repository": nativeGitRepositoryWarning, "http-archive": nativeHTTPArchiveWarning, "integer-division": integerDivisionWarning, + "keyword-parameters": keywordParametersWarning, "load": unusedLoadWarning, "load-on-top": loadOnTopWarning, "module-docstring": moduleDocstringWarning, diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index 7694811f1..facab86b4 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -832,3 +832,55 @@ func ruleImplReturnWarning(f *build.File) []*LinterFinding { return findings } + +var legacyNamedParameters = map[string]string{ + "all": "elements", + "any": "elements", + "tuple": "x", + "list": "x", + "len": "x", + "str": "x", + "repr": "x", + "bool": "x", + "int": "x", + "dir": "x", + "type": "x", + "hasattr": "x", + "getattr": "x", + "select": "x", +} + +// keywordParametersWarning checks for deprecated keyword parameters of builtins +func keywordParametersWarning(f *build.File) []*LinterFinding { + var findings []*LinterFinding + + build.Walk(f, func(expr build.Expr, stack []build.Expr) { + call, ok := expr.(*build.CallExpr) + if !ok || len(call.List) == 0 { + return + } + ident, ok := call.X.(*build.Ident) + if !ok { + return + } + parameter, ok := legacyNamedParameters[ident.Name] + if !ok { + return + } + assign, ok := call.List[0].(*build.AssignExpr) + if !ok || assign.Op != "=" { + return + } + key, ok := assign.LHS.(*build.Ident) + if !ok || key.Name != parameter { + return + } + + findings = append(findings, makeLinterFinding( + call, + fmt.Sprintf(`Keyword parameter "%s" for "%s" should be positional.`, key.Name, ident.Name), + LinterReplacement{&call.List[0], makePositional(call.List[0])})) + }) + + return findings +} diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index bc770812a..589e8171a 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -581,7 +581,7 @@ java_test() } func TestNativePyWarning(t *testing.T) { - checkFindingsAndFix(t, "native-py", ` + checkFindingsAndFix(t, "native-py", ` """My file""" def macro(): @@ -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) { @@ -650,3 +650,57 @@ def macro(): }, scopeBzl|scopeBuild) } + +func TestKeywordParameters(t *testing.T) { + checkFindingsAndFix(t, "keyword-parameters", ` +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) +hasattr(x = foo, name = "bar") +getattr(x = foo, name = "bar", default = "baz") +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) +hasattr(foo, name = "bar") +getattr(foo, name = "bar", default = "baz") +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 "hasattr" should be positional.`, + `:15: Keyword parameter "x" for "getattr" should be positional.`, + `:16: Keyword parameter "x" for "select" should be positional.`, + }, scopeEverywhere) +} From b011aa3a575bea31b98d320c1ac6663393e80c4a Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Tue, 6 Aug 2019 16:34:29 +0200 Subject: [PATCH 2/6] gofmt --- warn/warn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warn/warn.go b/warn/warn.go index 92143c12a..114b5a799 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -122,7 +122,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "git-repository": nativeGitRepositoryWarning, "http-archive": nativeHTTPArchiveWarning, "integer-division": integerDivisionWarning, - "keyword-parameters": keywordParametersWarning, + "keyword-parameters": keywordParametersWarning, "load": unusedLoadWarning, "load-on-top": loadOnTopWarning, "module-docstring": moduleDocstringWarning, From 1f84bcc77408131e9c1900c24373dc2eb2109df9 Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Tue, 6 Aug 2019 16:53:51 +0200 Subject: [PATCH 3/6] Warn for positional second argment for the glob function --- warn/warn_bazel_api.go | 52 ++++++++++++++++++++++++++++++++++++- warn/warn_bazel_api_test.go | 10 +++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index facab86b4..cb5cf40ce 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -850,10 +850,17 @@ var legacyNamedParameters = map[string]string{ "select": "x", } +var legacyPositionalParameters = map[string]map[int]string{ + "glob": { + 1: "exclude", + }, +} + // keywordParametersWarning checks for deprecated keyword parameters of builtins func keywordParametersWarning(f *build.File) []*LinterFinding { var findings []*LinterFinding + // Check for legacy keyword parameters build.Walk(f, func(expr build.Expr, stack []build.Expr) { call, ok := expr.(*build.CallExpr) if !ok || len(call.List) == 0 { @@ -878,9 +885,52 @@ func keywordParametersWarning(f *build.File) []*LinterFinding { findings = append(findings, makeLinterFinding( call, - fmt.Sprintf(`Keyword parameter "%s" for "%s" should be positional.`, key.Name, ident.Name), + fmt.Sprintf(`Keyword parameter %q for %q should be positional.`, key.Name, ident.Name), LinterReplacement{&call.List[0], makePositional(call.List[0])})) }) + // Check for legacy positional parameters + build.Walk(f, func(expr build.Expr, stack []build.Expr) { + call, ok := expr.(*build.CallExpr) + if !ok || len(call.List) == 0 { + return + } + + var name string + ident, ok := call.X.(*build.Ident) + if ok { + name = ident.Name + } else { + // Also check for `native.` + dot, ok := call.X.(*build.DotExpr) + if !ok { + return + } + ident, ok := dot.X.(*build.Ident) + if !ok || ident.Name != "native" { + return + } + name = dot.Name + } + + parameterInfo, ok := legacyPositionalParameters[name] + if !ok { + return + } + + for index, value := range parameterInfo { + if index >= len(call.List) { + continue + } + if _, ok := call.List[index].(*build.AssignExpr); ok { + continue + } + findings = append(findings, makeLinterFinding( + call, + fmt.Sprintf(`Parameter at the position %d for %q should be keyword (%s = ...).`, index+1, name, value), + LinterReplacement{&call.List[index], makeKeyword(call.List[index], value)})) + } + }) + return findings } diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index 589e8171a..8ee273fd0 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -669,6 +669,10 @@ type(x = foo) hasattr(x = foo, name = "bar") getattr(x = foo, name = "bar", default = "baz") select(x = {}) +glob(["*.cc"], ["test*"]) +native.glob(["*.cc"], ["test*"]) +glob(["*.cc"]) +native.glob(["*.cc"]) `, ` foo(key = value) all([True, False]) @@ -686,6 +690,10 @@ type(foo) hasattr(foo, name = "bar") getattr(foo, name = "bar", default = "baz") select({}) +glob(["*.cc"], exclude = ["test*"]) +native.glob(["*.cc"], exclude = ["test*"]) +glob(["*.cc"]) +native.glob(["*.cc"]) `, []string{ `:2: Keyword parameter "elements" for "all" should be positional.`, `:3: Keyword parameter "elements" for "any" should be positional.`, @@ -702,5 +710,7 @@ select({}) `:14: Keyword parameter "x" for "hasattr" should be positional.`, `:15: Keyword parameter "x" for "getattr" should be positional.`, `:16: Keyword parameter "x" for "select" should be positional.`, + `:17: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`, + `:18: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`, }, scopeEverywhere) } From 5f7867a7489ddd4cc959ac120a4219740a547a6d Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Wed, 7 Aug 2019 20:01:40 +0200 Subject: [PATCH 4/6] Refactor the warning --- WARNINGS.md | 6 +- warn/warn.go | 2 +- warn/warn_bazel_api.go | 184 +++++++++++++++++++++--------------- warn/warn_bazel_api_test.go | 54 ++++++++--- 4 files changed, 157 insertions(+), 89 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index e5ccfdb76..a9f4326c1 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -24,7 +24,7 @@ Warning categories supported by buildifier's linter: * [git-repository](#git-repository) * [http-archive](#http-archive) * [integer-division](#integer-division) - * [keyword-parameters](#keyword-parameters) + * [keyword-positional-params](#keyword-positional-params) * [load](#load) * [load-on-top](#load-on-top) * [module-docstring](#module-docstring) @@ -401,9 +401,9 @@ d //= e -------------------------------------------------------------------------------- -## Keyword parameter should be positional +## Keyword parameter should be positional - * Category_name: `keyword-parameters` + * Category_name: `keyword-positional-params` * Automatic fix: yes Some parameters for builtin functions in Starlark are keyword for legacy reasons; diff --git a/warn/warn.go b/warn/warn.go index 114b5a799..3d0418dc0 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -122,7 +122,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "git-repository": nativeGitRepositoryWarning, "http-archive": nativeHTTPArchiveWarning, "integer-division": integerDivisionWarning, - "keyword-parameters": keywordParametersWarning, + "keyword-positional-params": keywordPositionalParametersWarning, "load": unusedLoadWarning, "load-on-top": loadOnTopWarning, "module-docstring": moduleDocstringWarning, diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index cb5cf40ce..d4db76b85 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -833,103 +833,139 @@ func ruleImplReturnWarning(f *build.File) []*LinterFinding { return findings } -var legacyNamedParameters = map[string]string{ - "all": "elements", - "any": "elements", - "tuple": "x", - "list": "x", - "len": "x", - "str": "x", - "repr": "x", - "bool": "x", - "int": "x", - "dir": "x", - "type": "x", - "hasattr": "x", - "getattr": "x", - "select": "x", -} - -var legacyPositionalParameters = map[string]map[int]string{ - "glob": { - 1: "exclude", - }, -} - -// keywordParametersWarning checks for deprecated keyword parameters of builtins -func keywordParametersWarning(f *build.File) []*LinterFinding { +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"}, []string{}}, + "getattr": {[]string{"x"}, []string{}}, + "select": {[]string{"x"}, []string{}}, + "glob": {[]string{"include"}, []string{"exclude"}}, +} + +// 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.` + 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 keyword parameters + // Check for legacy typeKeyword parameters build.Walk(f, func(expr build.Expr, stack []build.Expr) { call, ok := expr.(*build.CallExpr) if !ok || len(call.List) == 0 { return } - ident, ok := call.X.(*build.Ident) + function, ok := functionName(call) if !ok { return } - parameter, ok := legacyNamedParameters[ident.Name] - if !ok { - return - } - assign, ok := call.List[0].(*build.AssignExpr) - if !ok || assign.Op != "=" { - return - } - key, ok := assign.LHS.(*build.Ident) - if !ok || key.Name != parameter { - return - } - findings = append(findings, makeLinterFinding( - call, - fmt.Sprintf(`Keyword parameter %q for %q should be positional.`, key.Name, ident.Name), - LinterReplacement{&call.List[0], makePositional(call.List[0])})) - }) + // Findings and replacements for the current call expression + var callFindings []*LinterFinding + var callReplacements []LinterReplacement - // Check for legacy positional parameters - build.Walk(f, func(expr build.Expr, stack []build.Expr) { - call, ok := expr.(*build.CallExpr) - if !ok || len(call.List) == 0 { + signature, ok := signatures[function] + if !ok { return } - var name string - ident, ok := call.X.(*build.Ident) - if ok { - name = ident.Name - } else { - // Also check for `native.` - dot, ok := call.X.(*build.DotExpr) - 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 } - ident, ok := dot.X.(*build.Ident) - if !ok || ident.Name != "native" { - return + + 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 } - name = dot.Name } - parameterInfo, ok := legacyPositionalParameters[name] - if !ok { + if len(callFindings) == 0 { return } - for index, value := range parameterInfo { - if index >= len(call.List) { - continue - } - if _, ok := call.List[index].(*build.AssignExpr); ok { - continue - } - findings = append(findings, makeLinterFinding( - call, - fmt.Sprintf(`Parameter at the position %d for %q should be keyword (%s = ...).`, index+1, name, value), - LinterReplacement{&call.List[index], makeKeyword(call.List[index], value)})) + // 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. + + if sort.IntsAreSorted(paramTypes) { + callFindings[0].Replacement = callReplacements } + + findings = append(findings, callFindings...) }) return findings diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index 8ee273fd0..3bca8f100 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -652,7 +652,7 @@ def macro(): } func TestKeywordParameters(t *testing.T) { - checkFindingsAndFix(t, "keyword-parameters", ` + checkFindingsAndFix(t, "keyword-positional-params", ` foo(key = value) all(elements = [True, False]) any(elements = [True, False]) @@ -669,10 +669,6 @@ type(x = foo) hasattr(x = foo, name = "bar") getattr(x = foo, name = "bar", default = "baz") select(x = {}) -glob(["*.cc"], ["test*"]) -native.glob(["*.cc"], ["test*"]) -glob(["*.cc"]) -native.glob(["*.cc"]) `, ` foo(key = value) all([True, False]) @@ -690,10 +686,6 @@ type(foo) hasattr(foo, name = "bar") getattr(foo, name = "bar", default = "baz") select({}) -glob(["*.cc"], exclude = ["test*"]) -native.glob(["*.cc"], exclude = ["test*"]) -glob(["*.cc"]) -native.glob(["*.cc"]) `, []string{ `:2: Keyword parameter "elements" for "all" should be positional.`, `:3: Keyword parameter "elements" for "any" should be positional.`, @@ -710,7 +702,47 @@ native.glob(["*.cc"]) `:14: Keyword parameter "x" for "hasattr" should be positional.`, `:15: Keyword parameter "x" for "getattr" should be positional.`, `:16: Keyword parameter "x" for "select" should be positional.`, - `:17: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`, - `:18: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`, + }, scopeEverywhere) + + checkFindingsAndFix(t, "keyword-positional-params", ` +glob(["*.cc"], ["test*"]) +glob(["*.cc"]) +glob(include = [], exclude = []) +glob([], exclude = []) +glob([], [], 1) +glob(*args, []) +`, ` +glob(["*.cc"], exclude = ["test*"]) +glob(["*.cc"]) +glob([], exclude = []) +glob([], exclude = []) +glob([], [], 1) +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 = ...)`, + `:6: 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(*args, []) +`, ` +native.glob(["*.cc"], exclude = ["test*"]) +native.glob(["*.cc"]) +native.glob([], exclude = []) +native.glob([], exclude = []) +native.glob([], [], 1) +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 = ...)`, + `:6: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`, }, scopeEverywhere) } From 1eb46262a0b12aec86c6573d3e527c0c2f7c5962 Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Thu, 8 Aug 2019 14:11:39 +0200 Subject: [PATCH 5/6] Update the fix for glob parameters --- warn/warn_bazel_api.go | 22 ++++++++++++++++++---- warn/warn_bazel_api_test.go | 14 ++++++++++++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index d4db76b85..1c06789fb 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -853,7 +853,7 @@ var signatures = map[string]signature{ "hasattr": {[]string{"x"}, []string{}}, "getattr": {[]string{"x"}, []string{}}, "select": {[]string{"x"}, []string{}}, - "glob": {[]string{"include"}, []string{"exclude"}}, + "glob": {[]string{"include"}, []string{"exclude", "exclude_directories"}}, } // functionName returns the name of the given function if it's a direct function call (e.g. @@ -907,8 +907,8 @@ func keywordPositionalParametersWarning(f *build.File) []*LinterFinding { var findings []*LinterFinding // Check for legacy typeKeyword parameters - build.Walk(f, func(expr build.Expr, stack []build.Expr) { - call, ok := expr.(*build.CallExpr) + build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) { + call, ok := (*expr).(*build.CallExpr) if !ok || len(call.List) == 0 { return } @@ -960,9 +960,23 @@ func keywordPositionalParametersWarning(f *build.File) []*LinterFinding { // 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) { - callFindings[0].Replacement = callReplacements + // 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...) diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index 3bca8f100..956591e92 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -710,19 +710,24 @@ 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([], [], 1) +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", ` @@ -731,18 +736,23 @@ 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([], [], 1) +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) } From fe14be0e63262168393387d0f7c87250c44be5d2 Mon Sep 17 00:00:00 2001 From: Vladimir Moskva Date: Thu, 8 Aug 2019 14:19:16 +0200 Subject: [PATCH 6/6] Update the fix for hasattr and getattr parameters --- warn/warn_bazel_api.go | 4 ++-- warn/warn_bazel_api_test.go | 46 +++++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index 1c06789fb..29be0d907 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -850,8 +850,8 @@ var signatures = map[string]signature{ "int": {[]string{"x"}, []string{}}, "dir": {[]string{"x"}, []string{}}, "type": {[]string{"x"}, []string{}}, - "hasattr": {[]string{"x"}, []string{}}, - "getattr": {[]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"}}, } diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index 956591e92..181a83f7b 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -666,8 +666,6 @@ int(x = "3") int(x = "13", base = 8) dir(x = foo) type(x = foo) -hasattr(x = foo, name = "bar") -getattr(x = foo, name = "bar", default = "baz") select(x = {}) `, ` foo(key = value) @@ -683,8 +681,6 @@ int("3") int("13", base = 8) dir(foo) type(foo) -hasattr(foo, name = "bar") -getattr(foo, name = "bar", default = "baz") select({}) `, []string{ `:2: Keyword parameter "elements" for "all" should be positional.`, @@ -699,9 +695,45 @@ select({}) `: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 "hasattr" should be positional.`, - `:15: Keyword parameter "x" for "getattr" should be positional.`, - `:16: Keyword parameter "x" for "select" 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", `