WIP: Using `Silent` case class to support suppressing output of repl commands #3048

3shv · 2017-08-31T17:23:12Z

felixmulder · 2017-09-01T08:48:01Z

Let's continue the discussion here:

Thanks for the help. I've submitted a WIP PR, to check if I'm going in right direction. If I understood marker traits correctly, I need to declare a trait, say Concrete and attach it to all Parsed, SyntaxErrors, NewLine etc right?
Say,
case class Parsed(...) extends ParseResult with Concrete

Also I'm getting this warning, even though I am unpacking the inner object and required Silent can't inherit Silent
[warn] It would fail on the following input: Silent(_) [warn] parseResult match {

Well, actually "it could", because your hierarchy says that silent is a subclass of ParseResult and Concrete is just an unaffiliated trait that you've marked the other subclasses with. What I'd do is something like this:

trait Parsing
trait ParseResult extends Parsing

case class Silent(res: ParseResult) extends Parsing

case class Parsed(...) extends ParseResult
case object NewLine extends ParseResult
...

This makes the subclasses of ParseResult exclude Silent and you don't get the exhaustivity warnings.

3shv · 2017-09-01T17:10:50Z

Thanks for the explanation. It works now without any warnings. I have added support for initial & cleanup commands. I made them Array[String] since they can be multiple. And didn't touch the bindValues as I couldn't get what they are meant for. I have a doubt:

Should the resetToInitial preserve the state of initialCommands or not?

Please have a look when you have time.

allanrenucci · 2017-09-01T19:52:41Z

Instead of introducing a marker trait, why not simply have run and interpret take a boolean flag to decide whether or not to run silently?
The new class hierarchy looks weird to me.

allanrenucci · 2017-09-01T19:54:02Z

compiler/src/dotty/tools/repl/ReplDriver.scala

@@ -141,12 +141,24 @@ class ReplDriver(settings: Array[String],
    }
  }

+  final def runWrapper(initialState: State = initState): State = {


I would not expose this as public API. I would move it into runUntilQuit.

I modified runUntilQuit now. Please have a look

allanrenucci · 2017-09-01T19:56:56Z

compiler/src/dotty/tools/repl/ReplDriver.scala

-        displayErrors,
+        {
+          case (errors: Errors) =>
+            if (!silent) displayErrors(errors)


I think we want to display error even in silent mode

allanrenucci · 2017-09-01T19:59:26Z

compiler/src/dotty/tools/repl/ReplDriver.scala

+    }
+
+    def isSyntheticCompanion(sym: Symbol) =
+      sym.is(Module) && sym.is(Synthetic)


sym.is(Module | Synthetic)

Changing sym.is(Module) && sym.is(Synthetic) to sym.is(Module | Synthetic) made some test cases fail. So I didn't use this as of now

This should not be the case. Could you try again with sym.is(Module | Synthetic). If that does not work you might have found a bug.

I tried again. This is the error

scala> Test.foo(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109) val res0: Int = 42 actual ===========> scala> object Test { def foo(p1: Int, p2: Int, p3: Int, p4: Int, p5: Int, p6: Int, p7: Int, p8: Int, p9: Int, p10: Int, p11: Int, p12: Int, p13: Int, p14: Int, p15: Int, p16: Int, p17: Int, p18: Int, p19: Int, p20: Int, p21: Int, p22: Int, p23: Int, p24: Int, p25: Int, p26: Int, p27: Int, p28: Int, p29: Int, p30: Int, p31: Int, p32: Int, p33: Int, p34: Int, p35: Int, p36: Int, p37: Int, p38: Int, p39: Int, p40: Int, p41: Int, p42: Int, p43: Int, p44: Int, p45: Int, p46: Int, p47: Int, p48: Int, p49: Int, p50: Int, p51: Int, p52: Int, p53: Int, p54: Int, p55: Int, p56: Int, p57: Int, p58: Int, p59: Int, p60: Int, p61: Int, p62: Int, p63: Int, p64: Int, p65: Int, p66: Int, p67: Int, p68: Int, p69: Int, p70: Int, p71: Int, p72: Int, p73: Int, p74: Int, p75: Int, p76: Int, p77: Int, p78: Int, p79: Int, p80: Int, p81: Int, p82: Int, p83: Int, p84: Int, p85: Int, p86: Int, p87: Int, p88: Int, p89: Int, p90: Int, p91: Int, p92: Int, p93: Int, p94: Int, p95: Int, p96: Int, p97: Int, p98: Int, p99: Int, p100: Int, p101: Int, p102: Int, p103: Int, p104: Int, p105: Int, p106: Int, p107: Int, p108: Int, p109: Int): Int = 42 } scala> Test.foo(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109) val res0: Int = 42 [error] Test dotty.tools.repl.ScriptedTests.replTests failed: java.lang.AssertionError: Error in file /Users/rveeranki/code/dotty/compiler/target/scala-2.12/test-classes/repl/functions, expected output did not match actual, took 1.259 sec

Basically Object Foo doesn't print anything.

It looks like you're not printing the definition of Foo for some reason, right?

Have a look at: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/repl/ReplDriver.scala#L226

I even tried on current master by changing sym.is(Module) && sym.is(Synthetic) to sym.is(Module | Synthetic)
Repl goes silent for object Foo however if I change it to sym.is(Module & Synthetic) it prints // defined object foo. So it looks like this behaviour is not introduced by these changes.

allanrenucci · 2017-09-01T20:03:53Z

compiler/src/dotty/tools/repl/ReplDriver.scala

          .withHistory(parsed.sourceCode :: state.history)
          .newRun(compiler, rootCtx)

      case SyntaxErrors(src, errs, _) =>
-        displayErrors(errs)
+        if (!isSilent) displayErrors(errs)


I think we want to display errors even in silent mode

felixmulder · 2017-09-01T22:31:49Z

@allanrenucci: my 2 centimes are that Boolean parameters are code smells. I'd rather encode correctness on the type level than adding additional checks to the logic.

There's one Boolean parameter in the ReplCompiler, but I think we can and should get rid of it.

Otherwise I agree with your review 👍

allanrenucci · 2017-09-04T07:52:20Z

compiler/src/dotty/tools/repl/ReplDriver.scala

+          case (errors: Errors) =>
+            displayErrors(errors)
+            state
+        },


You can revert back to simply displayErrors.

allanrenucci · 2017-09-04T08:07:53Z

@felixmulder True. But a flag is introduced anyway for compile and interpretCommand. We should be consistent.

felixmulder · 2017-09-04T08:12:05Z

@allanrenucci - I agree, there shouldn't be a need :)

3shv · 2017-09-04T08:30:29Z

Thanks @allanrenucci and @felixmulder for the review.
Running test:testOnly dotty.tools.repl.* succeeds in my local machine. However, it seems to be failing here in CI.
I can update the code to remove the new case classes and use boolean flags if you guys agree.

allanrenucci · 2017-09-04T09:24:37Z

sbt-bridge/src/xsbt/ConsoleInterface.scala

@@ -51,6 +33,6 @@ class ConsoleInterface {
      } ++


Try matching the signature here. That might fix your failing tests.

I changed the signature of run to match. Still no luck :(

felixmulder · 2017-09-04T09:30:22Z

Raj: IMO remove the Boolean parameters and rethink the call logic. Maybe the interpret method should know what to print to, or maybe it should return something else that can be given a printer in order to print, or otherwise just throw away.

Boolean parameters are smelly because they change the nature of the definition IMO, adding multiple paths through the method. Often, if you feel the need to have them, you can break apart the method to make it compose into parts where you don't need the Boolean.

But those are my two cents, in the end this PR improves the REPL functionality and as such it is super appreciated and if you don't feel you want to dig in that far - we'll take care of it in a follow-up :)

allanrenucci · 2017-09-04T12:44:36Z

scalac doesn't let you put command in initialCommands (e.g. :help), only scala code. So I think we should treat initialCommands as a single instruction to interpret.

@felixmulder scalac repl quits when given invalid initialCommands. What do we want to do in our case?

felixmulder · 2017-09-04T12:45:29Z

@allanrenucci - I reckon we should print the error an quit as well :)

allanrenucci · 2017-09-05T07:08:49Z

We updated the CI. Please rebase your PR on top of master.

3shv · 2017-09-05T17:34:20Z

I have rebased with the current master. Still some errors pop up

[error] -- [E045] Syntax Error: /drone/src/github.com/lampepfl/dotty/pull/3048/compiler/test/dotty/tools/repl/CompilerTests.scala:17:4 
[error] 17 |    compiler.compile("def foo: 1 = 1").stateOrFail
[error]    |    ^^^^^^^^
[error]    |    cyclic reference involving trait ParseResult
[error] one error found

allanrenucci · 2017-09-06T08:47:57Z

I think you are hitting a bug in Dotty

allanrenucci · 2017-09-22T15:58:09Z

@Rajesh-Veeranki If you can open an issue with a minimised test case that reproduces the bug you are encountering, it would be awesome!

odersky · 2017-11-13T16:53:23Z

Is there more work planned on this one or can we close it?

allanrenucci · 2017-11-13T17:57:26Z

I'll be refactoring some code in the REPL. Then there might be a better approach to do this.

However, it would be interesting to rebase this branch and see if the bug in Dotty you encountered is still present on master.

3shv · 2017-11-14T16:10:31Z

I rebased the branch. Now, a different set of tests seem to be failing.

allanrenucci · 2017-11-14T17:12:25Z

Your git submodules are out of date. Try running:

git submodule --init --recursive

OlivierBlanvillain · 2018-01-18T17:31:19Z

I will close this for now, @Rajesh-Veeranki if you find some time and interest in reviving this PR please reopen!

3shv force-pushed the support-supress-output-repl branch from 3b719a4 to f992c16 Compare September 1, 2017 16:52

allanrenucci reviewed Sep 1, 2017

View reviewed changes

3shv force-pushed the support-supress-output-repl branch 3 times, most recently from 16fa789 to 868edfd Compare September 4, 2017 07:42

allanrenucci reviewed Sep 4, 2017

View reviewed changes

3shv force-pushed the support-supress-output-repl branch from 868edfd to f1d9f9c Compare September 4, 2017 08:23

allanrenucci reviewed Sep 4, 2017

View reviewed changes

3shv force-pushed the support-supress-output-repl branch from 90e6038 to f448013 Compare September 5, 2017 05:11

3shv force-pushed the support-supress-output-repl branch 2 times, most recently from fa89883 to 5e7af96 Compare September 5, 2017 17:22

3shv force-pushed the support-supress-output-repl branch from 5e7af96 to 984a73d Compare September 15, 2017 14:55

3shv force-pushed the support-supress-output-repl branch from 984a73d to ff5d90e Compare September 22, 2017 15:14

Rajesh Veeranki added 2 commits November 14, 2017 22:52

Using Silent case class to support suppressing output of repl commands

720d291

Use Option[String] instead of Array[String] for initialCommands Repl

e3d86c7

3shv force-pushed the support-supress-output-repl branch from ff5d90e to e3d86c7 Compare November 14, 2017 15:52

OlivierBlanvillain closed this Jan 18, 2018

WIP: Using Silent case class to support suppressing output of repl commands #3048

WIP: Using Silent case class to support suppressing output of repl commands #3048

Uh oh!

Conversation

3shv commented Aug 31, 2017

Uh oh!

felixmulder commented Sep 1, 2017

Uh oh!

3shv commented Sep 1, 2017

Uh oh!

allanrenucci commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3shv Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Sep 4, 2017

Uh oh!

felixmulder commented Sep 4, 2017

Uh oh!

3shv commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Sep 4, 2017

Uh oh!

allanrenucci commented Sep 4, 2017

Uh oh!

felixmulder commented Sep 4, 2017

Uh oh!

allanrenucci commented Sep 5, 2017

Uh oh!

3shv commented Sep 5, 2017 • edited by felixmulder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci commented Sep 6, 2017

Uh oh!

allanrenucci commented Sep 22, 2017

Uh oh!

odersky commented Nov 13, 2017

Uh oh!

allanrenucci commented Nov 13, 2017

Uh oh!

3shv commented Nov 14, 2017

Uh oh!

allanrenucci commented Nov 14, 2017

Uh oh!

OlivierBlanvillain commented Jan 18, 2018

Uh oh!

Uh oh!

WIP: Using `Silent` case class to support suppressing output of repl commands #3048

WIP: Using `Silent` case class to support suppressing output of repl commands #3048

3shv Sep 4, 2017 •

edited

Loading

3shv commented Sep 5, 2017 •

edited by felixmulder

Loading