fix: user errors do not invite a bug report - #1306
gennaroprota wants to merge 10 commits into
Conversation
🧾 Changes by Scope
🔝 Top Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1306 +/- ##
========================================
Coverage 83.12% 83.12%
========================================
Files 35 35
Lines 3662 3662
Branches 844 844
========================================
Hits 3044 3044
Misses 410 410
Partials 208 208
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
An automated preview of the documentation is available at https://1306.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-10-01 11:07:20 UTC |
68fc5a0 to
3765317
Compare
|
The historical motivation is that when MrDocs was a new tool, errors were usually not user errors. They were actually MrDocs failing to extract the symbols. That's why we invited a bug report. If you notice, the message doesn't say this is a user error or a MrDocs error. The message used to say that if the user thinks this is a MrDocs error, they could report it (because, at least at the time, there was a high probability it was a MrDocs error, not a user error). This historical rationale matters because the whole point of the previous behavior was that we didn't know whether it was MrDocs or a user. There's no point in a new command to report bugs because we don't know if something is a bug or a user error. If we know there's a bug, we shouldn't be reporting it. We should fix the bug. If fixing the bug is too hard, we should never take this path, and the feature is simply unsupported. Thus, there's no need to And as far as user errors that are actually MrDocs errors are concerned (the original motivation for these messages), if we now think that, for an individual error, the probability that it's the user's fault is much higher than the probability that this is MrDocs' fault, then we can simply remove the part of the message that suggests opening a pull request. Then the user will figure it out in the rarer case when it's not really his fault. |
gennaroprota
changed the title
It's not that we know in advance that there's a bug, just that, sometimes, the code can detect a bug. For instance, if you get past the (we might as well use an assertion for such cases, as long as it yields a clear error message). |
|
Yes. The project convention for code invariants (or "contract preconditions") is MRDOCS_ASSERT (or MRDOCS_UNREACHABLE in some cases). |
|
One problem, tough, is that |
Yes. These are known and intentional properties of these macros, their actual underlying primitives (such as cassert), and their proposed replacements ( At best, I believe this is all unrelated to the scope of the original issue, which is just about adapting the message because of the historical problem that comes from the fact that an error used to be more likely on mrdocs than on the user at the time. We could either remove the message completely now that mrdocs seems stable enough and the users can more easily find how to open issues. Or, if still in doubt, we can have a single message about how to open issues at the end of the run IF the user thinks they didn't commit an error. I understand Matheus also makes the false distinction between "internal" and "external" errors, because he's also not aware of this background, but there should be no intentional internal errors.
These messages would be ironic because the effort to fix the error is lower than the effort to notice there's an error and to come up with a message in a PR. |
Every error-level message ended with an invitation to report a bug, followed by the MrDocs version and the places in the MrDocs sources that raised and reported the error. A missing config file, for instance, produced all that and, with `--warn-as-error`, so did every undocumented symbol. The invitation dates from when an error was more likely to come from MrDocs than from the user, which is no longer the case. So, we now drop the whole block, together with report::setSourceLocationWarnings, which only switched it off, and report::call, which only supplied a location for it. Fixes cppalliance#1116.
The places which quote the message of one error inside another one used Error::message, which ends with the file and line, in the MrDocs sources, where the error was created. So, for instance, a doc-comment warning came out as "HTML <td> tag not followed by </td> (src/mrdocs/AST/ExtractDocComment.cpp:774) at <their file> (6)". They now quote Error::reason, which is the message without the location.
The default value of `source-root` is the bare placeholder "<config-dir>", but the resolver stripped a placeholder only when a '/' followed it. So, the placeholder survived into the path, and every configuration which omitted `source-root` failed with "path does not exist", though the documentation says that a mrdocs.yml at the root of the project needs nothing set. We now treat an empty remainder as ".". I've added a test.
Reason: A few places tested `_NDEBUG`, which nothing defines, instead of `NDEBUG`, so no build had the handlers which report an exception that reaches the main function. Contextually, this removes the calls to `PrintStackTrace`, because they were in the catch handlers, where the stack has already been unwound, so they would show the handler rather than the place where the exception was thrown.
Reason: Nothing used it, and it was error-prone: it mapped 0 to `Level::debug`, whose value is 1, and so on, one off the values of the enumerators.
Its format string had no placeholder for the error passed with it, so the reason was dropped and the warning ended with "because ". It now reads "Mapping failed: <reason>". Also, no other warning starts with a hand-written "Warning: ", so we drop that prefix.
When `getFileType` failed for a reason other than a missing file (which isn't a failure), the error said "Config file does not exist" and filled its only placeholder with the `Error` rather than the path. So, the path was dropped, and the message showed a location in the MrDocs sources. We now name the path and quote the reason of the `Error`.
8e92ad5 to
9b22b91
Compare
The four `addMember` overloads which file a symbol under its parent ended with a `report::error` for a kind they didn't handle, and three conversions in ExtractDocComment.cpp reported an error right before `MRDOCS_UNREACHABLE`. None of that code can be reached: `getParent` only ever yields a namespace, a record or an enum, the overloads handle every kind those can contain, and an overload set or a using-declaration is never a parent. So, the messages could only report a defect in MrDocs, which gives the user nothing to act on. We now mark that code with `MRDOCS_UNREACHABLE`, as the project does for its other invariants. Note that, in a release build, reaching any of it would now be undefined behavior, rather than produce a message.
This branch has not been deployed
3 participants
Every error-level message in MrDocs printed the internal source location and an invitation to file a bug report. For instance, a missing configuration file produced one, and with
--warn-as-errorso did every undocumented symbol, thus a reader was asked to report a MrDocs defect for a mistake in their own input.report::bugis now the only function that does that: it prints the message followed by the version and the source location a bug report needs, whileerror,fatal,warnand the rest print the message alone.Other defects surfaced while going through the reporting code and are fixed in their own commits.
Changes
report::bugnow reports MrDocs defects, with the bug-report details suppressed everywhere else; quoted errors carry their reason without the raising location;source-root's bare-placeholder default resolves to the configuration file's directory; the release-build exception report is compiled in again; the overload-set diagnostic names overload sets rather than enums; the unusedgetLevelis gone.source-rootdefault.Testing
tests/unit/Config.cpp covers the
source-rootdefault: it writes a minimal mrdocs.yml into a temporary directory and asserts the value ofsource-rootcomes back as that file's directory, which the old resolver could not do.The diagnostics themselves are not tested, as that seemed overkill.
Documentation
No page describes the diagnostics or reproduces their wording, so nothing needed updating.
report::bugis public API and carries its doc-comments, which include when to reach for it instead oferror.Closes #1116.