Add annotations phase #1693

odersky · 2016-11-10T13:25:47Z

New phase for entering annotations

If we want to do annotation macros right, we need to add
annotations before completing definitions. This commit achieves
that by adding a new "phase" between index and typecheck.

Also: Fixes #1647 be tweaking the context in which we evaluate annotation argumnents

Review by @liufengyun or @olafurpg

felixmulder · 2016-11-10T16:24:22Z

src/dotty/tools/dotc/typer/ImportInfo.scala

+   *
+   *  TODO: Once we have fully bootstrapped, I would prefer if we expressed
+   *  unimport with an `override` modifier, and generalized it to all imports.
+   *  I believe this would be more transparent than the curren set of conditions. E.g.


Super-nitpick: curren => current :)

liufengyun

Another concern is about how annotation macros fit into this picture. Current implementation doesn't allow expansion during annotate phase, as it returns Unit. Adding/removing top-level definitions in macros can also be very tricky with indexing.

It seems a relative reliable way is to expand macros inside expand during indexing phase -- and we need to restrict that annotation macros can't be used in the same compilation run when they're defined.

liufengyun · 2016-11-11T00:33:23Z

src/dotty/tools/dotc/typer/Namer.scala

+      original match {
+        case original: MemberDef => addAnnotations(denot.symbol, original)
+        case _ =>
+      }


It's not clear to me why we need to addAnnotations again for ValDef and DefDef here, as they are already handled by a call to annotate when they are indexed (in ClassCompleter and typedBlockStats).

Is it just for breaking the cyclic dependence between Predef and deprecated in test compileStdLib as commented before? Then maybe a comment here as well.

Annotations, and classes or objects containing them, might be completed out of order as part of the annotations phase. It seems necessary that these symbols get annotated on completion, or otherwise we'd introduce a compilation order dependency.

liufengyun · 2016-11-11T00:57:31Z

src/dotty/tools/dotc/typer/Namer.scala

+    //
+    // `@ann` is evaluated in the context just outside `C`, where the `a.b`
+    // import is visible but the `d.e` import is forgotten. This measure is necessary
+    // in order to avoid cycles.


The lexical scope for @ann is very counter-intuitive. If it's only some rare erroneous programs cause cycles, it seems justified to keep the intuitive scoping rules as a trade off.

@odersky Can you provide an example of a program that leads to spurious cycles?

@xeno-by I had lots of failing test cases in the regression tests. They failed with CyclicReference errors. I did not keep track of which ones.

smarter · 2016-11-22T10:43:25Z

src/dotty/tools/dotc/repl/CompilingInterpreter.scala

@@ -6,7 +6,7 @@ import java.io.{
  File, PrintWriter, PrintStream, StringWriter, Writer, OutputStream,
  ByteArrayOutputStream => ByteOutputStream
 }
-import java.lang.{Class, ClassLoader}
+import java.lang.{Class, ClassLoader, Thread, System, StringBuffer}


Is this something that should be handled by the rewriting tool?

I'm missing context, what exactly do you mean by "this"?

See commit message "Honor the new scheme where any explicit import of a root import will disable the root import."

Yes, that sounds like a prime candidate for rewriting. I opened scalacenter/scalafix#21

smarter · 2016-11-22T10:50:56Z

src/dotty/tools/dotc/typer/Typer.scala

@@ -1135,7 +1137,13 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
  def completeAnnotations(mdef: untpd.MemberDef, sym: Symbol)(implicit ctx: Context): Unit = {
    // necessary to force annotation trees to be computed.
    sym.annotations.foreach(_.ensureCompleted)
-    val annotCtx = ctx.outersIterator.dropWhile(_.owner == sym).next
+    lazy val annotCtx = {


Would be nice to add a comment explaining why this is needed as the commit message does.

smarter · 2016-11-22T10:53:41Z

src/dotty/tools/dotc/typer/Namer.scala

+   *  that are already added to the symbol.
+   */
+  def addAnnotations(sym: Symbol, stat: MemberDef)(implicit ctx: Context) = {
+    // (1) The context in which an annotation of a top-evel class or module is evaluated


typo: top-evel -> top-level

smarter · 2016-11-22T10:54:12Z

src/dotty/tools/dotc/typer/Namer.scala

+    // (1) The context in which an annotation of a top-evel class or module is evaluated
+    // is the closest enclosing context which has the enclosing package as owner.
+    // (2) The context in which an annotation for any other symbol is evaluated is the
+    // closest enclosing context which has the owner of the class enclpsing the symbol as owner.


typo: enclpsing -> enclosing

smarter · 2016-11-22T10:56:27Z

src/dotty/tools/dotc/repl/CompilingInterpreter.scala

@@ -6,7 +6,7 @@ import java.io.{
  File, PrintWriter, PrintStream, StringWriter, Writer, OutputStream,
  ByteArrayOutputStream => ByteOutputStream
 }
-import java.lang.{Class, ClassLoader}
+import java.lang.{Class, ClassLoader, Thread, System, StringBuffer}


See commit message "Honor the new scheme where any explicit import of a root import will disable the root import."

odersky · 2016-11-24T15:52:06Z

Rebased to master

Had duplications of `import` before. Also: add import info when printing contexts.

We got a hard to track down error when changing to the new annotations elaboration scheme (should be in the next commit): When running `testNonCyclic`, `DotClass` was not found in object Trees even though it was imported from `util`. It turned out that the import was ignored because the `util` symbol was completing. This commit adds a warning when this happens. The warning currently applies only to named imports because several false negatives were encountered if we do this also on wildcard imports. I.e. we get a warning, but the searched after symbol is not a member of the wildcard qualifier. This commit also refactors namedImportRef, so that `site` is only computed when the name to reseolve appears in the selector list. That change made the previously observed error go away because less is now forced.

Honor the new scheme where any explicit import of a root import will disable the root import.

It was broken before, since it worked only on wildcard imports.

Got a "next on empty iterator" exception before.

If we want to do annotation macros right, we need to add annotations before completing definitions. This commit achieves that by adding a new "phase" between index and typecheck.

Need to evaluate annotation arguments in an expression context, since classes defined in asuch arguments should not be entered into enclosing class. Fixes scala#1647

This PR also fixes scala#1649

odersky added the stat:in progress label Nov 10, 2016

This was referenced Nov 10, 2016

Annotation macros in Dotty #1694

Closed

crash with assertion failed: duplicate attachment for key ... #1649

Closed

odersky added stat:needs review and removed stat:in progress labels Nov 10, 2016

felixmulder reviewed Nov 10, 2016

View reviewed changes

liufengyun reviewed Nov 11, 2016

View reviewed changes

smarter reviewed Nov 22, 2016

View reviewed changes

olafurpg mentioned this pull request Nov 22, 2016

Insert missing root imports scalacenter/scalafix#21

Closed

odersky force-pushed the add-annotations-phase branch from 1a07ff3 to bf84725 Compare November 24, 2016 15:51

odersky added 8 commits December 1, 2016 13:55

Better printing of import symbols

7263d25

Had duplications of `import` before. Also: add import info when printing contexts.

Clean up imports

3469039

Honor the new scheme where any explicit import of a root import will disable the root import.

Fix import disabling

36f9133

It was broken before, since it worked only on wildcard imports.

Make phasesStack more robust

4ae76f2

Got a "next on empty iterator" exception before.

New phase for entering annotations

2b12868

If we want to do annotation macros right, we need to add annotations before completing definitions. This commit achieves that by adding a new "phase" between index and typecheck.

Fix scala#1647 Evaluate annotation arguments in proper context

4f15dfa

Need to evaluate annotation arguments in an expression context, since classes defined in asuch arguments should not be entered into enclosing class. Fixes scala#1647

Additional test case

9a3f82c

This PR also fixes scala#1649

odersky force-pushed the add-annotations-phase branch from bf84725 to e1d79a2 Compare December 1, 2016 13:01

Address reviewers comments

e1d79a2

odersky merged commit 47d2084 into scala:master Dec 1, 2016

odersky removed the stat:needs review label Dec 1, 2016

allanrenucci deleted the add-annotations-phase branch December 14, 2017 15:01

smarter mentioned this pull request Jan 10, 2018

SymDenotation.annotations fails to complete annotations of DefDef #341

Closed

Add annotations phase #1693

Add annotations phase #1693

Uh oh!

Conversation

odersky commented Nov 10, 2016

Uh oh!

felixmulder Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Nov 24, 2016

Uh oh!

Uh oh!

felixmulder Nov 10, 2016 •

edited

Loading