Pretty-printing: handle type-operator precedence correctly #4072

Blaisorblade · 2018-03-05T00:41:21Z

Fix #4068 (pretty-printing precedence for function arrows).

And pretty-print type operators by handling precedence and associativity more correctly: that's the biggest part of the PR.

This also encapsulate the precedence of type operators, and don't assume it's the same as the one for term operators. It will be if SIP-33's implementation is merged, but since this flexibility is cheap, I'd rather keep it till we're sure it can be simplified.

The PR has been rebased and edited so that reviewing by commit should be feasible, but you'll want to start from the new documentation for atPrec and changePrec.

I didn't document it just for fun, but because they must be used in an extremely precise way, and the existing code showed the correct approach wasn't clear to past maintainers. If one doesn't use precedence correctly, one must handle pretty-printing for all combinations of operators.

https://github.com/lampepfl/dotty/pull/4072/files#diff-cc3cc3d0c223249636132a7b11e4690d

I'm not as confident about my handling of associativity mismatches, but the code still needs at worst to handle each type constructor, not each combination, so it should be OK.

OlivierBlanvillain · 2018-05-01T19:46:29Z

Looks reasonable, what would you like to do better?

Blaisorblade · 2018-05-01T19:55:25Z

I wanted to use atPrec(...) but failed. Feel free to merge if you think it improves things, I can still fix it better later.

`argText` uses `toTextGlobal` to change priority; instead, call `toText(arg)` and leave the priority up to the caller. - call-sites where `GlobalPrec` makes sense use now `argsText`. - to ensure this is a refactoring, other call-sites now use `atPrec(GlobalPrec)` explicitly; all those calls will be corrected in subsequent commits.

Using `atPrec(GlobalPrec)` ensures that `argText(args.head)` never adds parentheses around its output, but that's inappropriate for a function type `T1 => T2`. The nested calls to `atPrec` make no sense, but `argText` hid that before it was refactored.

Encapsulate the precedence rules in `parsing.precedence` while allowing for different precedence between term- and type-level operators, as mandated by the spec. This is just a refactoring, the actual priority changes are separate. * Add an `isType` flag to `parsing.precedence` and add constants for `AndTypePrec` and `OrTypePrec` (distinct from `OrPrec` and `AndPrec`, which is now unused and dropped). * In the parser, consider `isType` to decide priority. * In the pretty-printer, use `AndTypePrec` and `OrTypePrec` where appropriate.

While the operators should be associative, they aren't actually guaranteed to be, so correct pretty-printing will help users. * PrinterTests: Add first test for TypeTree printing. * PrinterTests: disable colors, following ErrorMessagesTest.scala.

This way, printing an infix type need not inspect its arguments; that ensures we don't need to handle *all* the possible forms of types that need to be wrapped in parentheses. I did this purely as a refactoring, based on how pretty-printers are supposed to be written. But the new test case, combining infix type operators and builting connectives, didn't work with the old code. It also doesn't work in Scala 2: ```scala Welcome to Scala 2.12.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_172). Type in expressions for evaluation. Or try :help. scala> class &&[T,U] defined class $amp$amp scala> def foo: Int && Boolean = ??? foo: Int && Boolean scala> def foo: Int && Boolean with String = ??? foo: Int && Boolean with String scala> def foo: Int && (Boolean with String) = ??? foo: Int && Boolean with String scala> def foo: (Int && Boolean) with String = ??? foo: Int && Boolean with String ```

* RefinedPrinter: Handle associativity mismatches for Type operators. * Add Documentation/tutorial on how to use atPrec and changePrec correctly, since so much code got it wrong and it takes a while to get it. * Extend tests for Type printing.

Refines 8b941d6: classSymbol is unnecessarily aggressive here.

OlivierBlanvillain

Looks good!

OlivierBlanvillain · 2018-08-15T05:42:14Z

compiler/src/dotty/tools/dotc/printing/Printer.scala

@@ -17,10 +17,24 @@ abstract class Printer {

  private[this] var prec: Precedence = GlobalPrec

-  /** The current precedence level */
+  /** The current precedence level.
+   *  WHen pretty-printing arguments of operator `op`, `currentPrecedence` must equal `op`'s precedence level,


s/WHen/When/

allanrenucci · 2018-08-15T09:20:26Z

compiler/src/dotty/tools/dotc/parsing/package.scala

@@ -7,7 +7,7 @@ import core.NameOps._

 package object parsing {

-  def precedence(operator: Name): Int =
+  def precedence(operator: Name, isType: Boolean = false): Int =


isType unused?

allanrenucci · 2018-08-15T09:24:26Z

compiler/test-resources/type-printer/functions

+scala> val tupFun: ((Int, Int)) => Int = new { def apply(a: (Int, Int)) = 1; override def toString() = "<func4>" }
+val tupFun: ((Int, Int)) => Int = <func4>
+scala> val binFun: (Int, Int) => Int = new { def apply(a: Int, b: Int) = 1; override def toString() = "<func5>" }
+val binFun: (Int, Int) => Int = <func5>


Can we rewrite these tests as:

- scala> val toInt: Any => Int = new { def apply(a: Any) = 1; override def toString() = "<func1>" } - val toInt: Any => Int = <func1> + scala> def toInt: Any => Int = ??? + def toInt: Any => Int

There is too much noise with vals

allanrenucci · 2018-08-20T16:22:41Z

compiler/test-resources/type-printer/functions

@@ -1,2 +1,10 @@
-scala> val toInt: Any => Int = new { def apply(a: Any) = 1; override def toString() = "<func1>" }
-val toInt: Any => Int = <func1>
+scala> def toInt: Any => Int = new { def apply(a: Any) = 1; override def toString() = "<func1>" }


RHS can be ???. it would make the tests more readable. That was the point of changing to def 😄

aaah sorry i wasn't thinking fixed 😅

I verified the links use correct Dottydoc syntax.

Blaisorblade added the stat:wip label Mar 5, 2018

Blaisorblade self-assigned this Mar 5, 2018

Blaisorblade force-pushed the fix-#4068 branch 3 times, most recently from e490aa3 to 2fc99ca Compare July 30, 2018 22:07

This comment has been minimized.

Sign in to view

Blaisorblade mentioned this pull request Aug 5, 2018

Dotty uses SIP-12 syntax in output and input #4599

Closed

Blaisorblade changed the title ~~Fix precedence in pretty-printing~~ Fix precedence for infix type-operators (pretty-printing & parsing) Aug 5, 2018

Blaisorblade added 8 commits August 9, 2018 16:59

Seal Texts.Text

586527d

Bugfix: missing return

dac4265

Treat & and | as *left*-associative

a87418b

While the operators should be associative, they aren't actually guaranteed to be, so correct pretty-printing will help users. * PrinterTests: Add first test for TypeTree printing. * PrinterTests: disable colors, following ErrorMessagesTest.scala.

Blaisorblade force-pushed the fix-#4068 branch from ed8de0b to 6f2e39d Compare August 9, 2018 17:15

Use typeSymbol not classSymbol

5e01496

Refines 8b941d6: classSymbol is unnecessarily aggressive here.

Blaisorblade force-pushed the fix-#4068 branch from 6f2e39d to 5e01496 Compare August 9, 2018 17:17

Blaisorblade changed the title ~~Fix precedence for infix type-operators (pretty-printing & parsing)~~ Pretty-printing: handle type-operator precedence correctly Aug 9, 2018

Blaisorblade mentioned this pull request Aug 10, 2018

Tune precedence of type operators #4920

Closed

2 tasks

Blaisorblade added stat:needs review and removed stat:wip labels Aug 10, 2018

Blaisorblade requested a review from OlivierBlanvillain August 10, 2018 09:53

Blaisorblade assigned OlivierBlanvillain and unassigned Blaisorblade Aug 10, 2018

OlivierBlanvillain approved these changes Aug 15, 2018

View reviewed changes

allanrenucci reviewed Aug 15, 2018

View reviewed changes

Blaisorblade mentioned this pull request Aug 17, 2018

Dottydoc: Show implicit modifier in implicit function types #4954

Merged

Blaisorblade removed the stat:needs review label Aug 20, 2018

Blaisorblade assigned Blaisorblade and unassigned OlivierBlanvillain Aug 20, 2018

Fix typo

0333dd6

allanrenucci reviewed Aug 20, 2018

View reviewed changes

Blaisorblade added 3 commits August 20, 2018 20:45

Type-printer tests: simplify per review comment

dfdf9a1

Hardcode SIP-33: term and type operators have the same priority

de8eb4d

Give normative references for operator precedence

f9990c7

I verified the links use correct Dottydoc syntax.

Blaisorblade force-pushed the fix-#4068 branch from 950adde to f9990c7 Compare August 20, 2018 18:46

Blaisorblade assigned allanrenucci and Blaisorblade and unassigned Blaisorblade and allanrenucci Aug 20, 2018

Blaisorblade merged commit da2a245 into scala:master Aug 21, 2018

Blaisorblade deleted the fix-#4068 branch August 21, 2018 02:21

Blaisorblade mentioned this pull request Aug 21, 2018

Fixes to transparent to enable generic tuples #4916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pretty-printing: handle type-operator precedence correctly #4072

Pretty-printing: handle type-operator precedence correctly #4072

Uh oh!

Blaisorblade commented Mar 5, 2018 •

edited

Loading

Uh oh!

OlivierBlanvillain commented May 1, 2018

Uh oh!

Blaisorblade commented May 1, 2018

Uh oh!

This comment has been minimized.

OlivierBlanvillain left a comment

Uh oh!

OlivierBlanvillain Aug 15, 2018 •

edited

Loading

Uh oh!

allanrenucci Aug 15, 2018

Uh oh!

allanrenucci Aug 15, 2018

Uh oh!

allanrenucci Aug 20, 2018 •

edited

Loading

Uh oh!

Blaisorblade Aug 20, 2018

Uh oh!

Uh oh!

Pretty-printing: handle type-operator precedence correctly #4072

Pretty-printing: handle type-operator precedence correctly #4072

Uh oh!

Conversation

Blaisorblade commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain commented May 1, 2018

Uh oh!

Blaisorblade commented May 1, 2018

Uh oh!

This comment has been minimized.

OlivierBlanvillain left a comment

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain Aug 15, 2018 • 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!

allanrenucci Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Aug 20, 2018 • 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!

Blaisorblade Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Blaisorblade commented Mar 5, 2018 •

edited

Loading

OlivierBlanvillain Aug 15, 2018 •

edited

Loading

allanrenucci Aug 20, 2018 •

edited

Loading