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

Integrate macro paradise under -Ymacro-annotations #6606

Merged
merged 1 commit into from
May 10, 2018

Conversation

adriaanm
Copy link
Contributor

@adriaanm adriaanm commented May 6, 2018

Importing from scalamacros/paradise, since xeno-by has stepped down
as maintainer. Since the eco-system has come to rely on macro paradise,
we acknowledge the status quo by integrating paradise.

HOWEVER, please note that support for macro annotations remains
EXPERIMENTAL, and support for them will be much more restricted
in Scala 3. We will try to provide a preview of this subset in 2.14.

Most notably, we do not intend to let macro annotations
synthesize code that is visible during synthesis. Concretely,
you'd have to first compile the macros, then the code that
uses them as annotations, and finally the code that needs to
see the code synthesized by the annotation macros. This is
the same workflow as usual with code generation.

The tests are included as a new project (macroAnnot),
which uses the quick compiler as a scalaInstance to compile
the tests, so that we could integrate the test suite more easily.

Finally, note the diff re: List toStream flatMap headOption:
its behavior changed with the new collections.

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone May 6, 2018
@adriaanm
Copy link
Contributor Author

adriaanm commented May 6, 2018

/cc @xeno-by

@milessabin
Copy link
Contributor

Bravo!

@hrhino
Copy link
Member

hrhino commented May 6, 2018

Indeed! 🎉 Is the -Y flag temporary?

@adriaanm
Copy link
Contributor Author

adriaanm commented May 7, 2018

My current thinking is that it will remain under a -Y flag, as much as I would like to get rid of those. The implementation is pretty hairy, and affects the core of type checking in namers. (That's why we kept it in a plugin originally.)

@SethTisue
Copy link
Member

SethTisue commented May 7, 2018

Agree on the -Y, it's a marker that it remains permanently-experimental-until-Scala-3-at-which-point-things-change.

Copy link
Member

@xeno-by xeno-by left a comment

Choose a reason for hiding this comment

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

Thank you for giving macro paradise a new home! I am ultra thrilled to see this happening 😍

val global: Global.this.type = Global.this
} with Analyzer
lazy val analyzer =
if (settings.YmacroAnnotations) new { val global: Global.this.type = Global.this } with Analyzer with MacroAnnotationNamers
Copy link
Member

Choose a reason for hiding this comment

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

🎉


override def newNamer(context: Context): Namer = new MacroAnnotationNamer(context)

private class MacroAnnotationNamer(context: Context) extends Namer(context) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this class (as well as the accompanying Typer) publicized? In our SemanticDB compiler plugin, we are hijacking namers and typers to retain some information about tree originals, so we'll need to inherit from this class to be compatible with macro annotations (https://github.com/scalameta/scalameta/blob/v3.7.3/semanticdb/scalac/library/src/main/scala-2.11.11/scala/meta/internal/semanticdb/scalac/SemanticdbAnalyzer.scala#L20).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, but I am planning to rework this code for M5


def MacroAnnotationNotExpandedMessage = {
"macro annotation could not be expanded " +
"(the most common reason for that is that you need to enable the macro paradise plugin; " +
Copy link
Member

Choose a reason for hiding this comment

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

Am I right in thinking that there will no longer be a need in the plugin, and this is just a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed

else NoSymbol
}
// def isAccessible(cx: Context, sym: Symbol) = if (canDefineMann(cx.owner)) cx.isAccessible(sym, cx.prefix, superAccess = false) else false
def isAccessible(cx: Context, sym: Symbol) = true // TODO: sorry, it's 2am, and I can't figure this out
Copy link
Member

Choose a reason for hiding this comment

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

I am excited to see this comment making it to scala/scala :)

Importing from scalamacros/paradise, since xeno-by has stepped down
as maintainer. Since the eco-system has come to rely on macro paradise,
we acknowledge the status quo by integrating paradise.

HOWEVER, please note that support for macro annotations remains
EXPERIMENTAL, and support for them will be much more restricted
in Scala 3. We will try to provide a preview of this subset in 2.14.

Most notably, we do not intend to let macro annotations
synthesize code that is visible during synthesis. Concretely,
you'd have to first compile the macros, then the code that
uses them as annotations, and finally the code the needs to
see the code synthesized by the annotation macros. This is
the same workflow as usual with code generation.

The tests are included as a new project (macroAnnot),
which uses the quick compiler as a scalaInstance to compile
the tests, so that we could integrate the test suite more easily.

Finally, note the diff re: `List toStream flatMap headOption`:
its behavior changed with the new collections.
@adriaanm adriaanm changed the title wip: bring paradise down to earth Integrate macro paradise under -Ymacro-annotations May 8, 2018
@adriaanm adriaanm requested a review from lrytz May 8, 2018 12:46
} else None
} else None

annZippers.iterator.flatMap(annz => maybeExpand(annz.annotation, annz.annottee, annz.owner)).nextOption match {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szeiger this used to be annZippers.toStream.flatMap(annz => maybeExpand(annz.annotation, annz.annottee, annz.owner)).headOption, which fails in macroAnnot/testOnly Multiple

@adriaanm

This comment has been minimized.

@soronpo

This comment has been minimized.

@adriaanm
Copy link
Contributor Author

adriaanm commented May 8, 2018

Since I didn't preserve history, here's the core of the diff: https://gist.github.com/adriaanm/bdaec0588ad6fdf3bbe6ea09d31edf87

produced with

cat plugin/src/main/scala/org/scalamacros/paradise/typechecker/Namers.scala plugin/src/main/scala/org/scalamacros/paradise/typechecker/Expanders.scala > /tmp/para-namers.scala
diff -uw  /tmp/para-namers.scala ../scala/src/compiler/scala/tools/nsc/typechecker/MacroAnnotationNamers.scala

@adriaanm

This comment has been minimized.

@SethTisue

This comment has been minimized.

@adriaanm

This comment has been minimized.

@SethTisue

This comment has been minimized.

@SethTisue
Copy link
Member

SethTisue commented May 9, 2018

a 2.13 community build run on this is here: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1095/ . (it works by enabling -Ymacro-annotations globally and removing paradise from libraryDependencies globally)

the run shows:

  • enabling -Ymacro-annotations everywhere didn't break anything
  • two paradise-using projects went green: macro-compat (which has a "bundle" macro), minitest (which has a "source location" macro)
  • all failing projects are failing for collections reasons, not paradise reasons

in my opinion, this is the best we can expect at this stage, and this PR should be merged, with the expectation that it may need refinement later. it is at least minimally functional — the 2.13 community build can't currently tell us more than that. we can find out more, post-M4.

the only way I can see that we could get substantially better feedback on this, right now, would be to backport this PR to 2.12 and run the 2.12 community build on it.

@soronpo
Copy link

soronpo commented May 9, 2018

the only way I can see that we could get substantially better feedback on this, right now, would be to backport this PR to 2.12 and run the 2.12 community build on it.

Is it not easier to create a fork of 2.13-M3 and apply this PR on it to test the community build on?

@adriaanm
Copy link
Contributor Author

adriaanm commented May 9, 2018

I do intend to backport this to 2.12, hopefully in time for 2.12.7, but that has lower priority than 2.13 milestone tasks, which will keep me plenty busy for the coming months :-)

@SethTisue
Copy link
Member

Is it not easier to create a fork of 2.13-M3 and apply this PR on it to test the community build on?

(probably, but even the 2.13-community-build-as-of-2.13.0-M3 didn't have anywhere near as many projects in it as the 2.12 one)

@adriaanm adriaanm merged commit c4adc81 into scala:2.13.x May 10, 2018
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 14, 2018
@raulraja
Copy link

I just want to say thank you! This is huge for Freestyle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
9 participants