Enable -verrors=context by default

[ellie-idb]

Why?

It's been nearly 3 years since this comment (which introduced -verrors=context): #8487 (comment), where @wilzbach said that -verrors=context would be eventually enabled by default, dependent on community feedback.

I feel that it's been enough time for that community feedback, and leaving it behind a feature flag is doing a disservice to most developers. dmd, admittedly, still has a pretty awful user experience, and this is one of many planned fixes to improve that.

[dlang-bot]

Thanks for your pull request and interest in making D better, @hatf0! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13174"

[maxhaton]

I think our error messages are massively behind the competition so I'm tempted to say this should be a no-brainer.

[ellie-idb]

Gotta do more work on fixing up the tests, since we're modifying the default compiler output.

[PetarKirov]

@hatf0 Check out the AUTO UPDATE option of run.d:
https://github.com/dlang/dmd/tree/master/test#automatically-update-the-test_output-segments

[ellie-idb]

Regarding forcibly specifying -verrors=basic, It looks like some test cases explicitly specify -verrors=context, but the current implementation makes the detailed context.... undeterministic to say the least (seen with test cases like fail_compilation/reserved_version.d). While it may have worked in those test cases before, I think that it's /generally/ in our best interest to just keep the output to be simple for the test suite.

This will, of course, be eventually fixed once I have my way (and introduce error codes to the compiler), and we can just test for those rather then greping over the output >:)

[ellie-idb]

Also looks like this exposed an issue with -verrors=context -- it segfaults when reading __stdin.d.

[Geod24]

I would say that's a blocker. First because now the tests that check -verror=context don't test it anymore.
Second because we don't want to enabled by default a feature that is broken.

[ellie-idb]

I would say that's a blocker. First because now the tests that check -verror=context don't test it anymore. Second because we don't want to enabled by default a feature that is broken.

Yeah... it looks like this exposed a lot of issues with it. I think I'm going to close this and take a stab at refactoring -verror=context, because it's horribly broken as-is.

The issues are mostly fixed (other then the whole "we don't test for -verrors=context"), but comes at the price of this PR becoming incredibly bloated beyond just "enabling -verrors=context" -- i.e. adding a header for FileCache & now requiring that it becomes initialized by other users of the front-end.

Realistically, it looks like the majority of the errors are related to FileCache, and it's near-total abandonment.

[Geod24]

Thanks! Getting features to a finished state is ungrateful work, yet highly appreciated.

Personally I don't mind keeping this PR open - For large body of work, I tend to make a giant PR that shows the final product, and break it down into smaller PRs to make it reviewable. But without a clear vision of the final product, it's hard for reviewers to tell whether the design is workable or not. But to each his own.

Feel free to ping (if needed, repeatedly) if your other PRs aren't reviewed timely.

[maxhaton]

There is a GSoC thingy open in projects for someone to work just on error messages. Could be one for next time round.

…

On Mon, 18 Oct 2021, 02:27 Mathias LANG, ***@***.***> wrote: Thanks! Getting features to a finished state is ungrateful work, yet highly appreciated. Personally I don't mind keeping this PR open - For large body of work, I tend to make a giant PR that shows the final product, and break it down into smaller PRs to make it reviewable. But without a clear vision of the final product, it's hard for reviewers to tell whether the design is workable or not. But to each his own. Feel free to ping (if needed, repeatedly) if your other PRs aren't reviewed timely. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13174 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLI75FFSGD7WH7GZDUMDSDUHNZWJANCNFSM5GC32X7Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[MoonlightSentinel]

This will, of course, be eventually fixed once I have my way (and introduce error codes to the compiler), and we can just test for those rather then greping over the output >:)

No, it won't. Tests must verify that the error messages point to the symbols/expressions/... causing the error - even with error codes. (Context: TEST_OUTPUT used to be optional which hid some really bad errors in the error messages). Have a look at the special sequences mentioned in the README if some tests need more flexibility.

W.r.t. to the output of -verrors=context being unreliable, that really needs to be fixed before enabling -verror=context.

[ellie-idb]

This will, of course, be eventually fixed once I have my way (and introduce error codes to the compiler), and we can just test for those rather then greping over the output >:)

No, it won't. Tests must verify that the error messages point to the symbols/expressions/... causing the error - even with error codes. (Context: TEST_OUTPUT used to be optional which hid some really bad errors in the error messages). Have a look at the special sequences mentioned in the README if some tests need more flexibility.

Point taken -- I'm just pretty irked with the current test suite and the over-bearingly large usage of grep (and subsequently how fragile it is to breakage), especially as I'm planning some significant work on error messages (and don't exactly want to have to re-generate every test case's test output). Would prefer a different solution to what we have now.

W.r.t. to the output of -verrors=context being unreliable, that really needs to be fixed before enabling -verror=context.

Agreed. No clue how and why -verrors=context became indeterministic and random, but it certainly is a blocker / ruins the user experience.