Make sure messages are lazily evaluated until `report` in `Reporter` #1696

felixmulder · 2016-11-10T14:21:33Z

In this PR I make sure that messages are lazily evaluated to the same point in the code it was previously. Namely in Reporter.scala in the method Reporter#report. The message is forced within this method, WDYT - should it be made lazy as well?

Review by @odersky

felixmulder · 2016-11-10T14:23:41Z

The PR also adds a persist method to Message in order to allow for persisting messages without also accidentally persisting contexts resulting in memory leaks 👍 also docs

felixmulder · 2016-11-10T14:40:15Z

Also @Blaisorblade - if you're interested in having a look I'd appreciate it :)

odersky · 2016-11-10T14:51:02Z

I am afraid we should not evaluate on report either. Only leaf reporters (e.g. ConsoleReporter and ThrowingReporter) are allowed to force this. Previously this was achieved by making the message string a lazy val in Diagnostics.

felixmulder · 2016-11-10T16:00:09Z

Sounds reasonable - I'll update the PR with your feedback - thanks Martin.

Blaisorblade · 2016-11-10T16:06:42Z

test/dotty/tools/dotc/reporting/TestMessageLaziness.scala

+    override def report(d: => MessageContainer)(implicit ctx: Context) = ()
+  }
+
+  case class LazyError() extends Message(1000) {


Blaisorblade · 2016-11-10T16:07:29Z

src/dotty/tools/dotc/reporting/diagnostic/Message.scala

+  * This is useful when we need to add additional information to an existing
+  * message.
+  */
+class ExtendMessage(_msg: () => Message)(f: String => String) { self =>


Maybe MappedMessage or TransformedMessage would be even clearer.

Blaisorblade · 2016-11-10T16:08:48Z

src/dotty/tools/dotc/reporting/Reporter.scala

@@ -236,7 +240,7 @@ abstract class Reporter extends interfaces.ReporterResult {
    override def default(key: String) = 0
  }

-  def report(d: MessageContainer)(implicit ctx: Context): Unit =
+  def report(d: => MessageContainer)(implicit ctx: Context): Unit =


MessageContainer still takes messages by-name so I doubt it needs to be by-name itself.

If there's some reason I'm missing (totally possible), other uses of MessageContainer would have to also become by-name—starting from isHidden. A test might be harder, so one would have to grep for uses of MessageContainer.

I think I'll investigate this with...more tests! 😮

Blaisorblade · 2016-11-10T16:36:38Z

IIUC Reporter#report does not force the Message thunk itself, even if the PR description says so—that's delegated to doReport, so the PR might still pass @odersky's comment.

To know I inspected overrides of doReport, and found two problematic cases:

mixin HideNonSensicalMessages forces the message to check isNonSensical, hence so does Reporter.fromSimpleReporter (which is just a forwarder). I'm not sure that's legal.
StoreReporter might force the message in debug mode (if Printers.typr is changed, but the logic isn't documented), but it violates the new invariants on leaks — if it can't call persist, it needs to be documented as "leaky" and used with care (only held as long as the context is alive anyway).

Conversely, ConsoleReporter forces Message as allowed, and ThrowingReporter does not force Message itself.

This sort of complicated invariant is why I advocated dynamic checking via assertions, at least in some (existing?) debug mode (beyond the testcase, which are a good step): even with your new ScalaDocs on leaks, we need to trust reviewers to maintain a non-local invariant. Some other solution might work, but enforcing the invariant isn't.

Also, to prevent leaks, MessageContainer.contained should probably still clear msgFn (so it should be a var, or contained could be a lazy val once By-name params used only in a lazy field should be cleared to prevent leaks #1692 is fixed), and contained should store the persisted message. Or does the PR address this somehow? Otherwise, holding onto a MessageContainer will also leak contexts.

EDIT: added some checkboxes so I remember what might still be open.

felixmulder · 2016-11-11T16:21:25Z

Just to update this - I've been hacking on this throughout the day. But I'm not ready to update the PR yet, still have a few kinks to work out.

Blaisorblade · 2016-11-14T15:32:15Z

src/dotty/tools/dotc/parsing/Parsers.scala

@@ -99,7 +99,7 @@ object Parsers {
    /** Issue an error at given offset if beyond last error offset
      *  and update lastErrorOffset.
      */
-    def syntaxError(msg: Message, offset: Int = in.offset): Unit =
+    def syntaxError(msg: => Message, offset: Int = in.offset): Unit =


👍 to this whole commit (without having rechecked other occurrences of Message).

Thanks! I discussed with @odersky and we agreed that my other approach was a tad over-engineered...hehe

WDYT @odersky? LGTY?

(Note that questions on StoreReporter and so on are still probably on, but all that commit does makes sense).

Yes, the store reporter will force if debugging - but I think we can live with that :)

FWIW, IMHO we can live with sharp edges as long as we document them when we notice them, and you already added some comments in this direction so 👍.
I'm just asking for a warning comment on the store reporter lest people use it in other scenarios, as I think Murphy's law applies to software evolution. Copy-pasting this would be enough, if drafting the text is the problem:

/** Beware this can leak memory, see https://github.com/lampepfl/dotty/pull/1696#issuecomment-259739205 */

As soon as the restructuring repo PR is merged - I wanted to make the StoreReporter private to the compiler - WDYT?

But sure, maybe it could use another comment :)

But sure, maybe it could use another comment :)

👍 . "Private to the compiler" doesn't help compiler contributors, neither core ones nor contributors outside EPFL. I also had no clue StoreReporter is for debugging only.

I'm also confused by the pushback — ideally one documents more pitfalls, not fewer (or even better, makes the comments unnecessary by removing them).

I'm aware there are bad comments, and double-checked checklists of comment downsides (on http://wiki.c2.com/?CommentCostsAndBenefits)—and I'm not sure I see a downside here (other than "time to write", which is why I offered text).

Yeah, didn't mean to push back - but at home right now ^^. The PR has been updated to document this in a2354dd 👍

but at home right now ^^

Oh sorry didn't mean to interrupt. Only time bound I care for: comments are best addressed before the issue/PR is closed, or they should be moved to a new issue lest they're forgot.

odersky · 2016-11-17T09:02:51Z

LGTM 👍

Make sure messages are lazily evaluated until report in Reporter

93d3f12

felixmulder added area:reporting Error reporting including formatting, implicit suggestions, etc stat:needs review labels Nov 10, 2016

Blaisorblade reviewed Nov 10, 2016

View reviewed changes

Blaisorblade mentioned this pull request Nov 10, 2016

Performance regression with Error Message classes #1691

Closed

felixmulder force-pushed the topic/assert-message-laziness branch 2 times, most recently from e24be23 to 93d3f12 Compare November 14, 2016 12:06

Make sure all Message creation is by name

f147aed

Blaisorblade reviewed Nov 14, 2016

View reviewed changes

Add note detailing possible memory leak in StoreReporter

a2354dd

dottybot added the reviewed label Nov 17, 2016

felixmulder merged commit a0169b7 into scala:master Nov 17, 2016

felixmulder deleted the topic/assert-message-laziness branch November 17, 2016 09:08

felixmulder removed reviewed labels Nov 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure messages are lazily evaluated until `report` in `Reporter` #1696

Make sure messages are lazily evaluated until `report` in `Reporter` #1696

felixmulder commented Nov 10, 2016 •

edited

Loading

felixmulder commented Nov 10, 2016 •

edited

Loading

felixmulder commented Nov 10, 2016

odersky commented Nov 10, 2016

felixmulder commented Nov 10, 2016

Blaisorblade Nov 10, 2016

Blaisorblade Nov 10, 2016

felixmulder Nov 10, 2016

Blaisorblade Nov 10, 2016

felixmulder Nov 10, 2016

Blaisorblade commented Nov 10, 2016 •

edited

Loading

felixmulder commented Nov 11, 2016

Blaisorblade Nov 14, 2016

felixmulder Nov 14, 2016

Blaisorblade Nov 14, 2016

felixmulder Nov 14, 2016

Blaisorblade Nov 14, 2016

felixmulder Nov 14, 2016 •

edited

Loading

Blaisorblade Nov 14, 2016

felixmulder Nov 14, 2016

Blaisorblade Nov 14, 2016

odersky commented Nov 17, 2016

Make sure messages are lazily evaluated until report in Reporter #1696

Make sure messages are lazily evaluated until report in Reporter #1696

Conversation

felixmulder commented Nov 10, 2016 • edited Loading

felixmulder commented Nov 10, 2016 • edited Loading

felixmulder commented Nov 10, 2016

odersky commented Nov 10, 2016

felixmulder commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented Nov 10, 2016 • edited Loading

felixmulder commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixmulder Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Nov 17, 2016

Make sure messages are lazily evaluated until `report` in `Reporter` #1696

Make sure messages are lazily evaluated until `report` in `Reporter` #1696

felixmulder commented Nov 10, 2016 •

edited

Loading

felixmulder commented Nov 10, 2016 •

edited

Loading

Blaisorblade commented Nov 10, 2016 •

edited

Loading

felixmulder Nov 14, 2016 •

edited

Loading