[flang] Lift -Werror checks into local functions

[ashermancinelli]

Summary

Lift checks for -Werror into local functions.

[ashermancinelli]

Notice that in PrescanAndSemaAction, the first check fails with a scanning error, and the second with a parsing error. This is not the case for PrescanAndParseAction, which makes me think the second check should fail with a parsing error, and the tests should be adjusted in the same way.

[awarzynski]

Really nice to see this refactor, thank you!

In D101261#2715404, @ashermancinelli wrote:

Notice that in PrescanAndSemaAction, the first check fails with a scanning error, and the second with a parsing error. This is not the case for PrescanAndParseAction, which makes me think the second check should fail with a parsing error, and the tests should be adjusted in the same way.

I agree with you. Could you update it?

[awarzynski]

@ashermancinelli , I had a quick look and I believe that this is what you had in mind (with a test):

diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 1dcb6e0a16be..ba738170e1e0 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -106,7 +106,7 @@ bool PrescanAndParseAction::BeginSourceFileAction(CompilerInstance &c1) {
   // Parse. In case of failure, report and return.
   ci.parsing().Parse(llvm::outs());

-  if (reportFatalScanningErrors(this))
+  if (reportFatalParsingErrors(this))
     return false;

   // Report the diagnostics from parsing
diff --git a/flang/test/Driver/parse-error.f95 b/flang/test/Driver/parse-error.f95
index 00126047e6df..cdcde0ee47f6 100644
--- a/flang/test/Driver/parse-error.f95
+++ b/flang/test/Driver/parse-error.f95
@@ -1,5 +1,8 @@
-! Verify that parsing errors are correctly reported by the driver
+! Verify that parsing errors are correctly reported by the driver. Currently this test focuses on actions inheriting from:
+!   * PrescanAndSemaAction (-fsyntax-only)
+!   * PrescanAndParseAction (-fdebug-unparse-no-sema)

+! RUN: not %flang_fc1 -fdebug-unparse-no-sema %s 2>&1 | FileCheck %s --check-prefix=ERROR
 ! RUN: not %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=ERROR

 ! ERROR: Could not parse {{.*}}parse-error.f95

The updated test won't pass without the change in FrontendActions.cpp that you suggested earlier. Does this make sense?

[ashermancinelli]

Update revision with supporting functionality test.

[awarzynski]

LGTM

Apologies for not getting back to this earlier and thanks again for working on this!

[ashermancinelli]

Rebase to ensure patch still builds cleanly.

flang/lib/Frontend/FrontendActions.cpp

 //===--- FrontendActions.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Frontend/FrontendActions.h"
 #include "flang/Common/default-kinds.h"
 #include "flang/Frontend/CompilerInstance.h"
 #include "flang/Frontend/FrontendOptions.h"
 #include "flang/Frontend/PreprocessorOptions.h"
 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Parser/dump-parse-tree.h"
 #include "flang/Parser/parsing.h"
 #include "flang/Parser/provenance.h"
if (semantics.AnyFatalError()) {
     unsigned DiagID = diags.getCustomDiagID(
         clang::DiagnosticsEngine::Error, "Semantic errors in %0");
     diags.Report(DiagID) << bufferName;
     return true;
   }
   return false;
 }
 
-bool PrescanAction::BeginSourceFileAction(CompilerInstance &c1) {
-  CompilerInstance &ci = this->instance();
-  std::string currentInputPath{GetCurrentFileOrBufferName()};
-  Fortran::parser::Options parserOptions = ci.invocation().fortranOpts();
-
-
-  // Prescan. In case of failure, report and return.
-  ci.parsing().Prescan(currentInputPath, parserOptions);
-
+template <unsigned N>
+static bool reportFatalErrors(
+    const FrontendAction *act, const char (&message)[N]) {
+  CompilerInstance &ci = act->instance();
   if (!ci.parsing().messages().empty() &&
       (ci.invocation().warnAsErr() ||
           ci.parsing().messages().AnyFatalError())) {
     const unsigned diagID = ci.diagnostics().getCustomDiagID(
-        clang::DiagnosticsEngine::Error, "Could not scan %0");
-    ci.diagnostics().Report(diagID) << GetCurrentFileOrBufferName();
+        clang::DiagnosticsEngine::Error, message);
+    ci.diagnostics().Report(diagID) << act->GetCurrentFileOrBufferName();
     ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
-
-    return false;
-  }
+    return true;
+  }
 
-  return true;
+inline bool reportFatalScanningErrors(const FrontendAction *act) {
+  return reportFatalErrors(act, "Could not scan %0");
+}
+
+inline bool reportFatalParsingErrors(const FrontendAction *act) {
+  return reportFatalErrors(act, "Could not parse %0");
+}
+
+bool PrescanAction::BeginSourceFileAction(CompilerInstance &c1) {
+  CompilerInstance &ci = this->instance();
+  std::string currentInputPath{GetCurrentFileOrBufferName()};
+  Fortran::parser::Options parserOptions = ci.invocation().fortranOpts();
+
+  // Prescan. In case of failure, report and return.
+  ci.parsing().Prescan(currentInputPath, parserOptions);
+
+  return !reportFatalScanningErrors(this);
 }
 
 bool PrescanAndParseAction::BeginSourceFileAction(CompilerInstance &c1) {
   CompilerInstance &ci = this->instance();
 
   std::string currentInputPath{GetCurrentFileOrBufferName()};
 
   Fortran::parser::Options parserOptions = ci.invocation().fortranOpts();
 
   if (ci.invocation().frontendOpts().fortranForm_ == FortranForm::Unknown) {
     // Switch between fixed and free form format based on the input file
     // extension.
     //
     // Ideally we should have all Fortran options set before entering this
     // method (i.e. before processing any specific input files). However, we
     // can't decide between fixed and free form based on the file extension
     // earlier than this.
     parserOptions.isFixedForm = currentInput().IsFixedForm();
   }
 
   // Prescan. In case of failure, report and return.
   ci.parsing().Prescan(currentInputPath, parserOptions);
 
-  if (ci.parsing().messages().AnyFatalError()) {
-    const unsigned diagID = ci.diagnostics().getCustomDiagID(
-        clang::DiagnosticsEngine::Error, "Could not scan %0");
-    ci.diagnostics().Report(diagID) << GetCurrentFileOrBufferName();
-    ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
-
+  if (reportFatalScanningErrors(this))
     return false;
-  }
 
   // Parse. In case of failure, report and return.
   ci.parsing().Parse(llvm::outs());
 
-  if (ci.parsing().messages().AnyFatalError()) {
-    unsigned diagID = ci.diagnostics().getCustomDiagID(
-        clang::DiagnosticsEngine::Error, "Could not parse %0");
-    ci.diagnostics().Report(diagID) << GetCurrentFileOrBufferName();
-
-    ci.parsing().messages().Emit(
-        llvm::errs(), this->instance().allCookedSources());
+  if (reportFatalParsingErrors(this))
     return false;
-  }
 
   // Report the diagnostics from parsing
   ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
 
   return true;
 }
 
 bool PrescanAndSemaAction::BeginSourceFileAction(CompilerInstance &c1) {
   CompilerInstance &ci = this->instance();
   std::string currentInputPath{GetCurrentFileOrBufferName()};
   Fortran::parser::Options parserOptions = ci.invocation().fortranOpts();
 
   // Prescan. In case of failure, report and return.
   ci.parsing().Prescan(currentInputPath, parserOptions);
 
-  if (!ci.parsing().messages().empty() &&
-      (ci.invocation().warnAsErr() ||
-          ci.parsing().messages().AnyFatalError())) {
-    const unsigned diagID = ci.diagnostics().getCustomDiagID(
-        clang::DiagnosticsEngine::Error, "Could not scan %0");
-    ci.diagnostics().Report(diagID) << GetCurrentFileOrBufferName();
-    ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
-
+  if (reportFatalScanningErrors(this))
     return false;
-  }
 
   // Parse. In case of failure, report and return.
   ci.parsing().Parse(llvm::outs());
 
-  if (!ci.parsing().messages().empty() &&
-      (ci.invocation().warnAsErr() ||
-          ci.parsing().messages().AnyFatalError())) {
-    unsigned diagID = ci.diagnostics().getCustomDiagID(
-        clang::DiagnosticsEngine::Error, "Could not parse %0");
-    ci.diagnostics().Report(diagID) << GetCurrentFileOrBufferName();
-
-    ci.parsing().messages().Emit(
-        llvm::errs(), this->instance().allCookedSources());
+  if (reportFatalParsingErrors(this))
     return false;
-  }
 
   // Report the diagnostics from parsing
   ci.parsing().messages().Emit(llvm::errs(), ci.allCookedSources());
 
   auto &parseTree{*ci.parsing().parseTree()};
 
   // Prepare semantics
   setSemantics(std::make_unique<Fortran::semantics::Semantics>(

flang/test/Driver/parse-error.f95

 ! Verify that parsing errors are correctly reported by the driver
+! Focuses on actions inheriting from the following:
+! * PrescanAndSemaAction (-fsyntax-only)
+! * PrescanAndParseAction (-fdebug-unparse-no-sema)
 
+! RUN: not %flang_fc1 -fdebug-unparse-no-sema %s 2>&1 | FileCheck %s --check-prefix=ERROR
 ! RUN: not %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=ERROR
 
 ! ERROR: Could not parse {{.*}}parse-error.f95
 
 "This file will not parse"