Skip to content

Under -Xsource:2.13, warn about changed precedence in imports #10095

Merged
lrytz merged 1 commit intoscala:2.12.xfrom
lrytz:import-migration
Aug 18, 2022
Merged

Under -Xsource:2.13, warn about changed precedence in imports #10095
lrytz merged 1 commit intoscala:2.12.xfrom
lrytz:import-migration

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Jul 28, 2022

No description provided.

@scala-jenkins scala-jenkins added this to the 2.12.17 milestone Jul 28, 2022
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jul 28, 2022
@SethTisue
Copy link
Member

maybe this should be enabled by -Xmigration?

@lrytz
Copy link
Member Author

lrytz commented Jul 29, 2022

I was thinking which flag to re-use; -Xmigration is for the @migration annotation, so I don't think it's a good match?

Probably -Xsource is the right choice, it enables additional warnings and errors (but also aligns some type checking and implicit logic with 2.13).

@lrytz lrytz force-pushed the import-migration branch from a079971 to a452aab Compare July 29, 2022 13:49
def typedPackageDef(pdef0: PackageDef) = {
val pdef = treeCopy.PackageDef(pdef0, pdef0.pid, pluginsEnterStats(this, pdef0.stats))
val pid1 = typedQualifier(pdef.pid).asInstanceOf[RefTree]
val pid1 = context.withMode(ContextMode.InPackageClauseName)(typedQualifier(pdef.pid).asInstanceOf[RefTree])
Copy link
Member Author

@lrytz lrytz Jul 29, 2022

Choose a reason for hiding this comment

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

I will forward this to 2.13 (#10111) and use it to fix lookup when type-checking a package clause.

In 2.13:

package a.x
class C { def inA = 0 }
package b
import a._
package x {
  class C { def inB = 0 }
}

the PackageDef.pid for b.x will refer to the package symbol a.x because of the fixed lookup rules.

I couldn't spot any uses of PackageDef.pid in the compiler, so it's probably benign.

@lrytz lrytz marked this pull request as ready for review July 29, 2022 13:54
@lrytz lrytz force-pushed the import-migration branch from a452aab to 38333bd Compare August 11, 2022 12:12
@lrytz lrytz force-pushed the import-migration branch from 38333bd to 4ee4a02 Compare August 15, 2022 08:28
@lrytz
Copy link
Member Author

lrytz commented Aug 16, 2022

This can lead to additional import cycle errors under -Xsource:2.13. Once a symbol is found in the current package, lookup continues to see if there's an import that also makes a matching symbol available. However, the same import cycle errors show up under 2.13, so this is expected.

Here's an example (3 separate files) that compiles with 2.12, but not with -Xsource:2.13, and neither with a 2.13 compiler:

package p
trait A
package p
import p.C._
object B extends A
package p
import p.B._
object C extends A

@retronym
Copy link
Member

Very useful!

@SethTisue SethTisue changed the title Warning about changed precedence in imports when migrating to 2.13 Warning about changed precedence in imports when migrating to 2.13 (under -Xsource:2.13) Aug 17, 2022
@SethTisue SethTisue changed the title Warning about changed precedence in imports when migrating to 2.13 (under -Xsource:2.13) Under -Xsource:2.13, warn about changed precedence in imports Aug 17, 2022
@lrytz lrytz merged commit 9d39865 into scala:2.12.x Aug 18, 2022
@som-snytt
Copy link
Contributor

Thanks, I never thought to do that.

dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Sep 17, 2022
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.12.17
- https://www.scala-lang.org/news/2.12.17

### Why are the changes needed?
The main [change](https://github.com/scala/scala/pulls?q=is%3Apr+sort%3Aupdated-desc+milestone%3A2.12.17+is%3Amerged+label%3Arelease-notes) fo this version as follows:

- scala/scala#10109
- scala/scala#10075
- scala/scala#10108
- scala/scala#10045
- scala/scala#10063
- scala/scala#10042
- scala/scala#10040
- scala/scala#10095

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
Existing Test

Closes #37892 from LuciferYang/SPARK-40436.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
LuciferYang added a commit to LuciferYang/spark that referenced this pull request Sep 20, 2022
### What changes were proposed in this pull request?
This PR aims to upgrade Scala to 2.12.17
- https://www.scala-lang.org/news/2.12.17

### Why are the changes needed?
The main [change](https://github.com/scala/scala/pulls?q=is%3Apr+sort%3Aupdated-desc+milestone%3A2.12.17+is%3Amerged+label%3Arelease-notes) fo this version as follows:

- scala/scala#10109
- scala/scala#10075
- scala/scala#10108
- scala/scala#10045
- scala/scala#10063
- scala/scala#10042
- scala/scala#10040
- scala/scala#10095

### Does this PR introduce _any_ user-facing change?
Yes, this is a Scala version change.

### How was this patch tested?
Existing Test

Closes apache#37892 from LuciferYang/SPARK-40436.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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

Development

Successfully merging this pull request may close these issues.

5 participants