add errorsink.d, an abstract interface to error messages by WalterBright · Pull Request #14906 · dlang/dmd

WalterBright · 2023-02-22T08:39:15Z

I've made considerable progress into making the lexer standalone, and the lexer/parser standalone. The problem is its reliance on globals.d and errors.d, which pull in a bunch of other stuff not relevant to lexing/parsing. The code in errors.d is of rather stupefying complexity, which is completely irrelevant to the lexer/parser. This complexity also makes it difficult to create a standalone lexer/parser.

Enter the abstract interface for error messages, ErrorSink, in errorsink.d. It's only got the functions in it that the lexer/parser actually need. The lexer/parser should not need to know anything about color formatting, highlighting, gagging, or where the messages are sent to.

The constructors for the lexer/parser are passed an instance of ErrorSink.

This isn't perfect, there's still an ugly wart in it that relies on getenv. I'll tackle that problem after this one is incorporated.

Sorry about the high line count, although nearly all of it is just changing the call to error(). The substantive changes are pretty small.

The more we can reduce all these "everybody imports everybody else" the more tractable the code base will be.

dlang-bot · 2023-02-22T08:39:19Z

Thanks for your pull request, @WalterBright!

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#14906"

ibuclaw · 2023-02-22T08:55:30Z

Errrrrrrrrrrrr @jacob-carlborg ?

ibuclaw · 2023-02-22T09:10:18Z

#9350
#9484
#9480
#9507
#10072
#10711

jacob-carlborg · 2023-02-22T09:25:20Z

🤦

compiler/src/dmd/lexer.d

thewilsonator · 2023-02-22T10:35:36Z

Yay, lets add even more files before packaging dmd properly.

ibuclaw · 2023-02-22T10:41:45Z

Yay, lets add even more files before packaging dmd properly.

Worse than that, this PR literally reinvents the wheel... why?

thewilsonator · 2023-02-22T11:11:25Z

compiler/src/dmd/errorsink.d

+/***************************************
+ * Where error/warning/deprecation messages go.
+ */
+abstract class ErrorSink


Reuse existing wheels or explain why they are insufficient.

Such deficiencies might include dmd.frontend imports std.

Note also that dmd.frontend is a stable externally available API and so at worst this PR should move the definition from there to here, and have a publicly imported alias to the new place.

Reuse existing wheels or explain why they are insufficient.

This line illustrates the insufficiency:

Returns: false if the message should also be printed to stderr, true otherwise

This is the wrong abstraction. The user of Lexer should be able to send the errors anywhere, baking into the API "stderr" or not is insufficent. ErrorSink puts no constraint on where the messages should go. They can even go nowhere, as ErrorSinkNull demonstrates.

It's other insufficiency is it is centered around a pointer to a function which overrides the default behavior. This is a primitive form of OOP that fell out of favor in the 1980s. The abstract class, for which the user adds derived classes with specific behaviors, is the more modern approach. The abstract class designer does not have to anticipate what the implementation classes will be doing.

I don't want to touch dmd.frontend in this PR. As I mentioned to @ibuclaw it needs a significant overhaul.

Then I consider this PR to require approval from @jacob-carlborg before merging.

don't want to touch dmd.frontend in this PR

I do not trust you will ever get around to this. And this adds yet more files without packaging things properly and I trust even less that you will ever do that.

They can even go nowhere, as ErrorSinkNull demonstrates.

So can DiagnositReporter, return true;.

As I mentioned to @ibuclaw it needs a significant overhaul.

Where? why? (Hint, no it doesn't).

compiler/src/dmd/lexer.d

ljmf00

Please explain why we defer the creation of a proper error handling mechanism on DMD and also why not use the available interfaces exposed to be used on diagnostics?

WalterBright · 2023-02-22T20:36:56Z

I'm mystified by the unittest runner:

./build unittest
(DC) LEXER-unittest
(DC) DMD-unittest
(RUN) DMD-UNITTEST
Success

However, the unittests were not run, as I put a printf in them. So looking at the -v output, I see it is linking in:

../generated/linux/release/64/lexer-unittest.o ../generated/linux/release/64/backend-unittest.o ../generated/linux/release/64/common-unittest.o

What? The unittest feature in D is designed so you compile with -unittest, run the program, and the unittests are run. Where are all these extra .o files for? And why won't the unittests run when I run the generated dmd? What happened to our simple way of doing unittests?

WalterBright · 2023-02-22T20:59:55Z

Why?

The best way to make understandable code is to have all the inputs and outputs at the entry point to the function. John Carmack explains this far better than I in http://www.sevangelatos.com/john-carmack-on/

What I've been doing over several previous PRs is just that - giving Lexer everything it needs via the call to its constructor, rather than "side loading" things through global variables, like global. This PR addresses a remaining problem - where do error messages go? The Lexer should not care what happens to the error messages, it just needs a place to send them. This is abstracted into ErrorSink, which is just what it says, it's a sink for error messages. Taking a look at the abstract class, it's utterly trivial and can be understood completely in 30 seconds. A couple handy implementations of it are there as well - ErrorSinkNull which just does nothing, and ErrorSinkStderr which just writes the messages to stderr. Both are trivial.

Contrast this with dmd.errors. It's a bewildering panoply of functions. There's a hook in it, diagnosticHandler, which snatches away control. But the user still has to understand all that stuff in errors.d, and it still all gets pulled into the executable, along with all its dependencies. It's full of global state, exactly what is undesirable in encapsulating functionality.

This PR does not break diagnosticHandler, or change the existing code that interfaces with it, other than adding a parameter to the constructor of Lexer.

Everything works but the unittests. So far, I have not been able to debug that because I have not managed to make heads or tails of how the unittests are run. Something about unittests in dmd have gone completely off the rails into a swamp.

thewilsonator · 2023-02-23T08:45:41Z

compiler/src/dmd/errorsink.d

+/***************************************
+ * Where error/warning/deprecation messages go.
+ */
+abstract class ErrorSink


Then I consider this PR to require approval from @jacob-carlborg before merging.

don't want to touch dmd.frontend in this PR

I do not trust you will ever get around to this. And this adds yet more files without packaging things properly and I trust even less that you will ever do that.

They can even go nowhere, as ErrorSinkNull demonstrates.

So can DiagnositReporter, return true;.

As I mentioned to @ibuclaw it needs a significant overhaul.

Where? why? (Hint, no it doesn't).

jacob-carlborg · 2023-02-23T08:51:02Z

Then I consider this PR to require approval from @jacob-carlborg before merging.

I'm not doing any code reviews. This PR is the perfect example of why I'm not contributing anymore.

Geod24

Lots of useless imports;
The lazy instantiation of globals in the middle of semantic is a big no;
The two classes you provide, ErrorSinkNull and ErrorSinkStderr, are completely unused, something you have repeatedly complained about;

Instead of re-doing what other people have been wanting for years, perhaps we should just look into how/what @jacob-carlborg did, with DiagnosticReporter, which was also introduced gradually (starting with the lexer).

compiler/src/dmd/dmodule.d

Geod24 · 2023-02-23T10:02:13Z

compiler/src/dmd/dmodule.d

 * Params:
 *      mod = the Module
- *	aclasses = array to fill in
+ *      aclasses = array to fill in


Unrelated change, should be its own commit

Another case of tabs being converted to spaces by my checkin procedure. Not worth another PR.

compiler/src/dmd/doc.d

compiler/src/dmd/dsymbolsem.d

Geod24 · 2023-02-23T10:03:15Z

compiler/src/dmd/dtemplate.d

                if (!s.isStaticIfDeclaration())
                {
-		    //s.dsymbolSemantic(sc2);
+                    //s.dsymbolSemantic(sc2);


Unrelated, own commit

That came about because of my checkin process converts all tabs to spaces, and apparently there were tabs on that line. Being a bit of trivia, there's no issue here.

compiler/src/dmd/expressionsem.d

Geod24 · 2023-02-23T10:07:19Z

compiler/src/dmd/globals.d


    extern (C++) void _init()
    {
+        global.errorSink = new ErrorSinkCompiler;


This should be moved out of globals and into main to reduce circular dependencies

It used to be in main, as you commented on later. The trouble was the main-less unit tester in the test suite. There's a weird problem with the test suite "unit tester" as it makes no sense as a unit tester. This needs to be redone as just tests (with a main entry point), but redoing that is beyond the scope of this PR.

Geod24 · 2023-02-23T10:07:53Z

compiler/src/dmd/iasmgcc.d

    import dmd.mtype : TypeBasic;

+    if (!global.errorSink)
+        global.errorSink = new ErrorSinkCompiler;


Likewise, this pattern is not good

You're right, that's left over from a previous incarnation of this PR. I had a fair amount of trouble figuring out where the unittests were being initialized.

Unfortunately, this code doesn't call globals._init, and so global.errorSink is not initialized. So I have to initialize it here.

Having everything in global is not ideal either.

compiler/src/dmd/mars.d

Geod24 · 2023-02-23T10:09:02Z

compiler/test/unit/lexer/location_offset.d

    enum code = "token";

-    scope lexer = new Lexer("test.d", code.ptr, 0, code.length, 0, 0);
+    scope lexer = new Lexer("test.d", code.ptr, 0, code.length, 0, 0, new ErrorSinkStderr);


I think this should just be a function at this point, instead of copy-pasted code

It's a class because it consists of several functions, not one.

I meant that at some point, we should realize that:

scope lexer = new TestLexer(code); // Or: scope lexer = makeTestLexer(code);

is the way to go.

That's worth considering for a future PR. We already use both techniques in dmd.

WalterBright · 2023-02-23T19:06:08Z

Lots of useless imports;

Removed. Explained in the comments.

The lazy instantiation of globals in the middle of semantic is a big no;

Removed. Explained in the comments.

The two classes you provide, ErrorSinkNull and ErrorSinkStderr, are completely unused, something you have repeatedly complained about;

ErrorSinkNull I plan to use in errors.d to replace the use of:

auto gaggedErrorsSave = global.startGagging();

in errors.d, which is the perfect use case for it.

I plan to use ErrorSinkError in the unit tests for Lexer.

Both are also an illustration for anyone needing their own custom error message sink.

Instead of re-doing what other people have been wanting for years, perhaps we should just look into how/what @jacob-carlborg did, with DiagnosticReporter, which was also introduced gradually (starting with the lexer).

ErrorSink is a better design. It's hard to think of anything simpler - if you can, I want to hear about it. Note also that the only import ErrorSink needs is dmd.location. I'd actually like to get it to zero imports. In general, using sinks is an excellent approach to eliminating arbitrary dependencies.

addressed all comments

No actionable technical changes requested

WalterBright · 2023-02-24T20:06:34Z

This serves our goals of reducing technical debt and making the lexer/parser more amenable to being standalone by reducing its dependencies on other modules. It's ready to merge. @RazvanN7 @dkorpel

dkorpel

Creating a copy of what already exists does not reduce technical debt. If there's deficiencies in DiagnosticReporter, why not fix that instead?

compiler/src/dmd/lexer.d

WalterBright · 2023-02-25T20:59:57Z

Creating a copy of what already exists does not reduce technical debt.

To reiterate:

it results in lexer.d having a dependency on dmd.console.
if a diagnosticreporter is used, the code for the non-use case remains in the program, unused
I quote:

 * The diagnostic handler.
 * If non-null it will be called for every diagnostic message issued by the compiler.
 * If it returns false, the message will be printed to stderr as usual.

it should not leave a builtin dependency on stderr. It should not have default behavior at all.
4. It uses an old-fashioned pointer-to-handler OOP design that was supplanted in 1990 (aka a "hook" interface)
5. abstract interfaces are a better design
6. It still leaves a dependency on the gagging global which is an execrable design practice
7. It is much more complicated than necessary
8. anyone can understand ErrorSink in 30 seconds
9. GUI programs don't have a console or stderr
10. it enables easy mock sinks, which are critical for unittesting
11. it "side loads" into Lexer, rather than going through the function parameter list

This PR does not delete diagnostichandler because that would break existing code. I wished to keep this PR as a refactoring. Migrating from diagnostichandler to the abstract interface can be done later.

answered and implemented

maxhaton · 2023-02-26T12:55:56Z

compiler/src/dmd/errorsink.d

+
+    void errorSupplemental(const ref Loc loc, const(char)* format, ...);
+
+    void warning(const ref Loc loc, const(char)* format, ...);


warningSupplemental?

I only added the ones used.

maxhaton · 2023-02-26T12:57:01Z

compiler/src/dmd/errorsink.d

+    void deprecation(const ref Loc loc, const(char)* format, ...);
+
+    void deprecationSupplemental(const ref Loc loc, const(char)* format, ...);
+}


message should also be included.

I only added the ones used. Others can be added as necessary.

maxhaton · 2023-02-26T13:01:40Z

Is this any less dependant on gagging?

WalterBright · 2023-02-26T23:14:25Z

Is this any less dependant on gagging?

Currently, no. But as I remarked upthread, there's an obvious case in colorHightlightCode() for using ErrorSinkNull instead of gagging. Eventually, I'd like to see all use of gagging replaced by ErrorSinkNull.

ibuclaw · 2023-02-26T23:26:58Z

compiler/src/dmd/iasmgcc.d

                else
                {
-                    p.error(s.loc, "expected identifier after `[`");
+                    p.eSink.error(s.loc, "expected identifier after `[`");


This just makes iasmgcc ugly to look at for no obvious benefit.

ibuclaw · 2023-02-26T23:28:14Z

compiler/src/dmd/iasmgcc.d

        const errors = global.errors;
        scope gas = new GccAsmStatement(Loc.initial, tokens);
-        scope p = new Parser!ASTCodegen(null, ";", false);
+        scope p = new Parser!ASTCodegen(null, ";", false, global.errorSink);


At this point, might as well make it a default param instead, there are dozens of repetitions of this pattern of change.

ibuclaw · 2023-02-26T23:32:38Z

compiler/src/dmd/errors.d

+    void error(const ref Loc loc, const(char)* format, ...)
+    {
+        va_list ap;
+        va_start(ap, format);
+        verror(loc, format, ap);
+        va_end(ap);
+    }


Dangerously close to hijacking gcc error diagnostics here. Or am I expected to just fork out the implementation to the C++ side of the compiler?

ntrel · 2023-09-01T10:20:45Z

compiler/src/dmd/errorsink.d

+
+    void error(const ref Loc loc, const(char)* format, ...)
+    {
+        fputs("Error: ", stderr);


Shouldn't loc come before "Error:" etc?

WalterBright added the Severity:Refactoring No semantic changes to code label Feb 22, 2023

WalterBright requested a review from ibuclaw as a code owner February 22, 2023 08:39

WalterBright force-pushed the errorsink branch 4 times, most recently from 4f04dca to 509ee75 Compare February 22, 2023 08:54

WalterBright force-pushed the errorsink branch 3 times, most recently from a3d5a8c to c1d394b Compare February 22, 2023 09:05

WalterBright force-pushed the errorsink branch 2 times, most recently from f467d27 to 06a8c6a Compare February 22, 2023 09:22

WalterBright force-pushed the errorsink branch 3 times, most recently from c4bcb74 to d36d348 Compare February 22, 2023 10:06

RazvanN7 reviewed Feb 22, 2023

View reviewed changes

compiler/src/dmd/lexer.d Show resolved Hide resolved

thewilsonator previously requested changes Feb 22, 2023

View reviewed changes

ljmf00 reviewed Feb 22, 2023

View reviewed changes

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

ljmf00 previously requested changes Feb 22, 2023

View reviewed changes

WalterBright force-pushed the errorsink branch 4 times, most recently from 8eb54d1 to 030f344 Compare February 22, 2023 20:00

thewilsonator previously requested changes Feb 23, 2023

View reviewed changes

Geod24 previously requested changes Feb 23, 2023

View reviewed changes

WalterBright force-pushed the errorsink branch from 0245a7b to 58ded7b Compare February 23, 2023 18:24

WalterBright force-pushed the errorsink branch from 58ded7b to 06b2a20 Compare February 23, 2023 19:11

WalterBright force-pushed the errorsink branch from 06b2a20 to 1966521 Compare February 24, 2023 08:15

WalterBright added the Review:Ready To Merge Let's get this merged label Feb 24, 2023

dkorpel previously requested changes Feb 25, 2023

View reviewed changes

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

WalterBright force-pushed the errorsink branch 3 times, most recently from ba5f8f1 to 245cc79 Compare February 26, 2023 01:44

add errorsink.d, an abstract interface to error messages

2d4267b

WalterBright force-pushed the errorsink branch from 245cc79 to 2d4267b Compare February 26, 2023 02:21

dkorpel approved these changes Feb 26, 2023

View reviewed changes

maxhaton reviewed Feb 26, 2023

View reviewed changes

WalterBright merged commit 49dec45 into dlang:master Feb 26, 2023

WalterBright deleted the errorsink branch February 26, 2023 23:10

ibuclaw reviewed Feb 26, 2023

View reviewed changes

dkorpel mentioned this pull request May 10, 2023

Lexer - new error handling #9480

Closed

ntrel reviewed Sep 1, 2023

View reviewed changes

RazvanN7 mentioned this pull request Feb 5, 2024

Add toPrettyChars method in ASTBase.Dsymbol #16153

Merged


		void errorSupplemental(const ref Loc loc, const(char)* format, ...);

		void warning(const ref Loc loc, const(char)* format, ...);

Uh oh!

Conversation

WalterBright commented Feb 22, 2023

Uh oh!

dlang-bot commented Feb 22, 2023

Bugzilla references

Testing this PR locally

Uh oh!

ibuclaw commented Feb 22, 2023

Uh oh!

ibuclaw commented Feb 22, 2023

Uh oh!

jacob-carlborg commented Feb 22, 2023

Uh oh!

Uh oh!

thewilsonator commented Feb 22, 2023

Uh oh!

ibuclaw commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ljmf00 left a comment

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Feb 22, 2023

Uh oh!

WalterBright commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Feb 23, 2023

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Feb 23, 2023

Uh oh!

WalterBright commented Feb 24, 2023

Uh oh!

dkorpel left a comment