Remove pending-error support from the ErrorReporter.

Pending errors were used to associate C++ file and line numbers with
C++ DSL code. (We would detect errors in expressions before they had
been associated with a Position, and needed to defer saving the error
until a valid Position had been found.)

We no longer try to assign a position to C++ DSL code, so pending
errors do not do anything useful.

Change-Id: I272a73f64e63269120a6f76f4a0153c11d98fb47
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/543018
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>

include/sksl/SkSLErrorReporter.h

@@ -10,9 +10,7 @@
 
 #include "include/core/SkTypes.h"
 
-#include <string>
 #include <string_view>
-#include <vector>
 
 namespace SkSL {
 
@@ -25,24 +23,14 @@ class ErrorReporter {
 public:
     ErrorReporter() {}
 
-    virtual ~ErrorReporter() {
-        SkASSERT(fPendingErrors.empty());
-    }
+    virtual ~ErrorReporter() {}
 
-    void error(std::string_view msg, Position position);
-
-    /**
-     * Reports an error message at the given line of the source text. Errors reported
-     * with a line of -1 will be queued until line number information can be determined.
-     */
-    void error(Position pos, std::string_view msg);
+    void error(Position position, std::string_view msg);
 
     std::string_view source() const { return fSource; }
 
     void setSource(std::string_view source) { fSource = source; }
 
-    void reportPendingErrors(Position pos);
-
     int errorCount() const {
         return fErrorCount;
     }
@@ -61,7 +49,6 @@ private:
     Position position(int offset) const;
 
     std::string_view fSource;
-    std::vector<std::string> fPendingErrors;
     int fErrorCount = 0;
 };
 

src/sksl/SkSLCompiler.cpp

@@ -722,7 +722,6 @@ static bool validate_spirv(ErrorReporter& reporter, std::string_view program) {
             errors.append(disassembly);
         }
         reporter.error(Position(), errors);
-        reporter.reportPendingErrors(Position());
 #else
         SkDEBUGFAILF("%s", errors.c_str());
 #endif
@@ -833,7 +832,6 @@ static bool validate_wgsl(ErrorReporter& reporter, const std::string& wgsl) {
         std::string diagOutput = diagFormatter.format(program.Diagnostics());
 #if defined(SKSL_STANDALONE)
         reporter.error(Position(), diagOutput);
-        reporter.reportPendingErrors(Position());
 #else
         SkDEBUGFAILF("%s", diagOutput.c_str());
 #endif

src/sksl/SkSLDSLParser.cpp

@@ -240,12 +240,12 @@ Position DSLParser::position(Token t) {
     }
 }
 
-void DSLParser::error(Token token, std::string msg) {
+void DSLParser::error(Token token, std::string_view msg) {
     this->error(this->position(token), msg);
 }
 
-void DSLParser::error(Position position, std::string msg) {
-    GetErrorReporter().error(msg.c_str(), position);
+void DSLParser::error(Position position, std::string_view msg) {
+    GetErrorReporter().error(position, msg);
 }
 
 Position DSLParser::rangeFrom(Position start) {

src/sksl/SkSLDSLParser.h

@@ -117,8 +117,8 @@ private:
      */
     bool expectIdentifier(Token* result);
 
-    void error(Token token, std::string msg);
-    void error(Position position, std::string msg);
+    void error(Token token, std::string_view msg);
+    void error(Position position, std::string_view msg);
 
     // Returns the range from `start` to the current parse position.
     Position rangeFrom(Position start);
@@ -309,7 +309,7 @@ private:
 
             void forwardErrors() {
                 for (Error& error : fErrors) {
-                    dsl::GetErrorReporter().error(error.fMsg.c_str(), error.fPos);
+                    dsl::GetErrorReporter().error(error.fPos, error.fMsg);
                 }
             }
 
@@ -324,7 +324,6 @@ private:
 
         void restoreErrorReporter() {
             SkASSERT(fOldErrorReporter);
-            fErrorReporter.reportPendingErrors(Position());
             dsl::SetErrorReporter(fOldErrorReporter);
             fOldErrorReporter = nullptr;
         }

src/sksl/SkSLErrorReporter.cpp

@@ -13,35 +13,15 @@
 
 namespace SkSL {
 
-void ErrorReporter::error(std::string_view msg, Position position) {
+void ErrorReporter::error(Position position, std::string_view msg) {
     if (skstd::contains(msg, Compiler::POISON_TAG)) {
-        // don't report errors on poison values
+        // Don't report errors on poison values.
         return;
     }
     ++fErrorCount;
     this->handleError(msg, position);
 }
 
-void ErrorReporter::error(Position pos, std::string_view msg) {
-    if (skstd::contains(msg, Compiler::POISON_TAG)) {
-        // don't report errors on poison values
-        return;
-    }
-    if (pos.valid()) {
-        this->error(msg, pos);
-    } else {
-        ++fErrorCount;
-        fPendingErrors.push_back(std::string(msg));
-    }
-}
-
-void ErrorReporter::reportPendingErrors(Position pos) {
-    for (const std::string& msg : fPendingErrors) {
-        this->handleError(msg, pos);
-    }
-    fPendingErrors.clear();
-}
-
 void TestingOnly_AbortErrorReporter::handleError(std::string_view msg, Position pos) {
     SK_ABORT("%.*s", (int)msg.length(), msg.data());
 }

src/sksl/SkSLThreadContext.cpp

@@ -110,7 +110,7 @@ void ThreadContext::SetErrorReporter(ErrorReporter* errorReporter) {
 }
 
 void ThreadContext::ReportError(std::string_view msg, Position pos) {
-    GetErrorReporter().error(msg, pos);
+    GetErrorReporter().error(pos, msg);
 }
 
 void ThreadContext::DefaultErrorReporter::handleError(std::string_view msg, Position pos) {
@@ -118,10 +118,6 @@ void ThreadContext::DefaultErrorReporter::handleError(std::string_view msg, Posi
              (int)msg.length(), msg.data());
 }
 
-void ThreadContext::ReportErrors(Position pos) {
-    GetErrorReporter().reportPendingErrors(pos);
-}
-
 thread_local ThreadContext* instance = nullptr;
 
 bool ThreadContext::IsActive() {

src/sksl/SkSLThreadContext.h

@@ -151,11 +151,6 @@ public:
      */
     static void ReportError(std::string_view msg, Position pos = {});
 
-    /**
-     * Forwards any pending errors to the DSL ErrorReporter.
-     */
-    static void ReportErrors(Position pos);
-
     static ThreadContext& Instance();
 
     static void SetInstance(std::unique_ptr<ThreadContext> instance);

src/sksl/analysis/SkSLNoOpErrorReporter.h

@@ -15,7 +15,6 @@ namespace SkSL {
 // We can use a no-op error reporter to silently ignore errors.
 class NoOpErrorReporter : public ErrorReporter {
 public:
-    ~NoOpErrorReporter() override { this->reportPendingErrors({}); }
     void handleError(std::string_view, Position) override {}
 };
 

src/sksl/codegen/SkSLMetalCodeGenerator.cpp

@@ -2803,7 +2803,6 @@ bool MetalCodeGenerator::generateCode() {
     write_stringstream(fExtraFunctionPrototypes, *fOut);
     write_stringstream(fExtraFunctions, *fOut);
     write_stringstream(body, *fOut);
-    fContext.fErrors->reportPendingErrors(Position());
     return fContext.fErrors->errorCount() == 0;
 }
 

src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp

@@ -4031,7 +4031,6 @@ bool SPIRVCodeGenerator::generateCode() {
     this->writeWord(fIdCount, *fOut);
     this->writeWord(0, *fOut); // reserved, always zero
     write_stringstream(buffer, *fOut);
-    fContext.fErrors->reportPendingErrors(Position());
     return fContext.fErrors->errorCount() == 0;
 }
 

src/sksl/codegen/SkSLWGSLCodeGenerator.cpp

@@ -429,7 +429,6 @@ bool WGSLCodeGenerator::generateCode() {
 
     write_stringstream(header, *fOut);
     write_stringstream(body, *fOut);
-    fContext.fErrors->reportPendingErrors(Position());
     return fContext.fErrors->errorCount() == 0;
 }
 

src/sksl/dsl/DSLCore.cpp

@@ -16,7 +16,6 @@
 #include "include/sksl/DSLSymbols.h"
 #include "include/sksl/DSLType.h"
 #include "include/sksl/DSLVar.h"
-#include "include/sksl/SkSLErrorReporter.h"
 #include "include/sksl/SkSLPosition.h"
 #include "src/sksl/SkSLBuiltinTypes.h"
 #include "src/sksl/SkSLCompiler.h"
@@ -115,9 +114,6 @@ public:
             // We have a successful program!
             success = true;
         }
-        if (!success) {
-            ThreadContext::ReportErrors(Position());
-        }
         if (pool) {
             pool->detachFromThread();
         }
@@ -260,7 +256,6 @@ public:
             SkSL::VarDeclaration::ErrorCheck(ThreadContext::Context(), field.fPosition,
                     field.fModifiers.fPosition, field.fModifiers.fModifiers, baseType,
                     Variable::Storage::kInterfaceBlock);
-            GetErrorReporter().reportPendingErrors(field.fPosition);
             skslFields.push_back(SkSL::Type::Field(field.fPosition, field.fModifiers.fModifiers,
                     field.fName, &field.fType.skslType()));
         }
@@ -286,7 +281,6 @@ public:
                 AddToSymbolTable(var);
             }
         }
-        GetErrorReporter().reportPendingErrors(pos);
         return var;
     }
 
@@ -390,7 +384,6 @@ DSLExpression sk_Position() {
 
 void AddExtension(std::string_view name, Position pos) {
     ThreadContext::ProgramElements().push_back(std::make_unique<SkSL::Extension>(pos, name));
-    ThreadContext::ReportErrors(pos);
 }
 
 DSLStatement Break(Position pos) {

src/sksl/dsl/DSLExpression.cpp

@@ -41,8 +41,6 @@ DSLExpression::DSLExpression(DSLExpression&& other)
 DSLExpression::DSLExpression(std::unique_ptr<SkSL::Expression> expression, Position pos)
     : fExpression(expression ? std::move(expression)
                              : SkSL::Poison::Make(pos, ThreadContext::Context())) {
-    ThreadContext::ReportErrors(pos);
-
     // If a position was passed in, it must match the expression's position.
     SkASSERTF(!pos.valid() || this->position() == pos,
               "expected expression position (%d-%d), but received (%d-%d)",

src/sksl/dsl/DSLFunction.cpp

@@ -68,7 +68,6 @@ void DSLFunction::init(DSLModifiers modifiers, const DSLType& returnType, std::s
                                                name,
                                                std::move(paramVars), returnType.fPosition,
                                                &returnType.skslType());
-    ThreadContext::ReportErrors(pos);
     if (fDecl) {
         for (size_t i = 0; i < params.size(); ++i) {
             params[i]->fVar = fDecl->parameters()[i];
@@ -113,7 +112,6 @@ void DSLFunction::define(DSLBlock block, Position pos) {
             *fDecl,
             std::move(body),
             /*builtin=*/false);
-    ThreadContext::ReportErrors(fPosition);
     fDecl->setDefinition(function.get());
     ThreadContext::ProgramElements().push_back(std::move(function));
 }

src/sksl/dsl/DSLStatement.cpp

@@ -46,7 +46,6 @@ DSLStatement::DSLStatement(std::unique_ptr<SkSL::Statement> stmt)
 
 DSLStatement::DSLStatement(std::unique_ptr<SkSL::Statement> stmt, Position pos)
         : fStatement(stmt ? std::move(stmt) : SkSL::Nop::Make()) {
-    ThreadContext::ReportErrors(pos);
     if (pos.valid() && !fStatement->fPosition.valid()) {
         fStatement->fPosition = pos;
     }

src/sksl/dsl/DSLSymbols.cpp

@@ -58,7 +58,6 @@ void AddToSymbolTable(DSLVarBase& var, Position pos) {
     if (skslVar) {
         CurrentSymbolTable()->addWithoutOwnership(skslVar);
     }
-    ThreadContext::ReportErrors(pos);
 }
 
 } // namespace dsl

src/sksl/dsl/DSLType.cpp

@@ -40,12 +40,11 @@ static const SkSL::Type* verify_type(const Context& context,
                                      Position pos) {
     if (!context.fConfig->fIsBuiltinCode) {
         if (!allowPrivateTypes && type->isPrivate()) {
-            context.fErrors->error("type '" + std::string(type->name()) + "' is private", pos);
+            context.fErrors->error(pos, "type '" + std::string(type->name()) + "' is private");
             return context.fTypes.fPoison.get();
         }
         if (!type->isAllowedInES2(context)) {
-            context.fErrors->error("type '" + std::string(type->name()) + "' is not supported",
-                                   pos);
+            context.fErrors->error(pos, "type '" + std::string(type->name()) +"' is not supported");
             return context.fTypes.fPoison.get();
         }
     }
@@ -57,13 +56,13 @@ static const SkSL::Type* find_type(const Context& context,
                                    std::string_view name) {
     const Symbol* symbol = (*ThreadContext::SymbolTable())[name];
     if (!symbol) {
-        context.fErrors->error(String::printf("no symbol named '%.*s'",
-                                              (int)name.length(), name.data()), pos);
+        context.fErrors->error(pos, String::printf("no symbol named '%.*s'",
+                                                   (int)name.length(), name.data()));
         return context.fTypes.fPoison.get();
     }
     if (!symbol->is<SkSL::Type>()) {
-        context.fErrors->error(String::printf("symbol '%.*s' is not a type",
-                                              (int)name.length(), name.data()), pos);
+        context.fErrors->error(pos, String::printf("symbol '%.*s' is not a type",
+                                                   (int)name.length(), name.data()));
         return context.fTypes.fPoison.get();
     }
     const SkSL::Type* type = &symbol->as<SkSL::Type>();
@@ -78,7 +77,6 @@ static const SkSL::Type* find_type(const Context& context,
     const Type* type = find_type(context, overallPos, name);
     type = type->applyPrecisionQualifiers(context, modifiers, ThreadContext::SymbolTable().get(),
             modifiersPos);
-    ThreadContext::ReportErrors(overallPos);
     return type;
 }
 
@@ -276,7 +274,6 @@ DSLExpression DSLType::Construct(DSLType type, SkSpan<DSLExpression> argArray) {
 DSLType Array(const DSLType& base, int count, Position pos) {
     count = base.skslType().convertArraySize(ThreadContext::Context(), pos,
             DSLExpression(count, pos).release());
-    ThreadContext::ReportErrors(pos);
     if (!count) {
         return DSLType(kPoison_Type);
     }

src/sksl/dsl/priv/DSLWriter.cpp

@@ -62,7 +62,6 @@ const SkSL::Variable* DSLWriter::Var(DSLVarBase& var) {
                 var.fInitialized = true;
             }
         }
-        ThreadContext::ReportErrors(var.fPosition);
     }
     return var.fVar;
 }