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.

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

Generate expressive diagnostic messages with the offending line #8487

Merged
merged 2 commits into from
Dec 21, 2018

Conversation

wilzbach
Copy link
Member

A very naive implementation to ask on feedback on this before I invest more time in this.

Questions

  1. Should we save the originally opened file (it's currently thrown away by default)? Re-opening them has the advantage of only paying this overhead in an error case.

  2. Under which CLI flag should this be exposed?

  3. Is there a chance that we can do sth. similar like we do with the coloring? Only if dmd is run in a terminal, turn of the expressive diagnostics by default if dmd, but still have flags to control the behavior manually

Clang calls this "expressive diagnostics" and there's a lot more cool stuff that we can do.

Motivation

Compiler error for humans sums it up pretty nicely.

Also note that all three major C++ compilers (Clang, GCC, MSVC) have such expressive diagnostics enabled by default. It's time for DMD to catch up with the modern era.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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 fetch digger
dub run digger -- build "master + dmd#8487"

@wilzbach wilzbach changed the title [RFC] Generate Clang/GCC-like expressive diagnostic messages with the offending line [RFC] Generate expressive diagnostic messages with the offending line Jul 11, 2018
@jacob-carlborg
Copy link
Contributor

I like it. Would be nice to have as default.

I’ve been thinking about fixits that Clang has as well. This would be especially useful for deprecations that usually contains a suggestion how to fix it.

@wilzbach
Copy link
Member Author

I'd add a test with mixin() and mixin template

Added. For simplicity, I don't do the pretty printing for mixins for now though.

Would be nice to have as default.

Yeah that's my goal too!
However, before we make it the default, I think we should have it as an opt-in for two or three releases, s.t. we get enough feedback/testing.

@WalterBright
Copy link
Member

I want to thank @wilzbach for finding a way to do this that doesn't involve changing 30 or 40 files :-) I like that the implementation is pretty encapsulated. When other PRs cross-cut through the files, it shows how poorly encapsulated the compiler internals are.

  1. Reopen the file for printing errors. (Otherwise, keeping the files around consumes a lot of memory.) But once it is reopened for error messages, cache it in a buffer.)

  2. I'd rather the switch be "context" rather than "pretty". Pretty-printing usually refers to a function identifier being expanded to printing the whole declaration.

  3. I don't think it being in outputting to a tty should change this behavior.

[On a personal note, I am annoyed by this, for the simple reason that the DMC C compiler has done the line with the caret since 1982, and not a soul ever cared for the feature or even mentioned it. So I dropped it for DMD. Suddenly, it's the height of fashion, innovation and modernity to have it, and I'm 180 degrees out of phase. Sigh.]

@WalterBright
Copy link
Member

In case I wasn't clear, yes, let's move forward with it.

@wilzbach wilzbach changed the title [RFC] Generate expressive diagnostic messages with the offending line Generate expressive diagnostic messages with the offending line Jul 19, 2018
@wilzbach wilzbach force-pushed the errors branch 3 times, most recently from 3c7d9ca to 835a240 Compare July 23, 2018 15:19
Copy link
Member Author

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Added a simple line-by-line cache, but before I figure out how to add it to all the makefile, it would be good to know if this should live in a separate module or somewhere else.

src/dmd/filecache.d Outdated Show resolved Hide resolved
@andralex
Copy link
Member

cc @WalterBright

src/dmd/filecache.d Outdated Show resolved Hide resolved
src/dmd/filecache.d Outdated Show resolved Hide resolved
src/dmd/filecache.d Outdated Show resolved Hide resolved
src/dmd/filecache.d Outdated Show resolved Hide resolved
src/dmd/filecache.d Outdated Show resolved Hide resolved
src/dmd/filecache.d Outdated Show resolved Hide resolved

static fileCache = FileCache();

shared static this()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid module constructors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the module constructor, but now we need to manually initialize in mars.d / frontend.d

src/dmd/globals.d Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Dec 18, 2018

Moving forward post this pr, it would be really nice to have location ranges (start and end locations for all expressions).

@wilzbach wilzbach force-pushed the errors branch 2 times, most recently from 368c908 to 5be6c0d Compare December 18, 2018 14:47
@wilzbach
Copy link
Member Author

Rebased this PR. It's still behind the -verrors=context flag, so that we can get some real-world feedback before we enable it by default.

src/dmd/errors.d Outdated
// ignore invalid files
loc != Loc.initial &&
// ignore mixins for now
!loc.filename.strstr(".d-mixin-"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the addition of -mixin=<filename> might want to doublecheck that logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. For now, I just added && !global.params.mixinOut as that's definitely safe.
In the future, we probably need to call flushMixins or make the fileCache aware of mixin files.

@wilzbach
Copy link
Member Author

Ping @thewilsonator. Anything blocking this PR?
It's not enabled by default yet and it would be great to gather some real-world feedback with the next release / nightlies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants