From 62cacc371f7e0d5745c28a8e1a77ddbc834df31c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 17:59:41 +0200 Subject: [PATCH 1/6] Fix BuildResult.toString() for NoSubstituters --- src/libstore/build-result.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/build-result.hh b/src/libstore/build-result.hh index e50ddbb8c..a12c599d9 100644 --- a/src/libstore/build-result.hh +++ b/src/libstore/build-result.hh @@ -53,6 +53,7 @@ struct BuildResult case LogLimitExceeded: return "LogLimitExceeded"; case NotDeterministic: return "NotDeterministic"; case ResolvesToAlreadyValid: return "ResolvesToAlreadyValid"; + case NoSubstituters: return "NoSubstituters"; default: return "Unknown"; }; }(); From ed7885017c25940f571dc58e94ca47fa84780f9d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 18:01:30 +0200 Subject: [PATCH 2/6] Fix systemd logging for lvlNotice: eqv to lvlInfo, not lvlVomit --- src/libutil/logging.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 7cac75ce1..28b385461 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -65,7 +65,7 @@ public: switch (lvl) { case lvlError: c = '3'; break; case lvlWarn: c = '4'; break; - case lvlInfo: c = '5'; break; + case lvlNotice: case lvlInfo: c = '5'; break; case lvlTalkative: case lvlChatty: c = '6'; break; default: c = '7'; } From 3dac4c7874b876dc28d522aa4eddd8b4deb64378 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 18:03:20 +0200 Subject: [PATCH 3/6] Add explicit case statements where -Wswitch-enum would report them --- src/libcmd/repl.cc | 2 ++ src/libexpr/eval.cc | 1 + src/libstore/nar-accessor.cc | 1 + src/libutil/logging.cc | 3 ++- src/nix-store/nix-store.cc | 2 +- 5 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index e3afb1531..e75ac2cf4 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -1024,6 +1024,8 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m str << v.fpoint; break; + case nThunk: + case nExternal: default: str << ANSI_RED "«unknown»" ANSI_NORMAL; break; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 22337a3ff..a5e9636ae 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2337,6 +2337,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v case nFloat: return v1.fpoint == v2.fpoint; + case nThunk: // Must not be left by forceValue default: error("cannot compare %1% with %2%", showType(v1), showType(v2)).withTrace(pos, errorCtx).debugThrow(); } diff --git a/src/libstore/nar-accessor.cc b/src/libstore/nar-accessor.cc index 9a0003588..f0dfcb19b 100644 --- a/src/libstore/nar-accessor.cc +++ b/src/libstore/nar-accessor.cc @@ -275,6 +275,7 @@ json listNar(ref accessor, const Path & path, bool recurse) obj["type"] = "symlink"; obj["target"] = accessor->readLink(path); break; + case FSAccessor::Type::tMissing: default: throw Error("path '%s' does not exist in NAR", path); } diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 28b385461..741e1d1f5 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -67,7 +67,8 @@ public: case lvlWarn: c = '4'; break; case lvlNotice: case lvlInfo: c = '5'; break; case lvlTalkative: case lvlChatty: c = '6'; break; - default: c = '7'; + case lvlDebug: case lvlVomit: c = '7'; + default: c = '7'; break; } prefix = std::string("<") + c + ">"; } diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index a62cb874f..2d784376d 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -443,7 +443,7 @@ static void opQuery(Strings opFlags, Strings opArgs) break; } - default: + default: case qDefault: abort(); } } From 9470ee877dabcf133469467a23649066e2bcce5c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 18:15:12 +0200 Subject: [PATCH 4/6] Allow open switch-enum in 5 places --- src/libexpr/eval.cc | 8 ++++++++ src/libexpr/flake/flake.cc | 4 ++++ src/libexpr/primops.cc | 4 ++++ src/libstore/filetransfer.cc | 5 +++++ 4 files changed, 21 insertions(+) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a5e9636ae..7f2065656 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -239,6 +239,9 @@ std::string_view showType(ValueType type) std::string showType(const Value & v) { + // Allow selecting a subset of enum values + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" switch (v.internalType) { case tString: return v.string.context ? "a string with context" : "a string"; case tPrimOp: @@ -252,16 +255,21 @@ std::string showType(const Value & v) default: return std::string(showType(v.type())); } + #pragma GCC diagnostic pop } PosIdx Value::determinePos(const PosIdx pos) const { + // Allow selecting a subset of enum values + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" switch (internalType) { case tAttrs: return attrs->pos; case tLambda: return lambda.fun->pos; case tApp: return app.left->determinePos(pos); default: return pos; } + #pragma GCC diagnostic pop } bool Value::isTrivial() const diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 81e94848a..7c6cd091d 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -125,6 +125,9 @@ static FlakeInput parseFlakeInput(EvalState & state, follows.insert(follows.begin(), lockRootPath.begin(), lockRootPath.end()); input.follows = follows; } else { + // Allow selecting a subset of enum values + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" switch (attr.value->type()) { case nString: attrs.emplace(state.symbols[attr.name], attr.value->string.s); @@ -139,6 +142,7 @@ static FlakeInput parseFlakeInput(EvalState & state, throw TypeError("flake input attribute '%s' is %s while a string, Boolean, or integer is expected", state.symbols[attr.name], showType(*attr.value)); } + #pragma GCC diagnostic pop } } catch (Error & e) { e.addTrace( diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 72faeada8..f1bce2fb6 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -577,6 +577,9 @@ struct CompareValues return v1->integer < v2->fpoint; if (v1->type() != v2->type()) state.error("cannot compare %s with %s", showType(*v1), showType(*v2)).debugThrow(); + // Allow selecting a subset of enum values + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" switch (v1->type()) { case nInt: return v1->integer < v2->integer; @@ -599,6 +602,7 @@ struct CompareValues } default: state.error("cannot compare %s with %s; values of that type are incomparable", showType(*v1), showType(*v2)).debugThrow(); + #pragma GCC diagnostic pop } } catch (Error & e) { if (!errorCtx.empty()) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 1ba399a29..2346accbe 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -407,6 +407,10 @@ struct curlFileTransfer : public FileTransfer err = Misc; } else { // Don't bother retrying on certain cURL errors either + + // Allow selecting a subset of enum values + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wswitch-enum" switch (code) { case CURLE_FAILED_INIT: case CURLE_URL_MALFORMAT: @@ -427,6 +431,7 @@ struct curlFileTransfer : public FileTransfer default: // Shut up warnings break; } + #pragma GCC diagnostic pop } attempt++; From fba7be80eb0fab54b570623d3991739cf3da0759 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 18:11:42 +0200 Subject: [PATCH 5/6] Enable -Werror=switch-enum switch statements must now match all enum values or disable the warning. Explicit is good. This has helped us find two bugs, after solving another one by debugging. From now on, adding to an enum will raise errors where they are not explicitly handled, which is good for productivity, and helps us decide the correct behavior in all usages. Notably still excluded from this though are the cases where the warning is disabled by local pragmas. fromTOML.cc did not build despite a top-level pragma, so I've had to resort to a makefile solution for that. --- local.mk | 4 +++- mk/patterns.mk | 4 ++-- src/libexpr/local.mk | 2 ++ src/libutil/logging.cc | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/local.mk b/local.mk index 6a7074e8e..6951c179e 100644 --- a/local.mk +++ b/local.mk @@ -1,6 +1,8 @@ clean-files += Makefile.config -GLOBAL_CXXFLAGS += -Wno-deprecated-declarations +GLOBAL_CXXFLAGS += -Wno-deprecated-declarations -Werror=switch +# Allow switch-enum to be overridden for files that do not support it, usually because of dependency headers. +ERROR_SWITCH_ENUM = -Werror=switch-enum $(foreach i, config.h $(wildcard src/lib*/*.hh), \ $(eval $(call install-file-in, $(i), $(includedir)/nix, 0644))) diff --git a/mk/patterns.mk b/mk/patterns.mk index 86a724806..c81150260 100644 --- a/mk/patterns.mk +++ b/mk/patterns.mk @@ -1,10 +1,10 @@ $(buildprefix)%.o: %.cc @mkdir -p "$(dir $@)" - $(trace-cxx) $(CXX) -o $@ -c $< $(CPPFLAGS) $(GLOBAL_CXXFLAGS_PCH) $(GLOBAL_CXXFLAGS) $(CXXFLAGS) $($@_CXXFLAGS) -MMD -MF $(call filename-to-dep, $@) -MP + $(trace-cxx) $(CXX) -o $@ -c $< $(CPPFLAGS) $(GLOBAL_CXXFLAGS_PCH) $(GLOBAL_CXXFLAGS) $(CXXFLAGS) $($@_CXXFLAGS) $(ERROR_SWITCH_ENUM) -MMD -MF $(call filename-to-dep, $@) -MP $(buildprefix)%.o: %.cpp @mkdir -p "$(dir $@)" - $(trace-cxx) $(CXX) -o $@ -c $< $(CPPFLAGS) $(GLOBAL_CXXFLAGS_PCH) $(GLOBAL_CXXFLAGS) $(CXXFLAGS) $($@_CXXFLAGS) -MMD -MF $(call filename-to-dep, $@) -MP + $(trace-cxx) $(CXX) -o $@ -c $< $(CPPFLAGS) $(GLOBAL_CXXFLAGS_PCH) $(GLOBAL_CXXFLAGS) $(CXXFLAGS) $($@_CXXFLAGS) $(ERROR_SWITCH_ENUM) -MMD -MF $(call filename-to-dep, $@) -MP $(buildprefix)%.o: %.c @mkdir -p "$(dir $@)" diff --git a/src/libexpr/local.mk b/src/libexpr/local.mk index 2171e769b..d243b9cec 100644 --- a/src/libexpr/local.mk +++ b/src/libexpr/local.mk @@ -46,3 +46,5 @@ $(foreach i, $(wildcard src/libexpr/flake/*.hh), \ $(d)/primops.cc: $(d)/imported-drv-to-derivation.nix.gen.hh $(d)/primops/derivation.nix.gen.hh $(d)/fetchurl.nix.gen.hh $(d)/flake/flake.cc: $(d)/flake/call-flake.nix.gen.hh + +src/libexpr/primops/fromTOML.o: ERROR_SWITCH_ENUM = diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 741e1d1f5..5a2dd99af 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -68,7 +68,7 @@ public: case lvlNotice: case lvlInfo: c = '5'; break; case lvlTalkative: case lvlChatty: c = '6'; break; case lvlDebug: case lvlVomit: c = '7'; - default: c = '7'; break; + default: c = '7'; break; // should not happen, and missing enum case is reported by -Werror=switch-enum } prefix = std::string("<") + c + ">"; } From bf2c5c3958c258584ac66519124d05ccf446aaeb Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 3 Apr 2023 18:38:11 +0200 Subject: [PATCH 6/6] nix-store.cc: Refactor, remove qDefault --- src/nix-store/nix-store.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 2d784376d..7035e6a7b 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -277,17 +277,17 @@ static void printTree(const StorePath & path, static void opQuery(Strings opFlags, Strings opArgs) { enum QueryType - { qDefault, qOutputs, qRequisites, qReferences, qReferrers + { qOutputs, qRequisites, qReferences, qReferrers , qReferrersClosure, qDeriver, qBinding, qHash, qSize , qTree, qGraph, qGraphML, qResolve, qRoots }; - QueryType query = qDefault; + std::optional query; bool useOutput = false; bool includeOutputs = false; bool forceRealise = false; std::string bindingName; for (auto & i : opFlags) { - QueryType prev = query; + std::optional prev = query; if (i == "--outputs") query = qOutputs; else if (i == "--requisites" || i == "-R") query = qRequisites; else if (i == "--references") query = qReferences; @@ -312,15 +312,15 @@ static void opQuery(Strings opFlags, Strings opArgs) else if (i == "--force-realise" || i == "--force-realize" || i == "-f") forceRealise = true; else if (i == "--include-outputs") includeOutputs = true; else throw UsageError("unknown flag '%1%'", i); - if (prev != qDefault && prev != query) + if (prev && prev != query) throw UsageError("query type '%1%' conflicts with earlier flag", i); } - if (query == qDefault) query = qOutputs; + if (!query) query = qOutputs; RunPager pager; - switch (query) { + switch (*query) { case qOutputs: { for (auto & i : opArgs) { @@ -443,7 +443,7 @@ static void opQuery(Strings opFlags, Strings opArgs) break; } - default: case qDefault: + default: abort(); } }