Skip to content

Conversation

@noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Feb 4, 2022

Fix #14319

When transforming a Block, the last expression should have import information from statements above.

@noti0na1
Copy link
Member Author

noti0na1 commented Feb 4, 2022

The akka build fails.

Accoding to tests/neg/3559.scala, it seems they are valid errors? I'm not sure about this.

@som-snytt
Copy link
Contributor

som-snytt commented Feb 8, 2022

IDK but there are default args in self invocation at the akka failures.

case class WeirdCtor(i: Int, s: Option[String] = None, b: Boolean = true) {
  def this(n: Int) = this(n, b = n > 0)
}

is

    def <init>(n: Int): Unit =
      {
        {
          val b$1: Boolean = n.>(0)
          val s$1: Option[String] @uncheckedVariance = WeirdCtor.$lessinit$greater$default$2
          this(n, s$1, b = b$1)
        }
        ()
      }

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I am not sure what this is supposed to achieve. The previous fixes should already do the right thing. Maybe it's just ExtractAPI that needs to be fixed here.

@odersky odersky assigned noti0na1 and unassigned odersky Feb 9, 2022
@olhotak
Copy link
Contributor

olhotak commented Feb 9, 2022

I am not sure what this is supposed to achieve. The previous fixes should already do the right thing. Maybe it's just ExtractAPI that needs to be fixed here.

A Block consists of stats followed by an expr. Before this PR, MegaPhase was fixed to thread contexts through the stats, but not the final expr. The expr was transformed using the original context from before the stats. The purpose of this PR is to use the context that comes out of the last of the stats as the context for the expr.

@odersky
Copy link
Contributor

odersky commented Feb 9, 2022

Thanks for explaining! Yes, I agree that makes sense. But it needs a different fix that does not append a list of statements on the right. And, one would need to troubleshoot why tests are failing.

@noti0na1
Copy link
Member Author

noti0na1 commented Feb 9, 2022

@odersky I will delete the changes to MegaPhase in this PR. The test should be fixed only by the changes in TreeMapWithPreciseStatContexts.

I will think about how to handle the last expr in MegaPhase at a seperate PR.

Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

Now LGTM.

class TreeMapWithPreciseStatContexts(cpy: TreeCopier = tpd.cpy) extends TreeMap(cpy):
override def transform(tree: Tree)(using Context): Tree = tree match
case Block(stats, expr) =>
val stats1 = transformStats(stats :+ expr, ctx.owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to great tree node churn. Every block will get new statements on every transform, so everything containing a block will be copied, which means you get a complete copy of the AST for a macro transform. I don't think this is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to have an optional expr argument in transformStats. It's more convoluted but I don't see an alternative.

@noti0na1
Copy link
Member Author

A better fix at #14523

@noti0na1 noti0na1 closed this Feb 21, 2022
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.

extractAPI fails with unsafeNulls and Yexplicit-nulls

4 participants