Skip to content

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

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.

Sign up for GitHub

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 3 commits into from
Nov 17, 2016
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93d3f12
Make sure messages are lazily evaluated until `report` in `Reporter`
felixmulder Nov 10, 2016
f147aed
Make sure all `Message` creation is by name
felixmulder Nov 14, 2016
a2354dd
Add note detailing possible memory leak in `StoreReporter`
felixmulder Nov 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/dotty/tools/dotc/parsing/Parsers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

WDYT @odersky? LGTY?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */

Copy link
Contributor Author

@felixmulder felixmulder Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (offset > lastErrorOffset) {
syntaxError(msg, Position(offset))
lastErrorOffset = in.offset
Expand All @@ -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)

}
Expand Down Expand Up @@ -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)
Expand Down
26 changes: 13 additions & 13 deletions src/dotty/tools/dotc/reporting/Reporter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 = {
Expand Down
4 changes: 2 additions & 2 deletions test/dotty/tools/dotc/reporting/TestMessageLaziness.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Expand Down