Skylint: improve message for missing return and unitialized variables

RELNOTES: none
PiperOrigin-RevId: 174310059

4 files changed, 45 insertions, 24 deletions

diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
index 63af0dff75..b7f94e9516 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
@@ -163,7 +163,12 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
       issues.add(
           Issue.create(
               MISSING_RETURN_VALUE_CATEGORY,
-              "some but not all execution paths of '" + node.getIdentifier() + "' return a value",
+              "some but not all execution paths of '"
+                  + node.getIdentifier()
+                  + "' return a value."
+                  + " If you know these cannot happen,"
+                  + " add the statement `fail('unreachable')` to them."
+                  + " For more details, have a look at the documentation.",
               node.getLocation()));
       for (Wrapper<ReturnStatement> returnWrapper : cfi.returnStatementsWithoutValue) {
         issues.add(
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
index 0c62e73bcd..d38932e2e3 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/UsageChecker.java
@@ -236,7 +236,13 @@ public class UsageChecker extends AstVisitorWithNameResolution {
       issues.add(
           Issue.create(
               UNINITIALIZED_VARIABLE_CATEGORY,
-              "variable '" + info.name + "' may not have been initialized",
+              "variable '"
+                  + info.name
+                  + "' may not have been initialized."
+                  + " If you believe this is wrong, you can add `fail('unreachable')"
+                  + " to the branches where it is not initialized"
+                  + " or initialize it with `None` at the beginning."
+                  + " For more details, have a look at the documentation.",
               node.getLocation()));
     }
   }
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
index e104d5d508..16099a7184 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
@@ -52,8 +52,10 @@ public class ControlFlowCheckerTest {
                     "    return x")
                 .toString())
         .contains(
-            "1:1-5:12: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+            "1:1-5:12: some but not all execution paths of 'some_function' return a value."
+                + " If you know these cannot happen,"
+                + " add the statement `fail('unreachable')` to them."
+                + " For more details, have a look at the documentation. [missing-return-value]");
   }
 
   @Test
@@ -68,8 +70,10 @@ public class ControlFlowCheckerTest {
             .toString();
     Truth.assertThat(messages)
         .contains(
-            "1:1-5:10: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+            "1:1-5:10: some but not all execution paths of 'some_function' return a value."
+                + " If you know these cannot happen,"
+                + " add the statement `fail('unreachable')` to them."
+                + " For more details, have a look at the documentation. [missing-return-value]");
     Truth.assertThat(messages)
         .contains(
             "5:5-5:10: return value missing (you can `return None` if this is desired)"
@@ -89,8 +93,10 @@ public class ControlFlowCheckerTest {
                     "    return not x")
                 .toString())
         .contains(
-            "1:1-7:16: some but not all execution paths of 'f' return a value"
-                + " [missing-return-value]");
+            "1:1-7:16: some but not all execution paths of 'f' return a value."
+                + " If you know these cannot happen,"
+                + " add the statement `fail('unreachable')` to them."
+                + " For more details, have a look at the documentation. [missing-return-value]");
   }
 
   @Test
@@ -107,8 +113,10 @@ public class ControlFlowCheckerTest {
                     "    return x")
                 .toString())
         .contains(
-            "1:1-8:12: some but not all execution paths of 'f' return a value"
-                + " [missing-return-value]");
+            "1:1-8:12: some but not all execution paths of 'f' return a value."
+                + " If you know these cannot happen,"
+                + " add the statement `fail('unreachable')` to them."
+                + " For more details, have a look at the documentation. [missing-return-value]");
   }
 
   @Test
@@ -121,9 +129,9 @@ public class ControlFlowCheckerTest {
                     "  elif not x:",
                     "    return not x")
                 .toString())
-        .contains(
-            "1:1-5:16: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+        .containsMatch(
+            "1:1-5:16: some but not all execution paths of 'some_function' return a value."
+                + " .+ \\[missing-return-value\\]");
   }
 
   @Test
@@ -136,9 +144,9 @@ public class ControlFlowCheckerTest {
                     "  print('foo')",
                     "  # return missing here")
                 .toString())
-        .contains(
-            "1:1-4:14: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+        .containsMatch(
+            "1:1-4:14: some but not all execution paths of 'some_function' return a value."
+                + " .+ \\[missing-return-value\\]");
   }
 
   @Test
@@ -151,9 +159,9 @@ public class ControlFlowCheckerTest {
                     "  print('foo')",
                     "  # return missing here")
                 .toString())
-        .contains(
-            "1:1-4:14: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+        .containsMatch(
+            "1:1-4:14: some but not all execution paths of 'some_function' return a value."
+                + " .+ \\[missing-return-value\\]");
   }
 
   @Test
@@ -166,9 +174,9 @@ public class ControlFlowCheckerTest {
                 "  return x")
             .toString();
     Truth.assertThat(messages)
-        .contains(
-            "1:1-4:10: some but not all execution paths of 'some_function' return a value"
-                + " [missing-return-value]");
+        .containsMatch(
+            "1:1-4:10: some but not all execution paths of 'some_function' return a value."
+                + " .+ \\[missing-return-value\\]");
     Truth.assertThat(messages)
         .contains(
             "3:5-3:10: return value missing (you can `return None` if this is desired)"
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java
index 3acf1d93d7..aebc9650dd 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/UsageCheckerTest.java
@@ -158,7 +158,8 @@ public class UsageCheckerTest {
                 "  print(y)")
             .toString();
     Truth.assertThat(message)
-        .contains("8:3-8:3: variable 'y' may not have been initialized [uninitialized-variable]");
+        .containsMatch(
+            "8:3-8:3: variable 'y' may not have been initialized. .+ \\[uninitialized-variable\\]");
   }
 
   @Test
@@ -166,7 +167,8 @@ public class UsageCheckerTest {
     String message =
         findIssues("def some_function():", "  for _ in []:", "    y = 1", "  print(y)").toString();
     Truth.assertThat(message)
-        .contains("4:9-4:9: variable 'y' may not have been initialized [uninitialized-variable]");
+        .containsMatch(
+            "4:9-4:9: variable 'y' may not have been initialized. .+ \\[uninitialized-variable\\]");
   }
 
   @Test