Skip to content

Fix #5386: Generate a synthetic val def and then apply the unary operator #5387

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.

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

Closed
wants to merge 6 commits into from

Conversation

biboudis
Copy link
Contributor

@biboudis biboudis commented Nov 4, 2018

I added the logic that lifts a block with a side-effectful operation at the InterceptedMethods phase. Is this the most appropriate place for that?

What happens is essentially:

~{
    println("!")
    1
}

is rewritten to ->

val temp = {
    println("!")
    1
}
!temp

@biboudis biboudis self-assigned this Nov 4, 2018
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

Open question: Should this happen earlier? What if I want to inspect the type of this block in a macro?

@biboudis biboudis assigned allanrenucci and unassigned biboudis Nov 5, 2018
@biboudis
Copy link
Contributor Author

biboudis commented Nov 5, 2018

@allanrenucci good question. What would be a recommended compiler phase?

@allanrenucci
Copy link
Contributor

allanrenucci commented Nov 5, 2018

@allanrenucci good question. What would be a recommended compiler phase?

I don't know that's why I am raising the question. Hopefully someone more knowledgeable will see the question 😄

cc/ @smarter @odersky

nicolasstucki
nicolasstucki previously approved these changes Nov 5, 2018
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

The implementation LGTM. Not sure if there is a better place for it.

@biboudis biboudis requested a review from smarter November 6, 2018 20:15
@biboudis biboudis assigned smarter and unassigned allanrenucci Nov 6, 2018
@biboudis biboudis dismissed allanrenucci’s stale review November 6, 2018 20:16

All suggestions where incorporated.

@@ -22,6 +23,7 @@ object InterceptedMethods {
* - `x.##` for ## in Any becomes calls to ScalaRunTime.hash,
* using the most precise overload available
* - `x.getClass` for getClass in primitives becomes `x.getClass` with getClass in class Object.
* - `!{...;true}` becomes val tmp = {...;true};!tmp (similar to other primitives/unary ops)
Copy link
Member

Choose a reason for hiding this comment

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

What other primitives ? Where is that defined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have written literal constants instead of primitives.

@@ -45,6 +47,16 @@ class InterceptedMethods extends MiniPhase {
ctx.log(s"$phaseName rewrote $tree to $rewritten")
rewritten
}
// if the qualifier is constant folded then its type doesn't have a symbol
Copy link
Member

Choose a reason for hiding this comment

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

but if its type doesn't have a symbol, does that always mean that the qualifier is constant-folded ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The qualifier is not constant folded due to the side effectful println.

@@ -45,6 +47,16 @@ class InterceptedMethods extends MiniPhase {
ctx.log(s"$phaseName rewrote $tree to $rewritten")
rewritten
}
// if the qualifier is constant folded then its type doesn't have a symbol
// in case a unary operator is applied to a block (potentially containing impure expressions)
// we create a synthetic variable and then apply the operator
Copy link
Member

Choose a reason for hiding this comment

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

What is special about unary operators here compared to any other method call on a block ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mean something like that?

  {
    println("!")
    1
  }+2

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this should have a more general solution, independent of the fact that these are operators. I.e. in general:

{ ... }.m where m has constant type v --> { ...; v }

@nicolasstucki nicolasstucki dismissed their stale review November 9, 2018 20:15

We should constant fold those cases

@odersky
Copy link
Contributor

odersky commented Nov 20, 2018

See #5479 for an another fix.

@biboudis
Copy link
Contributor Author

Solved by #5479.

@biboudis biboudis closed this Nov 23, 2018
@biboudis biboudis deleted the fix-#5386 branch November 23, 2018 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants