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

Unencode quote and splice trees #17342

Merged
merged 46 commits into from
May 2, 2023
Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Apr 25, 2023

We currently use the Quote and Splice ASTs for untyped quotes and splices. Then when we type them we encode them into scala.quoted.runtime.Expr.{quote,splice,nestedSplice}. This non-semantic representation if fragile and the source of many past bug. In this PR we change the internal representation of quotes and splices to use the Quote and Splice ASTs as typed trees.

The core of this change in the AST representation and how we type them in QuotesAndSplices. Other changes consist in adapting the code from one representation to the other.

- case class Quote(quoted: Tree)(implicit @constructorOnly src: SourceFile) extends TermTree
+ case class Quote[+T <: Untyped] private[ast] (body: Tree[T])(implicit @constructorOnly src: SourceFile) extends TermTree[T] 

- case class Splice(expr: Tree)(implicit @constructorOnly src: SourceFile) extends TermTree 
+ case class Splice[+T <: Untyped] private[ast] (expr: Tree[T])(implicit @constructorOnly src: SourceFile) extends TermTree[T] 
  • We do not change the TASTy (file and reflection) representation of quotes and splices. This is something that we should consider adding in a future PR.
  • We can use the simpler encoding of splice in all cases. The additional Quotes in nestedSplice is not used.
  • This does not change binary compatibility.

@nicolasstucki nicolasstucki self-assigned this Apr 25, 2023
@nicolasstucki nicolasstucki marked this pull request as ready for review April 26, 2023 11:34
@nicolasstucki nicolasstucki requested a review from smarter April 26, 2023 11:34
@@ -398,6 +398,9 @@ trait TypeAssigner {
.appliedTo(defn.QuotesClass.typeRef, defn.QuotedExprClass.typeRef.appliedTo(tpt.tpe))
tree.withType(lambdaType)

def assignType(tree: untpd.SplicedExpr, tpt: Tree)(using Context): SplicedExpr =
tree.withType(tpt.tpe)
Copy link
Member

Choose a reason for hiding this comment

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

If tpt is always the same as the type of the node, and if tpt is always EmptyTree in untyped trees, then it shouldn't be necessary and can be removed (possibly using promote in TreeChecker to retype it).

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 was wondering how the tpt could be removed. I will try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently use the tpt in the staging/splicing phases to set staged healed types for quotes and splices. I did not manage to remove it without breaking something in those phases.

Copy link
Member

Choose a reason for hiding this comment

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

I did not manage to remove it without breaking something in those phases.

We have other phases where we transform types of trees like https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala, does that not work here? Maybe I can have a look if you show me what the issues you ran into were.

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 managed to remove the tpt.

compiler/src/dotty/tools/dotc/ast/Trees.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Trees.scala Outdated Show resolved Hide resolved
/** A tree representing a quote `'{ expr }
*
* @param expr The tree that was quoted
* @param tpt The type of the tree that was quoted,
Copy link
Member

Choose a reason for hiding this comment

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

How does this differ from expr.tpe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the staging phase we use tpt to store the healed version of the type. This cannot be recomputed trivially from expr.tpe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now staging uses expr.tpe to store the new version of type.

compiler/src/dotty/tools/dotc/transform/ElimByName.scala Outdated Show resolved Hide resolved
@smarter smarter assigned nicolasstucki and unassigned smarter Apr 26, 2023
@smarter smarter removed their assignment May 1, 2023
@nicolasstucki nicolasstucki force-pushed the unencode-quote-trees branch from f4dd16f to 9214daa Compare May 2, 2023 06:50
@nicolasstucki
Copy link
Contributor Author

I updated the check-files in 95c39bc to fix the CI failures.

@nicolasstucki nicolasstucki requested a review from smarter May 2, 2023 06:52
@nicolasstucki nicolasstucki linked an issue May 2, 2023 that may be closed by this pull request
@smarter smarter merged commit ec3cdad into scala:main May 2, 2023
@smarter smarter deleted the unencode-quote-trees branch May 2, 2023 12:36
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Jun 27, 2023
nicolasstucki added a commit that referenced this pull request Jun 27, 2023
Closes #18059

Probably fixed by #17342

[skip mima]
[skip docs]
[skip community_build]
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
Kordyjan pushed a commit that referenced this pull request Nov 23, 2023
Closes #18059

Probably fixed by #17342

[Cherry-picked 050f54b]
Kordyjan pushed a commit that referenced this pull request Nov 29, 2023
Closes #18059

Probably fixed by #17342

[Cherry-picked 050f54b]
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.

Misleading error message when matching on quotes
3 participants