Fix #4947: Do not replace positions of inlined arguments #4949

nicolasstucki · 2018-08-15T16:14:39Z

No description provided.

Blaisorblade · 2018-08-15T17:54:17Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+              pos = last
+              res
+            case tree =>
+              if (pos.exists) super.transform(tree).withPos(pos)


@nicolasstucki says !pos.exists is only possible if inlined.call is emptyTree, which means that this is not a call to an inline method but an argument to it — and then bindings should be empty.
We should update docs for Inlined about this.

This does not look like the right algorithm. We have enclosingInlineds, which is interpreted as follows:

An Inlined node with a non-empty call is a plain inlined node

An inlined node with an empty tree as call in an inlined argument which cancels the enclosing inlined call.

So one should start with the outermost InlinedCall and update all positions in its expansion to the call position, except in those regions where there are as many empty as non-empty calls in enclosingInlineds.

That sounds like the algorithm I implemented though I use the pos state to know if it has been cancelled or not. I will change it to use enclosingInlineds directly.

smarter · 2018-08-16T11:27:07Z

Please add documentation, I have no idea what this does from looking at it.

odersky · 2018-08-16T14:25:20Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+              pos = last
+              res
+            case tree =>
+              if (pos.exists) super.transform(tree).withPos(pos)


This does not look like the right algorithm. We have enclosingInlineds, which is interpreted as follows:

An Inlined node with a non-empty call is a plain inlined node

An inlined node with an empty tree as call in an inlined argument which cancels the enclosing inlined call.

So one should start with the outermost InlinedCall and update all positions in its expansion to the call position, except in those regions where there are as many empty as non-empty calls in enclosingInlineds.

smarter · 2018-08-16T15:11:54Z

I think this also needs more extensive tests covering various nesting levels of inlined/non-inlined across one or more files and probably more low-level tests too (like https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/nsc/backend/jvm/BytecodeTest.scala#L32-L77, https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala#L1515, etc)

Reimplemented

Only reposition positions of inlined trees when they come from another file.

nicolasstucki · 2018-08-17T08:37:01Z

@Blaisorblade I when a bit than #4947 and kept the positions of trees inlined in the same file. Those already have the correct position and can be used in the debugger or stack traces.

Blaisorblade · 2018-08-17T16:38:31Z

I went a bit further than #4947 and kept the positions of trees inlined in the same file.

Sounds like what I hoped for!* Line numbers in debugging now work like a charm (minus, of course, setting breakpoints inside track, but that requires JSR-45). The only weird thing is that IntelliJ thinks you can set breakpoint in track, which suggests it is output at all, but that's an unfortunate UX for the semantics of transparent/@forceInline.

*EDIT: oh, I see that you preserve line numbers even for track if it's defined in the same file. Nice!

Blaisorblade

Cool, but I'm taking a look and changing a couple things which can be improved, and will reassign you for re-review.

Blaisorblade · 2018-08-22T15:41:57Z

tests/run/i4947.check

@@ -0,0 +1,4 @@
+track: Test$.main(i4947.scala:4)


These testcases are good and go beyond #4947.

Blaisorblade · 2018-08-22T16:11:54Z

compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala

+  @Test def i4947 = {
+    val source = """class Foo {
+                   |  transparent def track[T](f: => T): T = {
+                   |    println("tracking") // line 3


What about we call a new virtual method m1(String) in Foo, rather than println, a bit like the existing test? This test will break if any code figures that Predef$.MODULE$ needn't be reloaded.

Do these tests ever break? (Guess: yes). I don't know and I'm curious about the existing guidelines, but people maintaining them might like info on what bits matter and which don't (beyond what's already in this PR).

Nicolas mentioned he already tried this but the resulting test was more complex (which... well, ALOAD 0 seems pretty stable to me), and it's easy to fix the test by pasting the output.

Blaisorblade · 2018-08-22T17:07:59Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+              val transformed = super.transform(tree)
+              enclosingInlineds match {
+                case call :: _ if call.symbol.topLevelClass != ctx.owner.topLevelClass =>
+                  // reposition tree inlined from some other file


The comment makes sense, but the code is stricter, and resets the position even if the toplevel class is different but the source file is the same. In that case, it appears we don't need to reset the position. Fixing.

Also, this is an expensive (O(n)) way to test if enclosingInlineds.nonEmpty and get the head, and that could be expensive, but that's really enclosingInlineds's fault and I think can be fixed separately without affecting this code.

Blaisorblade · 2018-08-22T18:15:44Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

+              val transformed = super.transform(tree)
+              enclosingInlineds match {
+                case call :: _ if call.symbol.sourceFile != ctx.owner.sourceFile =>
+                  // reposition tree inlined from some other file


I think the documentation is too complicated. We're dropping Inlined nodes, and repositioning nodes from other files to the call position because we can't express their position otherwise in their debug info, pending JSR-45. The stuff about "empty call" is confusing and expressed in terms of the implementation.

Other issues that I think are there and that I'm trying to address:

The comment makes sense, but the code is stricter, and resets the position even if the toplevel class is different but the source file is the same. In that case, it appears we don't need to reset the position. Addressed by one commit I pushed.

Also, this is an expensive (O(n)) way to test if enclosingInlineds.nonEmpty and get the head, and that could be expensive, but that's really enclosingInlineds's fault and I think can be fixed separately without affecting this code.

So #4990 addresses performance.

Blaisorblade · 2018-08-23T12:50:30Z

@nicolasstucki The current algorithm is fishy; if I keep the full stack as described in #4990 (comment) (not the PR but the comment), the new tests break.

What we need is to compare the current source file ctx.owner.sourceFile with the source file of parameters, which is call.symbol.sourceFile for the enclosing call (not theirs).

I'm not sure Martin's suggested algorithm is correct either, or if so I don't get why:

So one should start with the outermost InlinedCall and update all positions in its expansion to the call position, except in those regions where there are as many empty as non-empty calls in enclosingInlineds.

It seems to keep unchanged the position of parameters (as many empty as non-empty calls), but that's wrong for parameters from another file (also featuring in the latest testcases).

EDIT: BTW, tests keep working with #4990 in.

EDIT2: I conjecture this algorithm works only assuming that enclosingInlineds is empty for parameters.

All the requested changes where made

nicolasstucki · 2018-08-24T09:40:45Z

@Blaisorblade I'll let you merge if all is in order for you.

Blaisorblade · 2018-08-24T16:36:40Z

@nicolasstucki I think I figured what's going on and why the code is correct. I'll update the comments and merge everything.

Blaisorblade · 2018-08-24T20:11:08Z

I'm merging the PRs as-is and pushing comment clarifications to followup PRs.

Fix scala#4947: Do not replace positions of inlined arguments

24b0d82

Blaisorblade self-assigned this Aug 15, 2018

Blaisorblade reviewed Aug 15, 2018

View reviewed changes

odersky previously requested changes Aug 16, 2018

View reviewed changes

Nicolas Stucki added 2 commits August 16, 2018 17:37

Use enclosingInlineds directly to know the position

3a6b56b

Add docs

21f7aa3

nicolasstucki force-pushed the fix-#4947 branch from 2b4d85b to 21f7aa3 Compare August 16, 2018 15:57

nicolasstucki added 4 commits August 17, 2018 09:23

Add bytecode tests and fix position of transitivly inlined trees

10ce2e8

Add more tests

0ff3fd7

Fix typo

2837365

Add more tests

857f53e

Retain positions of trees inlined from the same file

a11db37

Only reposition positions of inlined trees when they come from another file.

nicolasstucki force-pushed the fix-#4947 branch from 9b96b78 to a11db37 Compare August 17, 2018 08:30

Check that all trasnparent methods have been erased

a12688a

nicolasstucki added the stat:needs review label Aug 17, 2018

nicolasstucki and others added 2 commits August 17, 2018 11:17

Remove useless check

53e85d6

Add topLevelClass and do not assume transformation of posttyper

d52c3b2

nicolasstucki requested a review from Blaisorblade August 17, 2018 18:44

Blaisorblade mentioned this pull request Aug 20, 2018

Fix #4939: Add rewrite type match construct. #4962

Closed

Blaisorblade added 3 commits August 22, 2018 19:03

Test recursive transparent method

27ab7ee

Only reposition trees from other files, not other classes

f043599

Typo

e21ae3f

Blaisorblade previously requested changes Aug 22, 2018

View reviewed changes

Blaisorblade reviewed Aug 22, 2018

View reviewed changes

Blaisorblade mentioned this pull request Aug 22, 2018

Simplify enclosedInlineds #4990

Merged

nicolasstucki and others added 4 commits August 23, 2018 11:38

Use local method

cc6ca5f

Improve some docs

dc66cce

Complicate tests

bf1ae93

Test parameters from other files

7850b1e

nicolasstucki requested a review from odersky August 24, 2018 09:26

nicolasstucki assigned odersky and unassigned Blaisorblade Aug 24, 2018

odersky approved these changes Aug 24, 2018

View reviewed changes

nicolasstucki assigned Blaisorblade and unassigned odersky Aug 24, 2018

Use correct API ctx.source

4c51bc3

Blaisorblade merged commit 675b2ac into scala:master Aug 24, 2018

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

Blaisorblade deleted the fix-#4947 branch August 24, 2018 20:11

Blaisorblade mentioned this pull request Aug 24, 2018

enclosingInlineds: Clarify invariants #5009

Closed

Blaisorblade added a commit to dotty-staging/dotty that referenced this pull request Aug 27, 2018

Demonstrate regression in scala#4949 for positions of quote-inlined code

6cf40ee

Blaisorblade mentioned this pull request Aug 27, 2018

Demonstrate regression in #4949 for positions of quote-inlined code #5048

Closed

nicolasstucki pushed a commit to dotty-staging/dotty that referenced this pull request Aug 28, 2018

Demonstrate regression in scala#4949 for positions of quote-inlined code

b77530b

nicolasstucki pushed a commit to dotty-staging/dotty that referenced this pull request Sep 4, 2018

Demonstrate regression in scala#4949 for positions of quote-inlined code

ef5dcb4

nicolasstucki pushed a commit to dotty-staging/dotty that referenced this pull request Sep 4, 2018

Demonstrate regression in scala#4949 for positions of quote-inlined code

3467ebc

nicolasstucki pushed a commit to dotty-staging/dotty that referenced this pull request Nov 23, 2018

Demonstrate regression in scala#4949 for positions of quote-inlined code

0335197

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4947: Do not replace positions of inlined arguments #4949

Fix #4947: Do not replace positions of inlined arguments #4949

nicolasstucki commented Aug 15, 2018

Blaisorblade Aug 15, 2018

odersky Aug 16, 2018

nicolasstucki Aug 16, 2018

smarter commented Aug 16, 2018

odersky Aug 16, 2018

smarter commented Aug 16, 2018

nicolasstucki commented Aug 17, 2018

Blaisorblade commented Aug 17, 2018 •

edited

Loading

Blaisorblade left a comment

Blaisorblade Aug 22, 2018

Blaisorblade Aug 22, 2018

nicolasstucki Aug 23, 2018

Blaisorblade Aug 22, 2018

Blaisorblade Aug 22, 2018

Blaisorblade Aug 22, 2018

Blaisorblade Aug 22, 2018

Blaisorblade commented Aug 23, 2018 •

edited

Loading

nicolasstucki commented Aug 24, 2018

Blaisorblade commented Aug 24, 2018

Blaisorblade commented Aug 24, 2018

Fix #4947: Do not replace positions of inlined arguments #4949

Fix #4947: Do not replace positions of inlined arguments #4949

Conversation

nicolasstucki commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter commented Aug 16, 2018

Choose a reason for hiding this comment

smarter commented Aug 16, 2018

nicolasstucki commented Aug 17, 2018

Blaisorblade commented Aug 17, 2018 • edited Loading

Blaisorblade left a 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

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 Aug 23, 2018 • edited Loading

nicolasstucki commented Aug 24, 2018

Blaisorblade commented Aug 24, 2018

Blaisorblade commented Aug 24, 2018

Blaisorblade commented Aug 17, 2018 •

edited

Loading

Blaisorblade commented Aug 23, 2018 •

edited

Loading