feat(errors): make verrors=context global and update test outputs #17085

royalpinto007 · 2024-11-23T10:43:20Z

Make verror=context usage global and update related test files accordingly.

dlang-bot · 2024-11-23T10:43:24Z

Thanks for your pull request and interest in making D better, @royalpinto007! 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#17085"

compiler/src/dmd/mars.d

compiler/src/dmd/globals.d

dkorpel · 2024-11-23T15:39:35Z

Was it decided somewhere that this should become the default? I prefer the current default, and don't like how this makes the test suite harder to maintain.

maxhaton · 2024-11-23T23:35:38Z

Was it decided somewhere that this should become the default? I prefer the current default, and don't like how this makes the test suite harder to maintain.

Don't think so, but we are one of not many languages still not doing the squiggle by default. That debate aside, the last time I think someone (hatf0 I think) tried to turn it a bunch of things blew up so I asked @royalpinto007 to look into just to see how things were going on that front.

bangbangsheshotmedown · 2024-11-25T14:27:18Z

Was it decided somewhere that this should become the default? I prefer the current default, and don't like how this makes the test suite harder to maintain.

I like this PR

With the code example, i know exactly what's wrong without having to decipher a lengthily message

I use Zig a lot and this is so much better

royalpinto007 · 2024-12-03T13:53:15Z

I need some help with the compilable/testcolor.sh file. I tried resolving it, but unfortunately, I wasn't able to.

Signed-off-by: royalpinto007 <[email protected]>

thewilsonator · 2024-12-08T09:23:26Z

ping @maxhaton please review

RazvanN7

I like it.

dkorpel

I don't know if it's a good idea to switch the default, I'll bring that up tomorrow in the DLF monthly meeting.

However, the test suite doesn't have to use the default. It's interesting to see what all these errors now look like with -verrors=context, but it also makes me notice that often, the column location is wrong. So this is very useful to spot all the places where error locations can be improved, but perhaps we shouldn't promote -verrors=context yet while it's inaccurate so many times.

I also notice the way the tests are updated is that #line markers are removed and all test output blocks are merged and put at the beginning. This means that now, a small change in the test will cause a large diff in a big output block. The original structure was supposed to prevent that. I have been working on a special test runner directive that allows local, line number-independent error messages: #14515

In any case, I don't think the 5000 new copied lines of code with an error are worth adding to the test suite.

dkorpel · 2024-12-12T09:51:41Z

compiler/src/dmd/root/string.d

+            this.index = 0;
+            this.eolIndex = 0;
+            this.nextIndex = 0;
        }

-        public bool empty() { return index == text.length; }
+        public bool empty() { advance(); return index >= text.length; }

        public void popFront() { advance(); index = nextIndex; }

-        public const(char)[] front() { advance(); return text[index .. eolIndex]; }
+        public const(char)[] front()
+        {
+            advance();
+            if (index > eolIndex || index >= text.length) {
+                return "";
+            }
+            return text[index .. eolIndex];
+        }


What's this change for, does this PR uncover a bug in splitLines?

dkorpel · 2024-12-12T09:52:50Z

compiler/test/compilable/diag20916.d

 ---
 */

-#line 1
+// Line 1 starts here


But it doesn't, this comment makes no sense when there's no #line 1 directive anymore

dkorpel · 2024-12-12T09:56:42Z

compiler/test/compilable/b16976.d

+compilable/b16976.d(73): Deprecation: foreach: loop index implicitly converted from `size_t` to `int`
+    foreach(int i, v; dyn) { }
+    ^


Caret is in the wrong position

dkorpel · 2024-12-12T09:57:04Z

compiler/test/compilable/compile1.d

+compilable/compile1.d(232): Deprecation: use of complex type `cdouble` is deprecated, use `std.complex.Complex!(double)` instead
+cdouble c6096;
+        ^


Caret is in the wrong position

dkorpel · 2024-12-12T09:57:57Z

compiler/test/compilable/sw_transition_complex.d

-compilable/sw_transition_complex.d(39): Deprecation: use of imaginary type `idouble*` is deprecated, use `double` instead
-compilable/sw_transition_complex.d(40): Deprecation: use of imaginary type `ifloat*` is deprecated, use `float` instead
---
-*/
 creal* c80pointer;
 cdouble* c64pointer;
 cfloat* c32pointer;


The messages are no longer local to the relevant code.

dkorpel · 2024-12-12T10:05:17Z

changelog/dmd.verror_context.dd

@@ -0,0 +1,33 @@
+Make `verror=context` Usage Global and Update Related Test Files


Suggested change

Make `verror=context` Usage Global and Update Related Test Files

Enable `-verrors=context` by default

Test suite changes are irrelevant to users.

dkorpel · 2024-12-12T10:06:49Z

changelog/dmd.verror_context.dd

@@ -0,0 +1,33 @@
+Make `verror=context` Usage Global and Update Related Test Files
+
+The `verror=context` error handling mechanism has been made global, ensuring that error messages now consistently include context information across the entire codebase. Previously, error handling was localized, leading to some errors lacking context in the output.


This is a weird description: all -verrors=context does is print the line of code the error applies to under the message. It has nothing to do with 'across the entire codebase' vs 'localized'.

maxhaton · 2024-12-13T16:40:58Z

Consensus: enable this for users, disable it in almost all of the test suite for visual noise.

@royalpinto007 thank you very much for updating the tests though. We can't enable it without making sure it works acorss the whole test suite without blowing up! Now we can.

Can we name the option to turn it off -verrors=simple?

royalpinto007 marked this pull request as draft November 23, 2024 10:43

royalpinto007 force-pushed the verrors-context-global branch from 17c012d to 467933c Compare November 23, 2024 10:45

thewilsonator reviewed Nov 23, 2024

View reviewed changes

compiler/src/dmd/mars.d Outdated Show resolved Hide resolved

royalpinto007 force-pushed the verrors-context-global branch from 467933c to e9d3051 Compare November 23, 2024 12:03

thewilsonator reviewed Nov 23, 2024

View reviewed changes

compiler/src/dmd/mars.d Outdated Show resolved Hide resolved

compiler/src/dmd/globals.d Outdated Show resolved Hide resolved

royalpinto007 force-pushed the verrors-context-global branch 3 times, most recently from 0081e4e to e3d8e37 Compare November 23, 2024 15:24

royalpinto007 force-pushed the verrors-context-global branch 2 times, most recently from 61d3006 to 18a28ac Compare November 23, 2024 17:07

thewilsonator added Merge:Blocked Review:Needs Changelog A changelog entry needs to be added to /changelog labels Nov 23, 2024

royalpinto007 force-pushed the verrors-context-global branch from 18a28ac to f079c7c Compare November 25, 2024 09:37

royalpinto007 force-pushed the verrors-context-global branch 13 times, most recently from 8df6f29 to ffd4b87 Compare November 29, 2024 14:10

royalpinto007 force-pushed the verrors-context-global branch 2 times, most recently from a3b801e to bc982a6 Compare December 1, 2024 10:04

royalpinto007 force-pushed the verrors-context-global branch 15 times, most recently from 5808d4e to 222a468 Compare December 4, 2024 20:04

feat(errors): make verrors=context global and update test outputs

3fa3274

Signed-off-by: royalpinto007 <[email protected]>

royalpinto007 force-pushed the verrors-context-global branch from 222a468 to 3fa3274 Compare December 7, 2024 06:35

royalpinto007 marked this pull request as ready for review December 7, 2024 07:38

royalpinto007 requested a review from thewilsonator December 7, 2024 15:41

thewilsonator removed Merge:Blocked Review:Needs Changelog A changelog entry needs to be added to /changelog labels Dec 8, 2024

RazvanN7 approved these changes Dec 12, 2024

View reviewed changes

dkorpel requested changes Dec 12, 2024

View reviewed changes

royalpinto007 closed this Dec 16, 2024

royalpinto007 mentioned this pull request Dec 16, 2024

feat(errors): implement verrors=simple to skip context printing #20566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(errors): make verrors=context global and update test outputs #17085

feat(errors): make verrors=context global and update test outputs #17085

royalpinto007 commented Nov 23, 2024 •

edited

Loading

dlang-bot commented Nov 23, 2024

dkorpel commented Nov 23, 2024

maxhaton commented Nov 23, 2024 •

edited

Loading

bangbangsheshotmedown commented Nov 25, 2024

royalpinto007 commented Dec 3, 2024 •

edited

Loading

thewilsonator commented Dec 8, 2024

RazvanN7 left a comment

dkorpel left a comment

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024

dkorpel Dec 12, 2024 •

edited

Loading

maxhaton commented Dec 13, 2024

		@@ -0,0 +1,33 @@
		Make `verror=context` Usage Global and Update Related Test Files

	Make `verror=context` Usage Global and Update Related Test Files
	Enable `-verrors=context` by default

		@@ -0,0 +1,33 @@
		Make `verror=context` Usage Global and Update Related Test Files

		The `verror=context` error handling mechanism has been made global, ensuring that error messages now consistently include context information across the entire codebase. Previously, error handling was localized, leading to some errors lacking context in the output.

feat(errors): make verrors=context global and update test outputs #17085

feat(errors): make verrors=context global and update test outputs #17085

Conversation

royalpinto007 commented Nov 23, 2024 • edited Loading

dlang-bot commented Nov 23, 2024

Bugzilla references

Testing this PR locally

dkorpel commented Nov 23, 2024

maxhaton commented Nov 23, 2024 • edited Loading

bangbangsheshotmedown commented Nov 25, 2024

royalpinto007 commented Dec 3, 2024 • edited Loading

thewilsonator commented Dec 8, 2024

RazvanN7 left a comment

Choose a reason for hiding this comment

dkorpel left a comment

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024

Choose a reason for hiding this comment

dkorpel Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

maxhaton commented Dec 13, 2024

royalpinto007 commented Nov 23, 2024 •

edited

Loading

maxhaton commented Nov 23, 2024 •

edited

Loading

royalpinto007 commented Dec 3, 2024 •

edited

Loading

dkorpel Dec 12, 2024 •

edited

Loading