Skip to content

Error reporting improvements #99

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

Merged
merged 8 commits into from
Apr 4, 2014
Merged

Conversation

samuelgruetter
Copy link
Contributor

In today's meeting, it was decided that error-specific Diagnostics classes will be introduced, but only later, so that we can quickly & easily create string error messages as long as things change quickly.

So this PR leaves error-reporting as string-based as it was before, and only makes a few improvements to error-reporting which should be applied already now. The advantages are the following:

  • debug output using the i interpolator does not fail any more due to uncaught SuppressedMessage exceptions
  • the i string interpolator is usable everywhere, not just in typer
  • Diagnostic does not store a reference to ContextBase any more
  • no mutable field suppressNonSensicalErrors in Context
  • no (unchecked) SuppressedMessage throwing
  • two additional small fixes (see the last two commits)

Instead of only the i string interpolator, there are now two: i and d.

Originally, the i"..." string interpolator added three features to the s interpolator:

  1. On all Showables, show is called instead of toString
  2. Lists can be formatted using the desired separator between two % signs, eg i"myList = (${myList}%, %)"
  3. Detection of non-sensical error messages by throwing SuppressedMessage execptions

This PR removes 3) from the i interpolator, and adds the d interpolator, which does 1), 2) and 3), but instead of throwing SuppressedMessage execptions, it just marks the returned string with opening and closing "nonsensical" tags.

When should which interpolator be used:

  • d"..." if (the error message might be nonsensical) && (the string might end up in a Diagnostic)
  • i"..." otherwise

Ideally, d"..." would return a "magical data structure" which stores the message string and whether it's sensical or not (as sketched in today's meeting). However, this is not possible as long as we want to have all string operations (concatenation, stripMargin, interpolation, etc) on strings generated with d.

I agree that the way nonsensical error messages are marked in this PR is not very nice, and this PR is definitely not the final solution, but looking at the above list of improvements, I think it's still worth applying it.

@samuelgruetter
Copy link
Contributor Author

review by @odersky @gzm0 please

@DarkDimius
Copy link
Contributor

I'd say that 001bc10 should be squashed into 48196ca

@samuelgruetter
Copy link
Contributor Author

48196ca just copied the isSensical code that was already there before (in InfoString), and 001bc10 is a fix, so I'd like to keep them separate

@gzm0
Copy link
Contributor

gzm0 commented Mar 27, 2014

Do all individual commits compile?

@gzm0
Copy link
Contributor

gzm0 commented Mar 27, 2014

That's all

@odersky
Copy link
Contributor

odersky commented Mar 28, 2014

We could eliminate the redundant "show"s while we are at it, but it's not critical to do it now. Otherwise, looks good to me.

@odersky
Copy link
Contributor

odersky commented Apr 2, 2014

@samuelgruetter Can you remerge please? Then this can go in IMO.

@samuelgruetter
Copy link
Contributor Author

rebased and addressed comments @gzm0

@gzm0
Copy link
Contributor

gzm0 commented Apr 4, 2014

LGTM, sorry for being so late. My phone wouldn't send my comment and I forgot.

@odersky odersky closed this Apr 4, 2014
@odersky odersky reopened this Apr 4, 2014
odersky added a commit that referenced this pull request Apr 4, 2014
@odersky odersky merged commit fa098bd into scala:master Apr 4, 2014
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.

4 participants