Skip to content
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

Presentation compiler fails to resolve implicit defined in package object #8085

Closed
scabug opened this Issue Dec 16, 2013 · 14 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

commented Dec 16, 2013

The following was exctrated from the Coursera Reactive class assignment:

// nodescala/package.scala
import scala.concurrent.Future

package object nodescala {
  implicit class FutureCompanionOps[T](val f: Future.type) extends AnyVal {
    def always[T](value: T): Future[T] = Promise[T].success(value).future
  }
}

// nodescala/NodeScalaSuite.scala
package nodescala

import scala.concurrent.Future

class NodeScalaSuite {
  Future.always(517) //<-- error reported
}

The reported error is: "value always is not a member of object scala.concurrent.Future"

I've managed to create a presentation compiler test that demonstrates the problem, will be pushing a patch with the failing test soon.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 16, 2013

Imported From: https://issues.scala-lang.org/browse/SI-8085?orig=1
Reporter: @dotta
Other Milestones: 2.11.0-M8

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 16, 2013

@dotta said (edited on Dec 17, 2013 12:57:08 PM UTC):
Here is a link to the commit containing the test: 9c40363a38339e987b092f4bd08b305b64e4d49b dotta/scala@7cab3db

And I'm attaching that commit as a .patch to this ticket. (The discussion in the ticket proves it's better to reference a link)

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 16, 2013

@dotta said:
Test demonstrating the issue reported in this ticket.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 16, 2013

@dotta said:
Finally, below is the output of the test:

% ./test/partest show-diff test/files/presentation/t8085
Partest version:     1.0.0-RC8
Compiler under test: $baseDir/scala-compiler.jar
Scala version is:    Scala compiler version 2.11.0-20131216-141809-45c3c6632b -- Copyright 2002-2013, LAMP/EPFL
Scalac options are:  
Compilation Path:    $baseDir/scala-library.jar:$baseDir/scala-reflect.jar:$baseDir/scala-compiler.jar:$baseDir/scalap.jar:$baseDir/scala-actors.jar:/Users/mirco/.m2/repository/org/scala-lang/modules/scala-partest_2.11.0-M7/1.0.0-RC8/scala-partest_2.11.0-M7-1.0.0-RC8.jar:/Users/mirco/.m2/repository/org/apache/ant/ant/1.8.4/ant-1.8.4.jar:/Users/mirco/.m2/repository/org/apache/ant/ant-launcher/1.8.4/ant-launcher-1.8.4.jar:/Users/mirco/.m2/repository/com/googlecode/java-diff-utils/diffutils/1.3.0/diffutils-1.3.0.jar:/Users/mirco/.m2/repository/org/scala-sbt/test-interface/1.0/test-interface-1.0.jar:/Users/mirco/.m2/repository/org/scala-lang/modules/scala-xml_2.11.0-M7/1.0.0-RC7/scala-xml_2.11.0-M7-1.0.0-RC7.jar:/Users/mirco/.m2/repository/org/scala-lang/modules/scala-parser-combinators_2.11.0-M7/1.0.0-RC5/scala-parser-combinators_2.11.0-M7-1.0.0-RC5.jar:/Users/mirco/.m2/repository/org/scalacheck/scalacheck_2.11.0-M7/1.11.1/scalacheck_2.11.0-M7-1.11.1.jar:$baseDir/scala-partest-extras.jar:$baseDir/scala-partest-javaagent.jar:$sourceDir/lib/annotations.jar:$sourceDir/lib/enums.jar:$sourceDir/lib/genericNest.jar:$sourceDir/lib/jsoup-1.3.1.jar:$sourceDir/lib/methvsfield.jar:$sourceDir/lib/nest.jar:$sourceDir/lib/scalacheck.jar
Java binaries in:    /System/Library/Java/JavaVirtualMachines/1.6.0.jdk/Contents/Home/bin
Java runtime is:     Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-462, mixed mode)
Java options are:    -Xmx1024M -Xms64M -XX:MaxPermSize=128M 
baseDir:             /Users/mirco/Projects/ide/scala/build/pack/lib
sourceDir:           /Users/mirco/Projects/ide/scala/test/files
    
Selected 1 tests drawn from specified tests

# starting 1 test in presentation
!! 1 - presentation/t8085                        [output differs]
% diff /Users/mirco/Projects/ide/scala/test/files/presentation/t8085-presentation.log /Users/mirco/Projects/ide/scala/test/files/presentation/t8085.check
@@ -1,2 +1,2 @@
 reload: NodeScalaSuite.scala
-value always is not a member of object scala.concurrent.Future
+Test OK

# 0/1 passed, 1 failed in presentation

# Failed test paths (this command will update checkfiles)
test/partest --update-check \
  /Users/mirco/Projects/ide/scala/test/files/presentation/t8085

0/1 passed, 1 failed (elapsed time: 00:00:06)
Test Run FAILED
@scabug

This comment has been minimized.

Copy link
Author

commented Dec 16, 2013

@retronym said (edited on Dec 16, 2013 4:30:59 PM UTC):
Needs a bit of minimization.

//package.scala
package object nodescala {
  def Foo: Int = 0
}
// NodeScalaSuite.scala
package nodescala

class NodeScalaSuite {
  Foo
}

This sort of problem means the openPackageModule hasn't been triggered, which adds the members of package objects to the package. I'm not sure if the presentation compiler test you created is even meant to to work, though: it doesn't look at src/*, does it? Poking around for source files that contain package objects is the domain of BrowsingLoaders.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@dotta said:
@JasonZaugg I'm confused, why do you think the test doesn't do what it should? Also, if you uncomment this line, the test succeed, which is why I believe the the test is meaningful - but I might be missing something. As far as I can tell, the issue is that the package object needs to be explicitly loaded, otherwise the presentation compiler gets confused. What you say about openPackageModule makes sense to me, so indeed my test case may be too specific (compared to the one you provided in your previous comment).

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@retronym said:
If you uncomment that line, you are telling the compiler where to find the definition of the package object.

But I believe that without that line, the fact that you put the package object under a file in ./src is no different to putting it in ./tmp; the presentation compiler test won't know that it exists.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@dragos said:
The presentation compiler should have a -sourcepath pointing to src. Not sure if it's on by default.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@dotta said (edited on Dec 17, 2013 10:57:26 AM UTC):
@JasonZaugg You are absolutely right! Thanks for pointing that out. I'm updating the test so that the presentation compiler is instantiated with the expected sourcepath value. Will let you know when it's done. (and thanks @iulian for the the sourcepath tip ;))

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@dragos said:
Normally you can set that in the .flags file...

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@dotta said:
I've updated the commit and that lead to a pretty interesting discovery - have a read at the commit message for details dotta/scala@7cab3db

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

@retronym said:
In your failing test, the parser emits package <empty> { import s.c.Future; package nodescala {} }.

This logic in Namers:

    /** All PackageClassInfoTypes come from here. */
    private def createPackageSymbol(pos: Position, pid: RefTree): Symbol = {
      val pkgOwner = pid match {
        case Ident(_)                 => if (owner.isEmptyPackageClass) rootMirror.RootClass else owner
        case Select(qual: RefTree, _) => createPackageSymbol(pos, qual).moduleClass
      }

.. needs to be reflected in BrowserTraverser.

@scabug

This comment has been minimized.

@scabug

This comment has been minimized.

Copy link
Author

commented Dec 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.