From 93d3f1201e00c0436d2b2bcd3445ba218e0e1a89 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Thu, 10 Nov 2016 15:18:53 +0100 Subject: [PATCH 1/3] Make sure messages are lazily evaluated until `report` in `Reporter` --- src/dotty/tools/dotc/reporting/Reporter.scala | 24 +++++---- .../tools/dotc/reporting/StoreReporter.scala | 2 +- .../dotc/reporting/diagnostic/Message.scala | 54 ++++++++++++++----- .../dotc/reporting/diagnostic/messages.scala | 2 +- .../dotc/reporting/TestMessageLaziness.scala | 37 +++++++++++++ 5 files changed, 93 insertions(+), 26 deletions(-) create mode 100644 test/dotty/tools/dotc/reporting/TestMessageLaziness.scala diff --git a/src/dotty/tools/dotc/reporting/Reporter.scala b/src/dotty/tools/dotc/reporting/Reporter.scala index 49bd3e81113e..60ed18c7126d 100644 --- a/src/dotty/tools/dotc/reporting/Reporter.scala +++ b/src/dotty/tools/dotc/reporting/Reporter.scala @@ -38,16 +38,16 @@ trait Reporting { this: Context => reporter.report(new Info(msg, pos)) def deprecationWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.deprecationWarning(pos)) + reporter.report(new DeprecationWarning(msg, pos)) def migrationWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.migrationWarning(pos)) + reporter.report(new MigrationWarning(msg, pos)) def uncheckedWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.uncheckedWarning(pos)) + reporter.report(new UncheckedWarning(msg, pos)) def featureWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.featureWarning(pos)) + reporter.report(new FeatureWarning(msg, pos)) def featureWarning(feature: String, featureDescription: String, isScala2Feature: Boolean, featureUseSite: Symbol, required: Boolean, pos: SourcePosition): Unit = { @@ -73,23 +73,27 @@ trait Reporting { this: Context => } def warning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.warning(pos)) + reporter.report(new Warning(msg, pos)) def strictWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = if (this.settings.strict.value) error(msg, pos) - else warning(msg.mapMsg(_ + "\n(This would be an error under strict mode)"), pos) + else reporter.report { + new ExtendMessage(() => msg)(_ + "\n(This would be an error under strict mode)").warning(pos) + } def error(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - reporter.report(msg.error(pos)) + reporter.report(new Error(msg, pos)) def errorOrMigrationWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = if (ctx.scala2Mode) migrationWarning(msg, pos) else error(msg, pos) def restrictionError(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit = - error(msg.mapMsg(m => s"Implementation restriction: $m"), pos) + reporter.report { + new ExtendMessage(() => msg)(m => s"Implementation restriction: $m").error(pos) + } def incompleteInputError(msg: Message, pos: SourcePosition = NoSourcePosition)(implicit ctx: Context): Unit = - reporter.incomplete(msg.error(pos))(ctx) + reporter.incomplete(new Error(msg, pos))(ctx) /** Log msg if settings.log contains the current phase. * See [[config.CompilerCommand#explainAdvanced]] for the exact meaning of @@ -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 = if (!isHidden(d)) { doReport(d)(ctx.addMode(Mode.Printing)) d match { diff --git a/src/dotty/tools/dotc/reporting/StoreReporter.scala b/src/dotty/tools/dotc/reporting/StoreReporter.scala index e85017ed2bb5..bde034845cc4 100644 --- a/src/dotty/tools/dotc/reporting/StoreReporter.scala +++ b/src/dotty/tools/dotc/reporting/StoreReporter.scala @@ -31,7 +31,7 @@ class StoreReporter(outer: Reporter) extends Reporter { override def flush()(implicit ctx: Context) = if (infos != null) { - infos foreach ctx.reporter.report + infos.foreach(ctx.reporter.report(_)) infos = null } diff --git a/src/dotty/tools/dotc/reporting/diagnostic/Message.scala b/src/dotty/tools/dotc/reporting/diagnostic/Message.scala index 8b1f6567309f..8018a877715f 100644 --- a/src/dotty/tools/dotc/reporting/diagnostic/Message.scala +++ b/src/dotty/tools/dotc/reporting/diagnostic/Message.scala @@ -6,6 +6,8 @@ package diagnostic import util.SourcePosition import core.Contexts.Context +import messages._ + object Message { /** This implicit conversion provides a fallback for error messages that have * not yet been ported to the new scheme. Comment out this `implicit def` to @@ -20,6 +22,13 @@ object Message { * into a `MessageContainer` which contains the log level and can later be * consumed by a subclass of `Reporter`. * + * NOTE: you should not be persisting messages. Most messages take an implicit + * `Context` and these contexts weigh in at about 4mb per instance, as such + * persisting these will result in a memory leak. + * + * Instead use the `persist` method to create an instance that does not keep a + * reference to these contexts. + * * @param errorId a unique number identifying the message, this will later be * used to reference documentation online */ @@ -47,45 +56,62 @@ abstract class Message(val errorId: Int) { self => */ def explanation: String - /** It is possible to map `msg` to add details, this is at the loss of - * precision since the type of the resulting `Message` won't be original - * extending class - * - * @return a `Message` with the mapped message + /** The implicit `Context` in messages is a large thing that we don't want + * persisted. This method gets around that by duplicating the message + * without the implicit context being passed along. */ - def mapMsg(f: String => String) = new Message(errorId) { - val msg = f(self.msg) + def persist: Message = new Message (errorId) { + val msg = self.msg + val kind = self.kind + val explanation = self.explanation + } +} + +/** An extended message keeps the contained message from being evaluated, while + * allowing for extension for the `msg` string + * + * This is useful when we need to add additional information to an existing + * message. + */ +class ExtendMessage(_msg: () => Message)(f: String => String) { self => + lazy val msg = f(_msg().msg) + lazy val kind = _msg().kind + lazy val explanation = _msg().explanation + lazy val errorId = _msg().errorId + + private def toMessage = new Message(errorId) { + val msg = self.msg val kind = self.kind val explanation = self.explanation } /** Enclose this message in an `Error` container */ def error(pos: SourcePosition) = - new Error(self, pos) + new Error(toMessage, pos) /** Enclose this message in an `Warning` container */ def warning(pos: SourcePosition) = - new Warning(self, pos) + new Warning(toMessage, pos) /** Enclose this message in an `Info` container */ def info(pos: SourcePosition) = - new Info(self, pos) + new Info(toMessage, pos) /** Enclose this message in an `FeatureWarning` container */ def featureWarning(pos: SourcePosition) = - new FeatureWarning(self, pos) + new FeatureWarning(toMessage, pos) /** Enclose this message in an `UncheckedWarning` container */ def uncheckedWarning(pos: SourcePosition) = - new UncheckedWarning(self, pos) + new UncheckedWarning(toMessage, pos) /** Enclose this message in an `DeprecationWarning` container */ def deprecationWarning(pos: SourcePosition) = - new DeprecationWarning(self, pos) + new DeprecationWarning(toMessage, pos) /** Enclose this message in an `MigrationWarning` container */ def migrationWarning(pos: SourcePosition) = - new MigrationWarning(self, pos) + new MigrationWarning(toMessage, pos) } /** The fallback `Message` containing no explanation and having no `kind` */ diff --git a/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index 649f2a8f8db0..1693fc786bdf 100644 --- a/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -282,7 +282,7 @@ object messages { val explanation = "" } - case class EarlyDefinitionsNotSupported()(implicit ctx:Context) + case class EarlyDefinitionsNotSupported()(implicit ctx: Context) extends Message(9) { val kind = "Syntax" val msg = "early definitions are not supported; use trait parameters instead" diff --git a/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala b/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala new file mode 100644 index 000000000000..fe85edda6bb5 --- /dev/null +++ b/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala @@ -0,0 +1,37 @@ +package dotty.tools +package dotc +package reporting + +import org.junit.Assert._ +import org.junit.Test + +import core.Contexts._ + +import test.DottyTest + +import diagnostic.{ Message, MessageContainer, ExtendMessage } + +class TestMessageLaziness extends DottyTest { + ctx = ctx.fresh.setReporter(new NonchalantReporter) + + class NonchalantReporter(implicit ctx: Context) extends Reporter + with UniqueMessagePositions with HideNonSensicalMessages { + def doReport(d: MessageContainer)(implicit ctx: Context) = ??? + + override def report(d: => MessageContainer)(implicit ctx: Context) = () + } + + case class LazyError() extends Message(1000) { + throw new Error("Didn't stay lazy.") + + val kind = "Test" + val msg = "Please don't blow up" + val explanation = "" + } + + @Test def assureLazy = + ctx.error(LazyError()) + + @Test def assureLazyExtendMessage = + ctx.strictWarning(LazyError()) +} From f147aedd65407056e0682e3c15f5a7265b96ffa1 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 14 Nov 2016 13:22:17 +0100 Subject: [PATCH 2/3] Make sure all `Message` creation is by name --- src/dotty/tools/dotc/parsing/Parsers.scala | 14 +++++----- src/dotty/tools/dotc/reporting/Reporter.scala | 26 +++++++++---------- .../dotc/reporting/TestMessageLaziness.scala | 4 +-- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/dotty/tools/dotc/parsing/Parsers.scala b/src/dotty/tools/dotc/parsing/Parsers.scala index f442c13b3713..974d98d6ae84 100644 --- a/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/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 = if (offset > lastErrorOffset) { syntaxError(msg, Position(offset)) lastErrorOffset = in.offset @@ -108,7 +108,7 @@ object Parsers { /** Unconditionally issue an error at given position, without * updating lastErrorOffset. */ - def syntaxError(msg: Message, pos: Position): Unit = + def syntaxError(msg: => Message, pos: Position): Unit = ctx.error(msg, source atPos pos) } @@ -215,23 +215,23 @@ object Parsers { } } - def warning(msg: Message, sourcePos: SourcePosition) = + def warning(msg: => Message, sourcePos: SourcePosition) = ctx.warning(msg, sourcePos) - def warning(msg: Message, offset: Int = in.offset) = + def warning(msg: => Message, offset: Int = in.offset) = ctx.warning(msg, source atPos Position(offset)) - def deprecationWarning(msg: Message, offset: Int = in.offset) = + def deprecationWarning(msg: => Message, offset: Int = in.offset) = ctx.deprecationWarning(msg, source atPos Position(offset)) /** Issue an error at current offset taht input is incomplete */ - def incompleteInputError(msg: Message) = + def incompleteInputError(msg: => Message) = ctx.incompleteInputError(msg, source atPos Position(in.offset)) /** If at end of file, issue an incompleteInputError. * Otherwise issue a syntax error and skip to next safe point. */ - def syntaxErrorOrIncomplete(msg: Message) = + def syntaxErrorOrIncomplete(msg: => Message) = if (in.token == EOF) incompleteInputError(msg) else { syntaxError(msg) diff --git a/src/dotty/tools/dotc/reporting/Reporter.scala b/src/dotty/tools/dotc/reporting/Reporter.scala index 60ed18c7126d..8477cfe287ab 100644 --- a/src/dotty/tools/dotc/reporting/Reporter.scala +++ b/src/dotty/tools/dotc/reporting/Reporter.scala @@ -92,7 +92,7 @@ trait Reporting { this: Context => new ExtendMessage(() => msg)(m => s"Implementation restriction: $m").error(pos) } - def incompleteInputError(msg: Message, pos: SourcePosition = NoSourcePosition)(implicit ctx: Context): Unit = + def incompleteInputError(msg: => Message, pos: SourcePosition = NoSourcePosition)(implicit ctx: Context): Unit = reporter.incomplete(new Error(msg, pos))(ctx) /** Log msg if settings.log contains the current phase. @@ -196,7 +196,7 @@ trait Reporting { this: Context => abstract class Reporter extends interfaces.ReporterResult { /** Report a diagnostic */ - def doReport(d: MessageContainer)(implicit ctx: Context): Unit + def doReport(m: MessageContainer)(implicit ctx: Context): Unit /** Whether very long lines can be truncated. This exists so important * debugging information (like printing the classpath) is not rendered @@ -240,22 +240,22 @@ abstract class Reporter extends interfaces.ReporterResult { override def default(key: String) = 0 } - def report(d: => MessageContainer)(implicit ctx: Context): Unit = - if (!isHidden(d)) { - doReport(d)(ctx.addMode(Mode.Printing)) - d match { - case d: ConditionalWarning if !d.enablingOption.value => unreportedWarnings(d.enablingOption.name) += 1 - case d: Warning => warningCount += 1 - case d: Error => - errors = d :: errors + def report(m: MessageContainer)(implicit ctx: Context): Unit = + if (!isHidden(m)) { + doReport(m)(ctx.addMode(Mode.Printing)) + m match { + case m: ConditionalWarning if !m.enablingOption.value => unreportedWarnings(m.enablingOption.name) += 1 + case m: Warning => warningCount += 1 + case m: Error => + errors = m :: errors errorCount += 1 - case d: Info => // nothing to do here + case m: Info => // nothing to do here // match error if d is something else } } - def incomplete(d: MessageContainer)(implicit ctx: Context): Unit = - incompleteHandler(d)(ctx) + def incomplete(m: MessageContainer)(implicit ctx: Context): Unit = + incompleteHandler(m)(ctx) /** Summary of warnings and errors */ def summary: String = { diff --git a/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala b/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala index fe85edda6bb5..6892739e8f1c 100644 --- a/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala +++ b/test/dotty/tools/dotc/reporting/TestMessageLaziness.scala @@ -16,9 +16,9 @@ class TestMessageLaziness extends DottyTest { class NonchalantReporter(implicit ctx: Context) extends Reporter with UniqueMessagePositions with HideNonSensicalMessages { - def doReport(d: MessageContainer)(implicit ctx: Context) = ??? + def doReport(m: MessageContainer)(implicit ctx: Context) = ??? - override def report(d: => MessageContainer)(implicit ctx: Context) = () + override def report(m: MessageContainer)(implicit ctx: Context) = () } case class LazyError() extends Message(1000) { From a2354dd7e347a191c6077105b136d683013dd092 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 14 Nov 2016 21:14:37 +0100 Subject: [PATCH 3/3] Add note detailing possible memory leak in `StoreReporter` --- src/dotty/tools/dotc/reporting/StoreReporter.scala | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/dotty/tools/dotc/reporting/StoreReporter.scala b/src/dotty/tools/dotc/reporting/StoreReporter.scala index bde034845cc4..586273c2e29d 100644 --- a/src/dotty/tools/dotc/reporting/StoreReporter.scala +++ b/src/dotty/tools/dotc/reporting/StoreReporter.scala @@ -8,9 +8,16 @@ import config.Printers.typr import diagnostic.MessageContainer import diagnostic.messages._ -/** - * This class implements a Reporter that stores all messages - */ +/** This class implements a Reporter that stores all messages + * + * Beware that this reporter can leak memory, and force messages in two + * scenarios: + * + * - During debugging `config.Printers.typr` is set from `noPrinter` to `new + * Printer`, which forces the message + * - The reporter is not flushed and the message containers capture a + * `Context` (about 4MB) + */ class StoreReporter(outer: Reporter) extends Reporter { private var infos: mutable.ListBuffer[MessageContainer] = null