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

Allow macro annotation to transform companion #19677

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

hamzaremmal
Copy link
Member

@hamzaremmal hamzaremmal commented Feb 13, 2024

Allow MacroAnnotations to update the companion of a definition

We extend the MacroAnnotation api to allow to modify the companion of a class or an object.

Specification

  1. Order of expansion
  • We expand the definitions in program order.
  • We expand the annotations of the outer scope first, then we expand the inner definitions.
  • Annotations are expanded from the outer annotation to the inner annotation.

In the following example, we expand the annotations in this order: a1, a2, a3.

@a1 @a2
class Foo:
  @a3 def foo = ???
  1. Expansion of the companion

We always expand the latest available tree. If an annotation defined on class Foo changes its companion (object Foo) and the class is defined before object, the expansion of the annotations on the object will be performed on the result of the expansion of class.

  1. The program order is maintained

We maintain the program order in the definitions that were expanded.

  1. Backtrack and reprocess

Example:

@a1 class Foo
@a2 object Foo

If the @a2 annotation changes the definitions in class Foo, we will rerun the algorithm on the result of this new expansion. Please note that we don't allow to generate code with MacroAnnotations, the reason for rerunning the algorithm is to expand and inline possible macros that we generated.


Closes #19676

@hamzaremmal hamzaremmal added area:experimental needs-minor-release This PR cannot be merged until the next minor release labels Feb 13, 2024
@Gedochao
Copy link
Contributor

Gedochao commented Apr 3, 2024

It has been decided to be included in the 3.5.0 release.

@hamzaremmal hamzaremmal force-pushed the macro-annot-class-object branch 5 times, most recently from ef74f4d to d2c3db0 Compare April 15, 2024 12:03
@hamzaremmal hamzaremmal force-pushed the macro-annot-class-object branch 2 times, most recently from d711455 to e81ef44 Compare April 16, 2024 16:15
@hamzaremmal hamzaremmal marked this pull request as ready for review April 16, 2024 16:27
@hamzaremmal
Copy link
Member Author

I will add support for stoping macro expansion in a second PR (this one is already contained and has many changes in the tests)

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.

I still have not looked into compiler/src/dotty/tools/dotc/transform/MacroAnnotations.scala

library/src/scala/annotation/MacroAnnotation.scala Outdated Show resolved Hide resolved
library/src/scala/annotation/MacroAnnotation.scala Outdated Show resolved Hide resolved
tests/neg-macros/i19676/Macro_1.scala Show resolved Hide resolved
tests/neg-macros/i18677-b/Macro_1.scala Outdated Show resolved Hide resolved
tests/neg-macros/i18677-a/Macro_1.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Inlining.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Inlining.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Inlining.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Inlining.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Inlining.scala Outdated Show resolved Hide resolved
}

// Interpret macro annotation instantiation `new myAnnot(..)`
// TODO: Make this error handling stronger (no error handling at the moment)
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of error handling?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if the interpretation of the annotation failed and returned None ? We might add a better handling here and avoid a possible compiler crash in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should fix this now. Maybe this would be enough. Just check that the interpretation error has been reported.

    val annotInstance = interpreter.interpret[MacroAnnotation](annot.tree) match
      case Some(annotInstance) => annotInstance
      case None => return (List(tree), companion)

Copy link
Member Author

Choose a reason for hiding this comment

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

We agreed to investigate this later

@hamzaremmal hamzaremmal force-pushed the macro-annot-class-object branch 2 times, most recently from 2cbc56f to 1c26cda Compare April 22, 2024 20:18
tests/pos-macros/macro-annot-with-companion/Macro_1.scala Outdated Show resolved Hide resolved
tests/neg-macros/i18677-b/Macro_1.scala Outdated Show resolved Hide resolved
tests/run-macros/i19676/Macro_1.scala Outdated Show resolved Hide resolved
@hamzaremmal hamzaremmal force-pushed the macro-annot-class-object branch from 1c26cda to 58fe2ac Compare April 23, 2024 13:23
@hamzaremmal hamzaremmal requested a review from jchyb April 23, 2024 14:54
}

// Interpret macro annotation instantiation `new myAnnot(..)`
// TODO: Make this error handling stronger (no error handling at the moment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should fix this now. Maybe this would be enough. Just check that the interpretation error has been reported.

    val annotInstance = interpreter.interpret[MacroAnnotation](annot.tree) match
      case Some(annotInstance) => annotInstance
      case None => return (List(tree), companion)

@hamzaremmal hamzaremmal force-pushed the macro-annot-class-object branch from 58fe2ac to 4694b3b Compare April 29, 2024 12:55
Copy link
Contributor

@jchyb jchyb left a comment

Choose a reason for hiding this comment

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

Looks good to me, I cannot find anything wrong here. Just one semi-related question. I can't find where do we check if the generated tree contains a macro annotation. Is that in TreeChecker?

@hamzaremmal
Copy link
Member Author

I can't find where do we check if the generated tree contains a macro annotation. Is that in TreeChecker?

Generated trees cannot have annotations. We do a second run to inline code in the new defintions if necessary (last paragraph in the description).

@nicolasstucki nicolasstucki merged commit 4f39236 into scala:main Apr 30, 2024
19 checks passed
@hamzaremmal hamzaremmal deleted the macro-annot-class-object branch April 30, 2024 12:35
pweisenburger added a commit to pweisenburger/scala3 that referenced this pull request May 7, 2024
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
pweisenburger added a commit to pweisenburger/scala3 that referenced this pull request May 10, 2024
KacperFKorban pushed a commit to dotty-staging/dotty that referenced this pull request Nov 20, 2024
KacperFKorban pushed a commit to dotty-staging/dotty that referenced this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:experimental needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow macro annotation to transform companion
5 participants