-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Error-specific Diagnostics subclasses #92
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
Conversation
… object dotty.tools.dotc.reporting
error-specific Diagnostic subclass
This is just awesome! Very happy to see this direction. Slightly related to this (to guide the design, not saying you should implement this immediately): one feature request we get a lot is to make error / warning / deprecation / lint messages configurable in their severity based on where they occur, what the message is, when the deprecation was introduced. In a way this is the reporting counterpart of SIP-18 (language features): allow teams to decide themselves which warnings should be errors (right now it's all or nothing with -Xfatal-warnings). |
I like this idea! |
@@ -41,9 +40,9 @@ class ConsoleReporter( | |||
} | |||
|
|||
override def doReport(d: Diagnostic)(implicit ctx: Context): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some teams people make a decision to treat warning as errors. It would be good to have a way to configure which types of diagnostics get which level of Severity.
IMHO there should be way to increase severity, but decreasing should be forbidden.
This isn't directly related, but I noticed in the diff that we issue a parse error for invalid use of implicit modifiers on classes. The IDE experience is better is we defer that sort of error so that we can still typecheck the rest of the file. A major feature if the IntelliJ presentation compiler is its handling of invalid code. Regarding demoting errors to warnings: this might be valid for newly added soundness fixes that an upgrading user is not ready to address. Scala 2.11 has -Xsource:2.10 for this use case. Obviously doing this in general will lead to erroneous trees leaking into the back end so it would be a "use at your own risk" tool. |
I think this is a good direction at some point in the future, but I would not do it yet right now. The problem is that the added indirection imposes a tax on understanding and creating the typechecker code. The maintainers and builders of typechecker, refchecks, etc would be slowed down greatly if they had to deal with these indirections in their code. So I would let things shake themselves out a little and then decide on a good structure of the error messages that we can live with. I had the same experience with scalac. Once error messages were encoded in helper classes my perceived productivity dropped by half. The advantage of more structured errors are (1) localization -- but we don't plan to do that and (2) easier neg tests. Because of (2) I think there will be a time when the added overhead of writing and maintaining the compiler frontend will be justified by more stability of tests. But that time is not now. |
The biggest advantage is providing structured information for the IDE, which can render them in more useful ways and build "quick fixes" Right now, Scala IDE has to regex this sort of info out of unstructured strings. |
I agree with the advantages for IDE use, as long as this is part of the compiler API. |
We have decided we will come back to this in the future, when error messages have stabilized. Right now it is too much a drag on daily work flow. |
As suggested in this Google Groups Thread, this is an attempt to introduce specific error classes for each kind of error, instead of having String as the universal datatype.
There are many advantages:
This PR is only a small stub, and I invite everyone to give feedback and to help find the right way to do it.
There are still many things to do:
ctx.error(string, pos)
calls byctx.report(diagnostics)
, same forctx.warning
/cc @adriaanm