Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

frontend: propagate error as null instead of exit(1) #13625

Merged
merged 1 commit into from Feb 17, 2022
Merged

frontend: propagate error as null instead of exit(1) #13625

merged 1 commit into from Feb 17, 2022

Conversation

ljmf00
Copy link
Member

@ljmf00 ljmf00 commented Feb 8, 2022

This patch propagates null when Module AST node can't be constructed correctly
instead of doing fatal(), which terminates the program. This is useful if users
use DMD frontend as a library.

Signed-off-by: Luís Ferreira contact@lsferreira.net

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 8, 2022 •
edited

Thanks for your pull request, @ljmf00!

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 22751 normal DMD as a library hang with fatal() on parseModule

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

@ljmf00 ljmf00 marked this pull request as ready for review February 8, 2022 22:22
@ljmf00 ljmf00 changed the title frontend: propagate error instead of exit(1) frontend: propagate error as null instead of exit(1) Feb 8, 2022
@ljmf00 ljmf00 force-pushed the propagate-fatal-error branch 2 times, most recently from c7c8503 to 7e016e7 Compare February 8, 2022 22:28
@ljmf00 ljmf00 marked this pull request as draft February 8, 2022 22:32
@ljmf00 ljmf00 marked this pull request as ready for review February 8, 2022 23:49
ljmf00
ljmf00 commented Feb 8, 2022
View reviewed changes
src/dmd/dmodule.d
@@ -793,26 +793,26 @@ extern (C++) final class Module : Package
if (i >= eBuf.length)
{
error("surrogate UTF-16 high value %04x at end of file", u);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to trigger this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a really malformed file. Try using a hex editor and tack a surrogate UTF-16 high value to the end of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you present an example? I tried but it triggers other errors. Maybe I missed something else. I don't think this is mandatory in this PR anyway, but would be cool to test this.

@ljmf00
Copy link
Member Author

ljmf00 commented Feb 9, 2022

Unittests are not running, trying to enable it on #13626 . Rebasing on top of it.

@ljmf00 ljmf00 marked this pull request as draft February 9, 2022 02:20
@ljmf00 ljmf00 force-pushed the propagate-fatal-error branch 2 times, most recently from 4b8337c to a5cd846 Compare February 13, 2022 21:38
@ljmf00 ljmf00 mentioned this pull request Feb 13, 2022
Merged
@RazvanN7
Copy link
Contributor

A different approach would be to use version(DMDLIB) since AFAICT the current behavior is fine for the monolith compiler.

@ljmf00
Copy link
Member Author

ljmf00 commented Feb 14, 2022

A different approach would be to use version(DMDLIB) since AFAICT the current behavior is fine for the monolith compiler.

I don't think propagating a null or a custom Result struct will make things bad/hard to the compiler, but fatal() can bring problems in the flow. It's also not hard to debug such situations since an error is displayed anyway and the exit code is the same for the compiler, since it checks the global error count. I think bringing both won't add meaningful value to the logic but rather make it hard to read.

@thewilsonator
Copy link
Contributor

is this still draft?

@ljmf00
frontend: propagate error as null instead of exit(1)
ffd3994 
This patch propagates null when Module AST node can't be constructed correctly
instead of doing fatal(), which terminates the program. This is useful if users
use DMD frontend as a library. This patch also increases coverage on the
affected lines.

Fixes issue 22751.

Signed-off-by: Luís Ferreira <contact@lsferreira.net>
@ljmf00
Copy link
Member Author

ljmf00 commented Feb 17, 2022

is this still draft?

Since #13628 got merged, no.

@ljmf00 ljmf00 marked this pull request as ready for review February 17, 2022 17:15
@dlang-bot dlang-bot merged commit 7bf49c8 into dlang:master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thewilsonator thewilsonator thewilsonator approved these changes

Assignees
No one assigned
Labels
auto-merge Bug Fix
Projects
None yet
Milestone
No milestone
4 participants
@ljmf00 @dlang-bot @RazvanN7 @thewilsonator