From 660044b74fe8078966865a3f640df5bd72b9970f Mon Sep 17 00:00:00 2001 From: fzaiser Date: Thu, 2 Nov 2017 07:33:48 -0400 Subject: Skylint: improve message for missing return and unitialized variables RELNOTES: none PiperOrigin-RevId: 174310059 --- .../skylark/skylint/ControlFlowChecker.java | 7 +++- .../devtools/skylark/skylint/UsageChecker.java | 8 +++- .../skylark/skylint/ControlFlowCheckerTest.java | 48 +++++++++++++--------- .../devtools/skylark/skylint/UsageCheckerTest.java | 6 ++- 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 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 -- cgit v1.2.3