Ensure --fail-on always returns a non-zero exit code when issue is matched

CONTRIBUTORS.txt

@@ -512,3 +512,5 @@ contributors:
 * Daniel Dorani (doranid): contributor
 
 * Will Shanks: contributor
+
+* Mark Bell: contributor

ChangeLog

@@ -17,6 +17,11 @@ Release date: TBA
 ..
   Put bug fixes that should not wait for a new minor version here
 
+* Fix bug in which --fail-on can return a zero exit code even when the specified issue is present
+
+  Closes #4296
+  Closes #3363
+
 * Fix hard failure when handling missing attribute in a class with duplicated bases
 
   Closes #4687

pylint/lint/pylinter.py

@@ -707,7 +707,9 @@ def enable_fail_on_messages(self):
                         self.fail_on_symbols.append(msg.symbol)
 
     def any_fail_on_issues(self):
-        return any(x in self.fail_on_symbols for x in self.stats["by_msg"])
+        return self.stats is not None and any(
+            x in self.fail_on_symbols for x in self.stats["by_msg"]
+        )
 
     def disable_noerror_messages(self):
         for msgcat, msgids in self.msgs_store._msgs_by_category.items():

pylint/lint/run.py

@@ -394,14 +394,13 @@ def __init__(
         if exit:
             if linter.config.exit_zero:
                 sys.exit(0)
+            elif linter.any_fail_on_issues():
+                # We need to make sure we return a failing exit code in this case.
+                # So we use self.linter.msg_status if that is non-zero, otherwise we just return 1.
+                sys.exit(self.linter.msg_status or 1)
+            elif score_value is not None and score_value >= linter.config.fail_under:
+                sys.exit(0)
             else:
-                if (
-                    score_value
-                    and score_value >= linter.config.fail_under
-                    # detected messages flagged by --fail-on prevent non-zero exit code
-                    and not linter.any_fail_on_issues()
-                ):
-                    sys.exit(0)
                 sys.exit(self.linter.msg_status)
 
     def version_asked(self, _, __):

tests/regrtest_data/fail_on.py

@@ -0,0 +1,12 @@
+"""
+    Pylint score:  -1.67
+"""
+import nonexistent
+# pylint: disable=broad-except
+
+
+def loop():
+    count = 0
+    for _ in range(5):
+        count += 1
+    print(count)

tests/regrtest_data/fail_on_info_only.py

@@ -0,0 +1,11 @@
+"""
+    Pylint score:  -1.67
+"""
+# pylint: disable=broad-except
+
+def loop():
+    """Run a loop."""
+    count = 0
+    for _ in range(5):
+        count += 1
+    print(count)

tests/test_self.py

@@ -1136,6 +1136,44 @@ def test_output_file_invalid_path_exits_with_code_32(self):
         output_file = "thisdirectorydoesnotexit/output.txt"
         self._runtest([path, f"--output={output_file}"], code=32)
 
+    @pytest.mark.parametrize(
+        "args, expected",
+        [
+            ([], 0),
+            (["--enable=C"], 0),
+            (["--fail-on=superfluous-parens"], 0),
+            (["--fail-on=import-error"], 6),
+            (["--fail-on=unused-import"], 6),
+            (["--fail-on=unused-import", "--enable=C"], 22),
+            (["--fail-on=missing-function-docstring"], 22),
+            (["--fail-on=useless-suppression"], 6),
+            (["--fail-on=useless-suppression", "--enable=C"], 22),
+        ],
+    )
+    def test_fail_on_exit_code(self, args, expected):
+        path = join(HERE, "regrtest_data", "fail_on.py")
+        # We set fail-under to be something very low so that even with the warnings
+        # and errors that are generated they don't affect the exit code.
+        self._runtest([path, "--fail-under=-10"] + args, code=expected)
+
+    @pytest.mark.parametrize(
+        "args, expected",
+        [
+            ([], 0),
+            (["--enable=C"], 0),
+            (["--fail-on=superfluous-parens"], 0),
+            (["--fail-on=import-error"], 0),
+            (["--fail-on=unused-import"], 0),
+            (["--fail-on=unused-import", "--enable=C"], 0),
+            (["--fail-on=missing-function-docstring"], 0),
+            (["--fail-on=useless-suppression"], 1),
+            (["--fail-on=useless-suppression", "--enable=C"], 1),
+        ],
+    )
+    def test_fail_on_info_only_exit_code(self, args, expected):
+        path = join(HERE, "regrtest_data", "fail_on_info_only.py")
+        self._runtest([path] + args, code=expected)
+
     @pytest.mark.parametrize(
         "output_format, expected_output",
         [