Repository: bazelbuild/buildtools (go)

Issue:
unsorted-dict-items suggests strange ordering for 'exports'
Am I missing something with regards to the suggested ordering warned by `unsorted-dict-items`?

In a list of attributes, buildifier suggests the following ordering:

```
        "dynamic_deps"
        "linkopts"
        "preloaded_deps"
        "visibility_file"
        "exports"
        "_cc_toolchain"
```

`exports` seems out of place, here. If we rename it to `exportsfoo`, buildifier suggests a sensible ordering (`exportsfoo` should go before `linkopts`)

IIUC, the [documentation](https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#dictionary-items-should-be-ordered-by-their-keys) for this warning only remarks potentially overriding the ordering.

Pull request:
Don't use tables.NamePriority for sorting dict items
Rule attributes are sorted lexicographically but with a few exceptions, e.g. "name" should be the first item, "srcs" should be placed before "deps", etc.

The same list of exceptions (https://github.com/bazelbuild/buildtools/blob/master/tables/tables.go#L108) was used for sorting dict keys, however that's often confusing: if a dict's items don't represent rule attributes, it's not clear why, for example, the following ordering is correct: `{"name": ..., "first_name"; ..., "last_name": ...}`

The pull request removes the exceptions for dict keys, except for "//conditions:default" which is often used in dicts in select statements and should be the last by convention.

Fixes #760

Patch:
diff --git a/tables/tables.go b/tables/tables.go
index 05bda4c..5152576 100644
--- a/tables/tables.go
+++ b/tables/tables.go
@@ -199,9 +199,6 @@ var NamePriority = map[string]int{
 	"implementation": 5,
 	"implements":     6,
 	"alwayslink":     7,
-	// default condition in a dictionary literal passed to select should be
-	// the last one by convention.
-	"//conditions:default": 50,
 }
 
 var StripLabelLeadingSlashes = false
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index 4172f36..6da01d6 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/bazelbuild/buildtools/build"
 	"github.com/bazelbuild/buildtools/edit"
-	"github.com/bazelbuild/buildtools/tables"
 )
 
 func sameOriginLoadWarning(f *build.File) []*LinterFinding {
@@ -289,11 +288,15 @@ func unsortedDictItemsWarning(f *build.File) []*LinterFinding {
 		if strings.HasPrefix(key2, "_") {
 			return true
 		}
-		key1Priority := tables.NamePriority[key1]
-		key2Priority := tables.NamePriority[key2]
-		if key1Priority != key2Priority {
-			return key1Priority < key2Priority
+
+		// "//conditions:default" should always be the last
+		const conditionsDefault = "//conditions:default"
+		if key1 == conditionsDefault {
+			return false
+		} else if key2 == conditionsDefault {
+			return true
 		}
+
 		return key1 < key2
 	}
 


Test patch:
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index 485a947..b210539 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -372,12 +372,12 @@ d = {
 
 	checkFindingsAndFix(t, "unsorted-dict-items", `
 d = {
-	"deps": [],
 	"srcs": ["foo.go"],
+	"deps": [],
 }`, `
 d = {
-	"srcs": ["foo.go"],
 	"deps": [],
+	"srcs": ["foo.go"],
 }`,
 		[]string{"3: Dictionary items are out of their lexicographical order."},
 		scopeEverywhere)
@@ -409,15 +409,18 @@ foo_binary = rule(
 foo_binary = rule(
 	implementation = _foo_binary_impl,
 	attrs = {
-		"srcs": attr.label_list(allow_files = True),
 		"deps": attr.label_list(),
+		"srcs": attr.label_list(allow_files = True),
 		"_foocc": attr.label(
 			default = Label("//depsets:foocc"),
 		),
 	},
 	outputs = {"out": "%{name}.out"},
 )`,
-		[]string{"7: Dictionary items are out of their lexicographical order."},
+		[]string{
+			"7: Dictionary items are out of their lexicographical order.",
+			"8: Dictionary items are out of their lexicographical order.",
+		},
 		scopeEverywhere)
 
 	checkFindings(t, "unsorted-dict-items", `