Fix scala/bug#10490: Allow Java nested class selection #6053

jvican · 2017-09-02T18:14:06Z

Java type selections, for example Foo.Bar, can represent two different
trees depending on the context. On the one hand, it can be a type
selection over an object Foo. On the other hand, it can be a type
selection over a type (that is, a selection of a nested class).

However, when we parse the Scala signatures is not possible to know
which case the user meant. For example, Foo.Bar could be either:

Select(Ident(TermName("Foo")), TypeName("Bar")); or
SelectFromTypeTree(Ident(TypeName("Foo")), TypeName("Bar")).

By default, the Scala compiler parses Foo.Bar in a type position as
the first option, but it could very well be the case that the user meant
the second one, as specified by the tests in this commit.

To address the issue, then, there's nothing but the ugly solution of
injecting special logic into typer so that we try to typecheck both
trees for those different scenarios.

To handle this specific logic and try to make it as isolated as
possible, I've introduced a new context mode called JavaTypeSelection.
This context mode is set by typedSelectOrSuperCall when the
compilation unit is Java, the tree at hands selects a type name and the
qualifier is an identifier. typedIdent detects when this mode is on,
and if the default term symbol lookup fails it tries to find the type
symbol. If that succeeds, it then delegates to typedType. The
different returning trees are handled in typedSelectOrSuperCall.

Run tests in this commit illustrate both accesses. They are independent
because if t10490 had the Foo companion, the bug that this commit
fixes wouldn't have been required (i.e. it's a very subtle workaround).

jvican · 2017-09-02T18:23:01Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+              qual match {
+                case id: Ident if context.unit.isJava =>
+                  val qualTyped1 = context.withinJavaTypeSelection(typeQual)
+                  (qualTyped1, qualTyped1.isType)


It's really interesting that this is all we need to do in typedSelectOrSuperCall, since the whole method will return Select(qualTyped1, name) whereas the more correct tree would have been SelectFromTypeTree(qualTyped1, name). I have not done this change because of simplicity. But it does make me wonder what is the crucial difference between Select and SelectFromTypeTree... It seems that Select(Ident(TypeName("Foo")), _) == SelectFromTypeTree(Ident(TypeName("Foo")), _), no?

I think for consistency it would be better to generate the SelectFromTypeTree, even if a Select doesn't violate any of the current assumptions in the rest of the compiler.

I can do that, but I'm afraid to modify too much of the logic of this method since it's really hot. I also think that for consistency it's better to generate a SelectFromTypeTree. Doing that requires me to wrap almost all the rest of the body that follows this statement with an if.

This should also get a comment that explains why context.unit.isJava here needs to be reified as a context.withinJavaTypeSelection for consumption down the line in typedIdent.

lrytz · 2017-09-04T13:14:28Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+              qual match {
+                case id: Ident if context.unit.isJava =>
+                  val qualTyped1 = context.withinJavaTypeSelection(typeQual)
+                  (qualTyped1, qualTyped1.isType)


I think for consistency it would be better to generate the SelectFromTypeTree, even if a Select doesn't violate any of the current assumptions in the rest of the compiler.

lrytz · 2017-09-04T13:23:00Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+            if (name.isTermName) (typeQual, false)
+            else {
+              qual match {
+                case id: Ident if context.unit.isJava =>


A comment would be good here, e.g.

scala/bug#10490: given a Scala class A { class B }, in mixed compilation, a reference A.B in a Java source would cause a "not found: value A". The `JavaTypeSelection` mode makes `typedIdent` type check A as a type if no value is found. In case A has a companion, it works thanks to the fix of scala/bug#3120 in typedSelectInternal.

I'll add it 👍

retronym · 2017-09-04T22:43:05Z

It would be worth looking at #5762, which overhauls typechecking of selections to remove inconsistencies. I wasn't brave enough to merge that into 2.12.3, but will likely be merged soon.

smarter · 2017-09-10T00:49:33Z

To address the issue, then, there's nothing but the ugly solution of
injecting special logic into typer so that we try to typecheck both
trees for those different scenarios.

Isn't there already logic in the typer to do this in

scala/src/compiler/scala/tools/nsc/typechecker/Typers.scala

Lines 4919 to 4926 in e1e8d05

    
           if (context.unit.isJava && name.isTypeName) { 
        
             // scala/bug#3120 Java uses the same syntax, A.B, to express selection from the 
        
             // value A and from the type A. We have to try both. 
        
             atPos(tree.pos)(gen.convertToSelectFromType(qual, name)) match { 
        
               case EmptyTree => None 
        
               case tree1     => Some(typed1(tree1, mode, pt)) 
        
             } 
        
           }

? Dotty does something very much inspired by that (https://github.com/lampepfl/dotty/blob/851c3c2212b7457dedc9ad1e59ff688b17458db5/compiler/src/dotty/tools/dotc/typer/Typer.scala#L418-L421) and both of your test cases compile with it.

jvican · 2017-09-10T14:11:57Z

I saw that logic, I don't know why it was added but it doesn't seem to work; this path in typedSelect is never hit because it is typedIdent the one that fails to typecheck Ident(TypeName("foo")). An error is registered right away in the reporter and the symbol and name are marked as errors when they hit typedSelect.

I would also argue that aside from working, my fix is more elegant and correct. The real fix should happen in typedIdent, not typedSelect, and typedSelect should not try to reverse errors that are reported by typedIdent. The less communication and trial and error, the better IMO.

jvican · 2017-09-10T14:15:13Z

@retronym I've read the code in that PR. It doesn't seem to interact with the bug that I fix here, let me know if I'm wrong. I'm happy to rebase this when that gets merged.

lrytz · 2017-09-11T09:51:44Z

The existing logic in typedSelect is for a selection A.B in a Java source file, where A and B are java-defined classes.

class A { class B { } }        // v1
class A { static class B { } } // v2
class C { void foo(A.B) { } }

For each package-level Java class, the Scala compiler creates a class and a module symbol, static members are added to the module symbol.

When the Scala compiler type-checks A.B, it doesn't know if B is a static class or not. That's what the existing fix is for. (If A.B was written in Scala, we'd now it's the static member, as the non-static one would be A#B.)

The fix in this PR is when selecting A.B in a Java source with A defined in Scala. A is type checked as a term ident. This fails if A doesn't have a companion object, as there's no value A, only a type.

So there must be some other code path than https://github.com/lampepfl/dotty/blob/851c3c2212b7457dedc9ad1e59ff688b17458db5/compiler/src/dotty/tools/dotc/typer/Typer.scala#L418-L421 in dotty that makes the example work.

jvican · 2017-09-19T14:09:17Z

@adriaanm Any chance this can be merged for 2.12.4? I'll update it as soon as #5762 lands...

adriaanm · 2017-09-19T17:14:36Z

I don't see why not. I'm making my way through the PR queue. Will look at this one next (in the next few hours).

adriaanm · 2017-09-19T18:56:32Z

I assume you meant whether it can be merged once the review feedback has been addressed? I +1 the more comments as requested by Lukas -- this is tricky stuff and there's some good knowledge around this in this PR that should end up in the code.

I'll look at the actual code after lunch, but I would also second the other request in directly emitting the right tree (SelectFromType), if we can find an elegant way to do it.

adriaanm · 2017-09-19T22:39:52Z

When you rebase, please also drop "Fix scala/bug#10490: " from the commit title. That should go into the PR description. Thanks for the great commit message.

adriaanm · 2017-09-20T00:07:00Z

I've pushed a tree with a sketch that rebases your work on my earlier PR and also boyscouts a bit in the area. Could you verify that it makes sense and absorb this into your PR? Now that I've looked at the code, I feel this area has hit a critical point where we need to do a bit of reinforcement before we tack on more stuff.

jvican · 2017-09-21T11:54:45Z

I assume you meant whether it can be merged once the review feedback has been addressed?

Indeed.

When you rebase, please also drop "Fix scala/bug#10490: " from the commit title. That should go into the PR description. Thanks for the great commit message.

Deal.

I've pushed a tree with a sketch that rebases your work on my earlier PR and also boyscouts a bit in the area. Could you verify that it makes sense and absorb this into your PR? Now that I've looked at the code, I feel this area has hit a critical point where we need to do a bit of reinforcement before we tack on more stuff.

Excellent. I'm absorbing these commits into this PR.

jvican · 2017-09-25T11:52:42Z

This PR has been updated according to feedback and is ready for another pass. I haven't scheduled benchmarks because I'm not sure this PR requires them!

jvican · 2017-09-25T14:13:33Z

/rebuild

jvican · 2017-09-25T19:07:58Z

Weird -- I cannot reproduce the error locally. I'm trying tomorrow again.

adriaanm · 2017-09-25T20:42:40Z

I'll take a look as well

adriaanm · 2017-09-25T21:12:20Z

I think we should just update the check file. It doesn't make sense to me to expand to selectDynamic("<error>") when we encounter a mall-formed tree. This is why I pulled that condition out as if (name == nme.ERROR || qual.tpe.widen.isErroneous) EmptyTree.

Java type selections, for example `Foo.Bar`, can represent two different trees depending on the context. On the one hand, it can be a type selection over an object `Foo`. On the other hand, it can be a type selection over a type (that is, a selection of a nested class). However, when we parse the Scala signatures is not possible to know which case the user meant. For example, `Foo.Bar` could be either: 1. `Select(Ident(TermName("Foo")), TypeName("Bar"))`; or 2. `SelectFromTypeTree(Ident(TypeName("Foo")), TypeName("Bar"))`. By default, the Scala compiler parses `Foo.Bar` in a type position as the first option, but it could very well be the case that the user meant the second one, as specified by the tests in this commit. To address the issue, then, there's nothing but the ugly solution of injecting special logic into typer so that we try to typecheck both trees for those different scenarios. To handle this specific logic and try to make it as isolated as possible, I've introduced a new context mode called `JavaTypeSelection`. This context mode is set by `typedSelectOrSuperCall` when the compilation unit is Java, the tree at hands select a type name and the qualifier is an identifier. `typedIdent` detects when this mode is on, and if the default term symbol lookup fails it tries to find the type symbol. If that succeeds, it then delegates to `typedType`. The different returning trees are handled in `typedSelectOrSuperCall`. Run tests in this commit illustrate both accesses. They are independent because if t10490 had the `Foo` companion, the bug that this commit fixes wouldn't have been required (i.e. it's a very subtle workaround). Fixes scala/bug#10490.

jvican · 2017-09-25T22:01:01Z

@adriaanm Makes sense. PR has been updated with the updated check file, agreed that the new error message is better than exposing the last try with the dynamic call typechecking.

lrytz · 2017-09-26T14:10:41Z

I'd like to see a summary of Fix scala/bug#10490: Allow Java nested class selection #6053 (comment) in the commit message.
Adriaan's comment Fix scala/bug#10490: Allow Java nested class selection #6053 (comment) was not addressed, I agree we should document why a mode is necessary
I also proposed a comment here: Fix scala/bug#10490: Allow Java nested class selection #6053 (comment), I don't see it added.
We're still building a Select tree instead of a SelectFromTypeTree. I'd like to see that changed, @adriaanm wdyt?

jvican · 2017-09-26T15:21:56Z

I'd like to see a summary of #6053 (comment) in the commit message.

I'll add it.

Adriaan's comment #6053 (comment) was not addressed, I agree we should document why a mode is necessary

Sorry, I missed this one.

I also proposed a comment here: #6053 (comment), I don't see it added.

It's been added, but it's shorter:
https://github.com/scala/scala/pull/6053/files#diff-4eab1aad4533a31c10565971e90f73eaR5047

adriaanm · 2017-09-26T16:23:03Z

We're still building a Select tree instead of a SelectFromTypeTree. I'd like to see that changed, @adriaanm wdyt?

yes please -- let's stick to the convention (although the original design decision is not immediately clear to me)

adriaanm · 2017-09-27T00:34:47Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+      def typedTypeSelectionQualifier(tree: Tree, pt: Type = AnyRefTpe) = {
+        def typedQual = context.withImplicitsDisabled { typed(tree, MonoQualifierModes | mode.onlyTypePat, pt) }
+        tree match {
+          case id: Ident if context.unit.isJava => context.withinJavaTypeSelection { typedQual }


Talking this through with @retronym, it occurred to us that this withinJavaTypeSelection context bit would also be useful in @hrhino's quest to improve error messages in #6042. With that insight, should it just be withinTypeSelection (and should we have one for withinPattern?)

Also, why is this is only done when we reach an Ident? What are the varying combos of A, A.B and A.B.C.

why is this is only done when we reach an Ident

For selections there's an existing fix / workaround, see my comment here #6053 (comment).

Would it be feasible to combine the two fixes?

Yes, I think so.

@adriaanm i was having a look at rebasing this PR and addressing your comment. Do you want me to combine the two fixes on this PR or how should we operate? /cc @hrhino

combining sounds good to me

Alright, I'll cherry-pick @hrhino's changes into this PR 👍

jvican · 2017-09-27T22:07:42Z

@adriaanm Let's delay this change to 2.12.5. I'll update this PR and try to integrate it with other efforts in this area, as you've pointed out to @hrhino's efforts.

SethTisue · 2018-02-22T21:51:20Z

@jvican interested in trying to finish this up in time for 2.12.5...? I've changed the milestone to 2.12.6, but only tentatively.

jvican · 2018-03-05T13:46:04Z

@SethTisue Let's leave it for 2.12.6, since I believe that integrating Harrison's changes will take me a little bit of time and there are things with higher priority on my scala/scala queue 😄.

adriaanm · 2018-04-30T08:30:50Z

At this point I think we should move this work to 2.13.0-M5. I would also encourage you to integrate the other efforts in this area.

jvican · 2018-04-30T10:59:10Z

Can i reopen this PR targeting 2.13.0-M5?

SethTisue · 2018-04-30T11:34:52Z

sure, you can retarget and reopen

retronym · 2019-01-14T05:40:33Z

I'm taking a look at reviving this in 2.13.x...retronym:topic/java-inherited-type-ident

(cherry picked from commit 81fff93)

- Inherited type declarations are in scope in Java code - For static innner classes, we need to check in the companion module of each base classes. - Incorporate and accomodate test case from #6053 - Tests to java code referring to module-class owned classes via companion class prefix

- Inherited type declarations are in scope in Java code - For static innner classes, we need to check in the companion module of each base classes. - Incorporate and accomodate test case from scala#6053 - Tests to java code referring to module-class owned classes via companion class prefix Backport of scala#7671

scala-jenkins added this to the 2.12.4 milestone Sep 2, 2017

jvican commented Sep 2, 2017

View reviewed changes

jvican force-pushed the t10490 branch from 8a1f54f to bdddc50 Compare September 2, 2017 19:04

lrytz reviewed Sep 4, 2017

View reviewed changes

jvican force-pushed the t10490 branch from bdddc50 to a5b6fb1 Compare September 25, 2017 11:51

adriaanm and others added 2 commits September 26, 2017 00:00

boyscouting

b399046

jvican force-pushed the t10490 branch from a5b6fb1 to ef176d7 Compare September 25, 2017 22:00

adriaanm reviewed Sep 27, 2017

View reviewed changes

adriaanm modified the milestones: 2.12.4, 2.12.5 Sep 27, 2017

SethTisue modified the milestones: 2.12.5, 2.12.6 Feb 22, 2018

SethTisue added the WIP label Mar 5, 2018

SethTisue modified the milestones: 2.12.6, 2.12.7 Mar 23, 2018

adriaanm closed this Apr 30, 2018

SethTisue removed this from the 2.12.7 milestone Sep 10, 2018

retronym added a commit to retronym/scala that referenced this pull request Jan 14, 2019

Incorporate and accomodate test case from scala#6053

266c721

retronym added a commit to retronym/scala that referenced this pull request Jan 14, 2019

Incorporate and accomodate test case from scala#6053

e34260c

retronym added a commit to retronym/scala that referenced this pull request Jan 22, 2019

Incorporate and accomodate test case from scala#6053

f4f9e27

retronym added a commit to retronym/scala that referenced this pull request Jan 22, 2019

Incorporate and accomodate test case from scala#6053

81fff93

retronym added a commit to retronym/scala that referenced this pull request Feb 11, 2019

[backport] Incorporate and accomodate test case from scala#6053

ded4d85

(cherry picked from commit 81fff93)

adriaanm pushed a commit to retronym/scala that referenced this pull request Feb 11, 2019

Incorporate and accomodate test case from scala#6053

46b81b8

retronym mentioned this pull request Feb 12, 2019

[backport] Use Java rules for member lookup in .java sources #7738

Merged

Fix scala/bug#10490: Allow Java nested class selection #6053

Fix scala/bug#10490: Allow Java nested class selection #6053

Uh oh!

Conversation

jvican commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

retronym commented Sep 4, 2017

Uh oh!

smarter commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvican commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvican commented Sep 10, 2017

Uh oh!

lrytz commented Sep 11, 2017

Uh oh!

jvican commented Sep 19, 2017

Uh oh!

adriaanm commented Sep 19, 2017

Uh oh!

adriaanm commented Sep 19, 2017

Uh oh!

adriaanm commented Sep 19, 2017

Uh oh!

adriaanm commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvican commented Sep 21, 2017

Uh oh!

jvican commented Sep 25, 2017

Uh oh!

jvican commented Sep 25, 2017

Uh oh!

jvican commented Sep 25, 2017

Uh oh!

adriaanm commented Sep 25, 2017

Uh oh!

adriaanm commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvican commented Sep 25, 2017

Uh oh!

lrytz commented Sep 26, 2017

Uh oh!

jvican commented Sep 26, 2017

Uh oh!

adriaanm commented Sep 26, 2017

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!

jvican commented Sep 27, 2017

jvican commented Sep 2, 2017 •

edited

Loading

smarter commented Sep 10, 2017 •

edited

Loading

jvican commented Sep 10, 2017 •

edited

Loading

adriaanm commented Sep 20, 2017 •

edited

Loading

adriaanm commented Sep 25, 2017 •

edited

Loading