Fix #3917: Properly desugar `Ident` #3925

allanrenucci · 2018-01-26T13:12:50Z

No description provided.

odersky

Yep, that looks like the right fix.

allanrenucci · 2018-01-26T14:49:49Z

This fix doesn't work. We cannot always prefix identifier by this:

object Foo {
  var verbose = false
}

object Bar {
  def test = {
    import Foo._
    verbose = true // cannot be prefixed by this
  }
}

allanrenucci · 2018-01-26T17:43:37Z

Someone might want to take over this PR. I am not sure what is the proper way to fix the issue

smarter · 2018-01-26T20:24:22Z

We have code to transform Ident into Select, that may help: https://github.com/lampepfl/dotty/blob/02252a870f7eb510656632dca6d0f005d8754ee8/compiler/src/dotty/tools/dotc/transform/localopt/Simplify.scala#L152-L160

smarter · 2018-01-26T21:42:36Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

+        assert(setter.exists, tree.symbol.showLocated)
+        val qual = tree match {
+          case id: Ident =>
+            id.tpe match {


There's already three versions of desugarIdent in the codebase. I suggest we try to replace them by a common version in tpd.scala or something like that instead of adding one more variation.

I am only interested in the prefix of the desugared ident. Should I still extract the functionality and do something like:

val qual = tree match { case id: Ident => desugarIdent(id).get.qual ...

This would create an unnecessary Option and Select node

we could have a desugarIdentPrefix method.

odersky

Otherwise LGTM

odersky · 2018-01-30T08:37:35Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

@@ -1012,5 +1011,21 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
    val encoding = ctx.settings.encoding.value
    if (file != null && file.exists) new SourceFile(file, Codec(encoding)) else NoSource
  }
+
+  /** Desugar identifier into a select node. Return None if not possible */


We don't usually use Option in the compiler, first, because of the allocation overhead, and second, because we usually have sentinel values that signal absence of a proper result. E.g. EmptyTree for trees. So I would just let desugarIdent return an EmptyTree if no rules apply. That's more idiomatic. Hint: orElse works on empty trees.

I see there are quite a lot of uses of desugarIdent, and have the impression that most of these uses could be improved by going to EmptyTree. The use of Option.fold in JSCodeGen is particularly opaque.

allanrenucci · 2018-01-30T15:12:59Z

@OlivierBlanvillain Can you please review the last commit?

allanrenucci · 2018-01-30T15:14:09Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

@@ -444,8 +444,12 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
  def desugarIdent(i: Ident): Option[tpd.Select] = {


I could not get rid of the Option here since the backend expects this signature

allanrenucci · 2018-01-30T15:15:22Z

compiler/src/dotty/tools/dotc/transform/localopt/Simplify.scala

    case _ if t.symbol.is(Mutable) => true
-    case s: Select => s.symbol.owner == defn.SystemModule
-    case i: Ident  =>
-      desugarIdent(i) match {


I did not understand why desugaring was needed here because it doesn't change the symbol of the tree

allanrenucci · 2018-01-30T15:16:10Z

compiler/src/dotty/tools/dotc/transform/localopt/Devalify.scala

@@ -194,7 +194,7 @@ class Devalify extends Optimisation {
      readingOnlyVals(qual)

    case t: Ident if !t.symbol.is(Mutable | Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
-      desugarIdent(t).forall(readingOnlyVals)
+      !isEffectivelyMutable(t)


Did not understand why desugaring was needed here too

I would keep the logic untouched then:

desugarIdent(t) match { case s: Select => readingOnlyVals(s); case _ => true }

allanrenucci · 2018-01-30T15:17:54Z

compiler/src/dotty/tools/dotc/transform/localopt/ConstantFold.scala

-            case _ => t2
-          }
-          isSimilar(t, t2i)
-        case None => t1.symbol eq t2.symbol


I think the logic was wrong here. If t2 is a select then it is not enough to compare the symbols

Blaisorblade · 2018-03-01T16:40:25Z

Does this mean we can close #1686, though with a different fix than planned then?

allanrenucci · 2018-03-02T17:36:34Z

This doesn't really fix #1686. It gives you a way to desugar Ident into Select but it doesn't hide the distinction between Select and Ident. I think we can close with revisit though

odersky approved these changes Jan 26, 2018

View reviewed changes

smarter closed this Jan 26, 2018

smarter reopened this Jan 26, 2018

allanrenucci force-pushed the fix-3917 branch from 9f7bb74 to 3857489 Compare January 26, 2018 21:37

allanrenucci changed the title ~~Fix #3917: Prefix setter call by this when applied on Ident~~ Fix #3917: Properly desugar Ident Jan 26, 2018

smarter requested changes Jan 26, 2018

View reviewed changes

Fix scala#3917: Properly desugar Ident

6bd882e

allanrenucci force-pushed the fix-3917 branch from 3857489 to 6bd882e Compare January 26, 2018 21:59

Factor desugarIdent functionality

469fefe

smarter approved these changes Jan 27, 2018

View reviewed changes

allanrenucci requested a review from odersky January 29, 2018 09:15

odersky approved these changes Jan 30, 2018

View reviewed changes

allanrenucci force-pushed the fix-3917 branch from 5e7a56b to 8652469 Compare January 30, 2018 14:54

allanrenucci requested a review from OlivierBlanvillain January 30, 2018 15:13

allanrenucci commented Jan 30, 2018

View reviewed changes

OlivierBlanvillain approved these changes Jan 30, 2018

View reviewed changes

Make desugarIdent return a Tree

2d71030

allanrenucci force-pushed the fix-3917 branch from 8652469 to 2d71030 Compare January 30, 2018 16:43

allanrenucci merged commit 0a05275 into scala:master Jan 30, 2018

allanrenucci deleted the fix-3917 branch January 30, 2018 17:56

Blaisorblade mentioned this pull request May 30, 2018

MatchError in code gen emitting annotation with Ident arg #4551

Closed

Blaisorblade mentioned this pull request Aug 26, 2018

RuntimeError in backend / erasure #5006

Closed

		@@ -444,8 +444,12 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
		def desugarIdent(i: Ident): Option[tpd.Select] = {

Fix #3917: Properly desugar Ident #3925

Fix #3917: Properly desugar Ident #3925

Uh oh!

Conversation

allanrenucci commented Jan 26, 2018

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci commented Jan 26, 2018

Uh oh!

smarter commented Jan 26, 2018

Uh oh!

smarter Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 26, 2018 • 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!

smarter Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

odersky Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Jan 30, 2018

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jan 30, 2018 • 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!

Blaisorblade commented Mar 1, 2018

Uh oh!

allanrenucci commented Mar 2, 2018

Uh oh!

Uh oh!

Fix #3917: Properly desugar `Ident` #3925

Fix #3917: Properly desugar `Ident` #3925

allanrenucci commented Jan 26, 2018 •

edited

Loading

allanrenucci Jan 26, 2018 •

edited

Loading

allanrenucci Jan 30, 2018 •

edited

Loading