Skip to content
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

Fix #14319: Use correct stat context when transforming Block #14411

Closed
wants to merge 2 commits into from

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.

compiler/src/dotty/tools/dotc/transform/MegaPhase.scala Outdated Show resolved Hide resolved
@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.

@@ -1195,6 +1195,13 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
* - imports are reflected in the contexts of subsequent statements
*/
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