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

SI-9473 Cleaner references to statically owned symbols #4749

Merged
merged 1 commit into from Sep 22, 2015

Conversation

Projects
None yet
5 participants
@retronym
Member

retronym commented Sep 17, 2015

Ever wonder why identity("") typechecks to
scala.this.Predef.identity("")?

It turns out that mkAttributedRef was importing
q"$scalaPackageClass.this.Predef._" for all these years,
rather than q"$scalaModule.Predef._".

This commit makes mkAttributedRef special case static owners
by referring the the corresponding module, instead.

@scala-jenkins scala-jenkins added this to the 2.12.0-M3 milestone Sep 17, 2015

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 17, 2015

Member

Review by @lrytz

I tried an followup commit that uses more concise trees, so a reference to ScalaRuntime could just be Ident(ScalaRuntimeModule), rather than Select(Select(ScalaPackageClass.sourceModule, "runtime"), TermName("ScalaRuntime")).

This broke some assumptions about trees in some tests, so I didn't pursue it further.

Member

retronym commented Sep 17, 2015

Review by @lrytz

I tried an followup commit that uses more concise trees, so a reference to ScalaRuntime could just be Ident(ScalaRuntimeModule), rather than Select(Select(ScalaPackageClass.sourceModule, "runtime"), TermName("ScalaRuntime")).

This broke some assumptions about trees in some tests, so I didn't pursue it further.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Sep 18, 2015

Member

the test output suggests that a qualifying package is now missing - i think it should still be there:

% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/t8941-presentation.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/t8941.check
@@ -3,5 +3,5 @@ reload: Source.scala
 askType at Source.scala(6,7)
 ================================================================================
 [response] askTypeAt (6,7)
-scala.Predef.???
+Predef.???
 ================================================================================
% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/random-presentation.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/random.check
@@ -4,7 +4,7 @@ askType at Random.scala(18,14)
 ================================================================================
 [response] askTypeAt (18,14)
 val filter: Int => Boolean = try {
-  java.lang.Integer.parseInt(args.apply(0)) match {
+  lang.Integer.parseInt(args.apply(0)) match {
     case 1 => ((x: Int) => x.%(2).!=(0))
     case 2 => ((x: Int) => x.%(2).==(0))
Member

lrytz commented Sep 18, 2015

the test output suggests that a qualifying package is now missing - i think it should still be there:

% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/t8941-presentation.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/t8941.check
@@ -3,5 +3,5 @@ reload: Source.scala
 askType at Source.scala(6,7)
 ================================================================================
 [response] askTypeAt (6,7)
-scala.Predef.???
+Predef.???
 ================================================================================
% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/random-presentation.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/presentation/random.check
@@ -4,7 +4,7 @@ askType at Random.scala(18,14)
 ================================================================================
 [response] askTypeAt (18,14)
 val filter: Int => Boolean = try {
-  java.lang.Integer.parseInt(args.apply(0)) match {
+  lang.Integer.parseInt(args.apply(0)) match {
     case 1 => ((x: Int) => x.%(2).!=(0))
     case 2 => ((x: Int) => x.%(2).==(0))
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 18, 2015

Member

Pushed updated checkfiles.

Member

retronym commented Sep 18, 2015

Pushed updated checkfiles.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 21, 2015

Member

Pushed updated checkfiles, for real this time.

Member

retronym commented Sep 21, 2015

Pushed updated checkfiles, for real this time.

SI-9473 Cleaner references to statically owned symbols
Ever wonder why `identity("")` typechecks to
`scala.this.Predef.identity("")`?

It turns out that `mkAttributedRef` was importing
`q"$scalaPackageClass.this.Predef._"` for all these years,
rather than `q"$scalaModule.Predef._"`.

This commit makes `mkAttributedRef` special case static owners
by referring the the corresponding module, instead.
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Sep 22, 2015

Member

LGTM

Member

lrytz commented Sep 22, 2015

LGTM

lrytz added a commit that referenced this pull request Sep 22, 2015

Merge pull request #4749 from retronym/ticket/9473
SI-9473 Cleaner references to statically owned symbols

@lrytz lrytz merged commit 128d632 into scala:2.12.x Sep 22, 2015

5 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
integrate-ide [417] SUCCESS. Took 11 s.
Details
validate-main [508] SUCCESS. Took 147 min.
Details
validate-publish-core [494] SUCCESS. Took 21 min.
Details
validate-test [412] SUCCESS. Took 125 min.
Details

sjrd added a commit to sjrd/scala-js that referenced this pull request Oct 5, 2015

Fix scala-js#1932: Recognize 2.12.0-M3's new shape of spurious classO…
…f[T] trees.

The scala/scala PR scala/scala#4749 changed
the shape of the spurious `Predef.classOf[T]` that reach the
`jsinterop` phase. Previously they looked like
`scala.this.Predef.classOf[T]`, where as now they are
`scala.Predef.classOf[T]`.

This commit generalizes the shapes of trees we recognize, so that
both patterns are matched.

ummels added a commit to ummels/scala-js that referenced this pull request Oct 9, 2015

Fix scala-js#1932: Recognize 2.12.0-M3's new shape of spurious classO…
…f[T] trees.

The scala/scala PR scala/scala#4749 changed
the shape of the spurious `Predef.classOf[T]` that reach the
`jsinterop` phase. Previously they looked like
`scala.this.Predef.classOf[T]`, where as now they are
`scala.Predef.classOf[T]`.

This commit generalizes the shapes of trees we recognize, so that
both patterns are matched.

sjrd added a commit to sjrd/scala-js that referenced this pull request Oct 27, 2015

Fix scala-js#1932: Recognize 2.12.0-M3's new shape of spurious classO…
…f[T] trees.

The scala/scala PR scala/scala#4749 changed
the shape of the spurious `Predef.classOf[T]` that reach the
`jsinterop` phase. Previously they looked like
`scala.this.Predef.classOf[T]`, where as now they are
`scala.Predef.classOf[T]`.

This commit generalizes the shapes of trees we recognize, so that
both patterns are matched.

ilinum added a commit to ilinum/intellij-scala that referenced this pull request Nov 19, 2015

Fix decompilation of Predef for 2.12-M3+
It was caused by this PR: scala/scala#4749 #SCL-9457 Fixed

ilinum added a commit to ilinum/intellij-scala that referenced this pull request Nov 19, 2015

Fix decompilation of Predef for 2.12-M3+
It was caused by this PR: scala/scala#4749
#SCL-9457 Fixed

retronym added a commit to retronym/scala that referenced this pull request Nov 26, 2015

SI-9542 Unify different reprs. of module type refs
The new unit test shows failures in transitivity of subtyping
and type equivalence, which boil down the the inconsistent
handling of the semantically equivalent:

   ThisType(pre, ModuleClass)
   ModuleTypeRef(pre, ModuleClass)
   SingleType(pre, Module)

This commit:

   - adds a case to `normalizePlus` to unwrap a `ThisType` to
     a `ModuleTypeRef`
   - Use `normalizePlus` more widely during subtype comparison
   - refactor `fourthTry` (part of `isSubType`) to remove code
     that becomes obviated by the use of `normalizePlus`.

This fixes the regression in the extension methods phase which
was triggered by scala#4749.
We can also fix that regression by tweaking the extension methods
phase itself to emit the `ThisType` representation of the owner
of the value class, as before.

I plan to demonstrate the two approaches to fixing the regression
on separate branches, and the propose that the merged result of these
two is useds.

@joroKr21 joroKr21 referenced this pull request Jan 11, 2016

Closed

Remove Spark Vector #136

sjrd added a commit to sjrd/scala-js that referenced this pull request Jan 12, 2016

Fix scala-js#1932: Recognize 2.12.0-M3's new shape of spurious classO…
…f[T] trees.

The scala/scala PR scala/scala#4749 changed
the shape of the spurious `Predef.classOf[T]` that reach the
`jsinterop` phase. Previously they looked like
`scala.this.Predef.classOf[T]`, where as now they are
`scala.Predef.classOf[T]`.

This commit generalizes the shapes of trees we recognize, so that
both patterns are matched.

(cherry picked from commit eefb50a)

retronym added a commit to retronym/scala that referenced this pull request Feb 1, 2016

SI-9542 Unify different reprs. of module type refs
The new unit test shows failures in transitivity of subtyping
and type equivalence, which boil down the the inconsistent
handling of the semantically equivalent:

   ThisType(pre, ModuleClass)
   ModuleTypeRef(pre, ModuleClass)
   SingleType(pre, Module)

This commit:

   - adds a case to `normalizePlus` to unwrap a `ThisType` to
     a `ModuleTypeRef`
   - Use `normalizePlus` more widely during subtype comparison
   - refactor `fourthTry` (part of `isSubType`) to remove code
     that becomes obviated by the use of `normalizePlus`.

This fixes the regression in the extension methods phase which
was triggered by scala#4749.
We can also fix that regression by tweaking the extension methods
phase itself to emit the `ThisType` representation of the owner
of the value class, as before.

I plan to demonstrate the two approaches to fixing the regression
on separate branches, and the propose that the merged result of these
two is useds.

retronym added a commit to retronym/scala that referenced this pull request Feb 1, 2016

SI-9542 Unify different reprs. of module type refs
The new unit test shows failures in transitivity of subtyping
and type equivalence, which boil down the the inconsistent
handling of the semantically equivalent:

   ThisType(pre, ModuleClass)
   ModuleTypeRef(pre, ModuleClass)
   SingleType(pre, Module)

This commit:

   - adds a case to `normalizePlus` to unwrap a `ThisType` to
     a `ModuleTypeRef`
   - Use `normalizePlus` more widely during subtype comparison
   - refactor `fourthTry` (part of `isSubType`) to remove code
     that becomes obviated by the use of `normalizePlus`.

This fixes the regression in the extension methods phase which
was triggered by scala#4749.
We can also fix that regression by tweaking the extension methods
phase itself to emit the `ThisType` representation of the owner
of the value class, as before.

I plan to demonstrate the two approaches to fixing the regression
on separate branches, and the propose that the merged result of these
two is useds.

@adriaanm adriaanm added 2.12.0 and removed 2.12 labels Oct 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment