Require string literals for format strings (sorbet#1603)

* Get started on making formatting error not a thing * Add an include * Use a literal * Use a literal and a fmt::format
deborasetton · Aug 15, 2019 · 09fc625 · 09fc625
1 parent e098f59
commit 09fc625
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 26 deletions.
diff --git a/common/ConstExprStr.h b/common/ConstExprStr.h
@@ -0,0 +1,20 @@
+#ifndef SORBET_CONSTEXPRSTR_H
+#define SORBET_CONSTEXPRSTR_H
+#include <cstddef> // std::size_t
+
+namespace sorbet {
+struct ConstExprStr {
+    char const *str;
+    std::size_t size;
+
+    // can only construct from a char[] literal
+    template <std::size_t N>
+    constexpr ConstExprStr(char const (&s)[N])
+        : str(s), size(N - 1) // not count the trailing null
+    {}
+
+    ConstExprStr() = delete;
+};
+}; // namespace sorbet
+
+#endif
diff --git a/common/Counters.h b/common/Counters.h
@@ -1,5 +1,6 @@
 #ifndef SORBET_COUNTERS_H
 #define SORBET_COUNTERS_H
+#include "common/ConstExprStr.h"
 #include "common/common.h"
 #include <chrono>
 #include <string>
@@ -28,19 +29,6 @@ constexpr bool enable_counters = debug_mode;
 // units (but not necessarily between translation units), but this can't be
 // relied upon, so we canonicalize strings when retrieving statistics.
 
-struct ConstExprStr {
-    char const *str;
-    std::size_t size;
-
-    // can only construct from a char[] literal
-    template <std::size_t N>
-    constexpr ConstExprStr(char const (&s)[N])
-        : str(s), size(N - 1) // not count the trailing null
-    {}
-
-    ConstExprStr() = delete;
-};
-
 struct CounterImpl;
 
 // forward declarations for classes that need private access to the counter

diff --git a/core/Error.h b/core/Error.h
@@ -1,6 +1,6 @@
 #ifndef SORBET_REPORTER_H
 #define SORBET_REPORTER_H
-
+#include "common/ConstExprStr.h"
 #include "core/AutocorrectSuggestion.h"
 #include "core/Loc.h"
 #include "core/StrictLevel.h"
@@ -146,20 +146,20 @@ class ErrorBuilder {
         return state == State::WillBuild;
     }
     void addErrorSection(ErrorSection &&section);
-    template <typename... Args> void addErrorLine(Loc loc, std::string_view msg, const Args &... args) {
-        std::string formatted = ErrorColors::format(msg, args...);
+    template <typename... Args> void addErrorLine(Loc loc, ConstExprStr msg, const Args &... args) {
+        std::string formatted = ErrorColors::format(msg.str, args...);
         addErrorSection(ErrorSection({ErrorLine(loc, formatted)}));
     }
 
-    template <typename... Args> void setHeader(std::string_view msg, const Args &... args) {
-        std::string formatted = ErrorColors::format(msg, args...);
+    template <typename... Args> void setHeader(ConstExprStr msg, const Args &... args) {
+        std::string formatted = ErrorColors::format(msg.str, args...);
         _setHeader(move(formatted));
     }
 
     void addAutocorrect(AutocorrectSuggestion &&autocorrect);
     template <typename... Args>
-    void replaceWith(const std::string &title, Loc loc, std::string_view replacement, const Args &... args) {
-        std::string formatted = fmt::format(replacement, args...);
+    void replaceWith(const std::string &title, Loc loc, ConstExprStr replacement, const Args &... args) {
+        std::string formatted = fmt::format(replacement.str, args...);
         addAutocorrect(AutocorrectSuggestion{title, {AutocorrectSuggestion::Edit{loc, move(formatted)}}});
     }
 

diff --git a/core/types/calls.cc b/core/types/calls.cc
@@ -943,7 +943,7 @@ class T_must : public IntrinsicMethod {
             if (auto e = ctx.state.beginError(loc, errors::Infer::InvalidCast)) {
                 e.setHeader("T.must(): Expected a `T.nilable` type, got: `{}`", args.args[0]->type->show(ctx));
                 const auto locWithoutTMust = Loc{loc.file(), loc.beginPos() + 7, loc.endPos() - 1};
-                e.replaceWith("Remove the T.must", loc, locWithoutTMust.source(ctx));
+                e.replaceWith("Remove the T.must", loc, "{}", locWithoutTMust.source(ctx));
             }
         }
         res.returnType = move(ret);

diff --git a/main/options/options.h b/main/options/options.h
@@ -1,5 +1,6 @@
 #ifndef RUBY_TYPER_OPTIONS_H
 #define RUBY_TYPER_OPTIONS_H
+#include "common/ConstExprStr.h"
 #include "common/FileSystem.h"
 #include "common/common.h"
 #include "core/StrictLevel.h"
@@ -22,8 +23,8 @@ class PrinterConfig {
     bool supportsFlush = false;
 
     void print(const std::string_view &contents) const;
-    template <typename... Args> void fmt(const std::string &msg, const Args &... args) const {
-        print(fmt::format(msg, args...));
+    template <typename... Args> void fmt(const ConstExprStr msg, const Args &... args) const {
+        print(fmt::format(msg.str, args...));
     }
     void flush();
 

diff --git a/parser/Parser.cc b/parser/Parser.cc
@@ -45,12 +45,10 @@ class ErrorToError {
                 default:
                     Exception::notImplemented();
             }
-            string msg("Parse {}: ");
-            msg.append(dclassStrings[(int)diag.error_class()]);
             core::Loc loc(file, translatePos(diag.location().beginPos, maxOff - 1),
                           translatePos(diag.location().endPos, maxOff));
             if (auto e = gs.beginError(loc, core::errors::Parser::ParserError)) {
-                e.setHeader(msg, level, diag.data());
+                e.setHeader("Parse {}: {}", level, fmt::format(dclassStrings[(int)diag.error_class()], diag.data()));
             }
         }
     }