Re-order diagnostics to make it more readable

Author: prymitive

internal/checks/promql_impossible_test.go

@@ -417,6 +417,31 @@ func TestImpossibleCheck(t *testing.T) {
     sum by (job) (
       up{env="prod", job="foo"} * on () group_left(job) services_enabled{job=""}
     )
+`,
+			checker:    newImpossibleCheck,
+			prometheus: newSimpleProm,
+			problems:   true,
+		},
+		{
+			description: "join on possible label",
+			content: `
+- record: foo
+  expr: |
+    sum by (job) (
+      up{env="prod"} * on () group_left(job) services_enabled{job!=""}
+    )
+`,
+			checker:    newImpossibleCheck,
+			prometheus: newSimpleProm,
+		},
+		{
+			description: "join on possible but redundant label",
+			content: `
+- record: foo
+  expr: |
+    sum by (job) (
+      up{env="prod", job="foo"} * on () group_left(job) services_enabled{job!=""}
+    )
 `,
 			checker:    newImpossibleCheck,
 			prometheus: newSimpleProm,

internal/checks/promql_impossible_test.snap

@@ -1640,8 +1640,8 @@
       expr: foo / on(instance) group_left(cluster) sum(bar)
   output: |
     3 |   expr: foo / on(instance) group_left(cluster) sum(bar)
-                                              ^^^^^^^ You can't use `cluster` because this label is not possible here.
                                                        ^^^ Query is using aggregation that removes all labels.
+                                              ^^^^^^^ You can't use `cluster` because this label is not possible here.
   problem:
     reporter: promql/impossible
     summary: impossible label
@@ -2363,8 +2363,8 @@
   output: |
     4 |     sum by (job) (
     5 |       up{env="prod", job="foo"} * on () group_left(job) services_enabled{job=""}
-                                                           ^^^ You can't use `job` because this label is not possible here.
                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^ Query uses `{job=""}` selector which will filter out any time series with the `job` label set.
+                                                           ^^^ You can't use `job` because this label is not possible here.
     6 |     )
   problem:
     reporter: promql/impossible
@@ -2387,6 +2387,47 @@
 
 ---
 
+[TestImpossibleCheck/join_on_possible_but_redundant_label - 1]
+- description: join on possible but redundant label
+  content: |4
+
+    - record: foo
+      expr: |
+        sum by (job) (
+          up{env="prod", job="foo"} * on () group_left(job) services_enabled{job!=""}
+        )
+  output: |
+    4 |     sum by (job) (
+    5 |       up{env="prod", job="foo"} * on () group_left(job) services_enabled{job!=""}
+                                                           ^^^ Query is trying to join the `job` label that is already present on the other side of the query.
+              ^^^^^^^^^^^^^^^^^^^^^^^^^ Query will only return series where these labels are present.
+    6 |     )
+  problem:
+    reporter: promql/impossible
+    summary: redundant label
+    details: ""
+    diagnostics:
+        - message: Query is trying to join the `job` label that is already present on the other side of the query.
+          firstcolumn: 63
+          lastcolumn: 65
+          kind: 0
+        - message: Query will only return series where these labels are present.
+          firstcolumn: 18
+          lastcolumn: 42
+          kind: 1
+    lines:
+        first: 4
+        last: 6
+    severity: 1
+    anchor: 0
+
+---
+
+[TestImpossibleCheck/join_on_possible_label - 1]
+[]
+
+---
+
 [TestImpossibleCheck/joined_label_NOT_ignored_via_ignoring() - 1]
 []
 
@@ -2403,8 +2444,8 @@
   output: |
     4 |     (prometheus_ready * on(instance) group_left(version) prometheus_build_info)
     5 |     * ignoring(version) prometheus_config_last_reload_successful
-            ^ This binary operation prevents previously joined label `version` from being added to the results.
                        ^^^^^^^ Query is using one-to-one vector matching with `ignoring(version)`, all labels included inside `ignoring(...)` will be removed on the results.
+            ^ This binary operation prevents previously joined label `version` from being added to the results.
   problem:
     reporter: promql/impossible
     summary: orphaned label

internal/diags/problems.go

@@ -1,6 +1,7 @@
 package diags
 
 import (
+	"cmp"
 	"fmt"
 	"slices"
 	"strings"
@@ -43,10 +44,17 @@ func lineCoverage(diags []Diagnostic) (lines []int) {
 	return lines
 }
 
-func InjectDiagnostics(content string, diags []Diagnostic, color output.Color) string {
+func InjectDiagnostics(content string, d []Diagnostic, color output.Color) string {
+	diags := make([]Diagnostic, len(d))
+	copy(diags, d)
+
 	lines := lineCoverage(diags)
 	lastLine := slices.Max(lines)
 
+	slices.SortStableFunc(diags, func(a, b Diagnostic) int {
+		return cmp.Compare(b.FirstColumn, a.FirstColumn)
+	})
+
 	diagPositions := make([]PositionRanges, len(diags))
 	for i, diag := range diags {
 		dl := diag.Pos.Len()

internal/diags/problems_test.go

@@ -47,8 +47,8 @@ expr: sum(foo{job="bar"})
 			},
 			output: `2 | expr: sum(foo{job="bar"})
 3 |       / on(a,b)
-            ^^^^^^^ abc
                ^^^ efg
+            ^^^^^^^ abc
 4 |       sum(foo)
 `,
 		},
@@ -191,3 +191,22 @@ func TestInjectDiagnosticsKind(t *testing.T) {
 `
 	require.Equal(t, expected, out)
 }
+
+func TestInjectDiagnosticsOrder(t *testing.T) {
+	input := "expr: foo(bar) by()"
+	diags := []Diagnostic{
+		{FirstColumn: 1, LastColumn: 13, Message: "this is bad", Kind: Issue},
+		{FirstColumn: 10, LastColumn: 13, Message: "this is context", Kind: Context},
+	}
+	key, val := parseYaml(input)
+	pos := NewPositionRange(strings.Split(input, "\n"), val, key.Column+2)
+	for i := range diags {
+		diags[i].Pos = pos
+	}
+	out := InjectDiagnostics(input, diags, output.None)
+	expected := `1 | expr: foo(bar) by()
+                   ^^^^ this is context
+          ^^^^^^^^^^^^^ this is bad
+`
+	require.Equal(t, expected, out)
+}