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

Error-specific Diagnostics subclasses #92

Closed
wants to merge 3 commits into from

Conversation

samuelgruetter
Copy link
Contributor

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:

  • Testing doesn't need to regex-match error messages, but can test the error message type, and its fields, if desired. Changing the wording of an error message doesn't require updating of many tests.
  • We can also write better unit tests which just check for the type of error.
  • Once the error classes stabilize, we can expose them to external tools, such as IDEs, type debuggers, etc, and they will benefit a lot.

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:

  • Replace all ctx.error(string, pos) calls by ctx.report(diagnostics), same for ctx.warning
  • Integrate TypeError, FatalTypeError, MalformedType, CyclicReference, MergeError in Types.scala
  • Currently, the logic which generates the error messages is spread over the whole compiler. Move it into dotty.tools.dotc.reporting.
  • Once all is moved, it should be possible to make the suppressed messages communication between Diagnostics and the i String interpolator (InfoString) nicer. Currently, it's based on a mutable field suppressNonSensicalErrors in ContextBase and on throwing/catching SuppressedMessage exceptions. Decouple Diagnostics from ContextBase, so that it becomes a minimal data structure with a small API that we can expose to other tools.
  • Probably we will also be able to remove Severity, and just encode the severity in the class names.

/cc @adriaanm

@adriaanm
Copy link
Contributor

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).

@DarkDimius
Copy link
Member

I like this idea!
But I believe it would be more convenient to have all error classes defined in single file(possibly with multiple packages defined in it).

@@ -41,9 +40,9 @@ class ConsoleReporter(
}

override def doReport(d: Diagnostic)(implicit ctx: Context): Unit =
Copy link
Member

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.

@retronym
Copy link
Member

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.

@odersky
Copy link
Contributor

odersky commented Mar 25, 2014

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.

@retronym
Copy link
Member

The biggest advantage is providing structured information for the IDE, which can render them in more useful ways and build "quick fixes"

image

image

image

Right now, Scala IDE has to regex this sort of info out of unstructured strings.

/cc @dragos @skyluc @dotta @huitseeker

@dragos
Copy link

dragos commented Mar 25, 2014

I agree with the advantages for IDE use, as long as this is part of the compiler API.

@odersky
Copy link
Contributor

odersky commented Apr 8, 2014

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.

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

Successfully merging this pull request may close these issues.

None yet

6 participants