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

Nested case class construction fails with "no legal prefix" #4859

Closed
scabug opened this issue Jul 31, 2011 · 17 comments
Closed

Nested case class construction fails with "no legal prefix" #4859

scabug opened this issue Jul 31, 2011 · 17 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 31, 2011

==========================================================================================
This is a bug, affected versions (at least) 2.9.0.1, 2.9.1.RC1 and 2.10 (a recent nightly)

Try to compile the following:

object O {
val o = C().CC(0)
}
case class C(s: String = "") {
case class CC(ii: Int) {
val ss = s
}
}

Will result in
error: java.lang.NullPointerException
at scala.tools.nsc.typechecker.Typers$Typer.typedTypeConstructor(Typers.scala:4329)
at scala.tools.nsc.typechecker.Typers$Typer.typedTypeConstructor(Typers.scala:4353)
at scala.tools.nsc.typechecker.Typers$Typer.typedNew$1(Typers.scala:3185) ...

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 31, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4859?orig=1
Reporter: @hseeberger
Affected Versions: 2.9.0, 2.9.1

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 6, 2011

@paulp said:
Minimized.

object O {
  new C().CC()
}

class C() {
  case class CC()
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 6, 2011

Commit Message Bot (anonymous) said:
(extempore in r25453) Don't want to chase NPEs around for the rest of my life. Created
"NoCompilationUnit" and "NoSourceFile" objects to represent not-present
versions of these items. Seems a lot better than null. References
#4859, got past NPE only to uncover the actual problem. No review.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 6, 2011

@paulp said:
NPE fixed, actual problem is shown here (test/pending/pos/bug4859.scala)

object O {
  // error: C is not a legal prefix for a constructor
  C().CC()
  // but this works.
  D().DD()
}

case class C() {
  case class CC()
}

case class D() {
  class DD()
  object DD {
    def apply() = new DD()
  }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 16, 2011

@adriaanm said:
both should work

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 7, 2011

Matthew Farwell (mfarwell) said:
The following is also a manifestation of this bug:

object Test {
case class B() { case class C() }
var b = B()
var c = b.C()
}

This bug is now fixed in version scala-2.10.0.r25798-b20111007023456.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 3, 2011

Stefan Wehr (stefanwehr) said:
Is this bug fixed or not? The status is "open" and the resolution "unresolved", but Matthew Farwell's comment indicates that the bug is fixed in scala-2.10.0.r25798-b20111007023456.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 4, 2011

Matthew Farwell (mfarwell) said:
Sorry, the above was my opinion, I'm not an official person, I can't declare a bug fixed. I believe it to be fixed though.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 4, 2011

David Leuschner (dleuschner) said:
Thank you for the information, Matthew! It's good to know that it's fixed.

I just realized that this bug was already present before 2.9.1 was released on 2011-08-31 and the NPE was even fixed before it was released but it is still present in the current stable release. We as a company (factis research) are considering adopting Scala as the main language. We have a Haskell server in production and one of the reasons of choosing Scala over Haskell is that we're hoping that there's a focus on "industrial strength" quality, tools and commercial support. We got the impression that there's a big community using Scala not only for fun but for commercial services.

With this bug the compiler dies without helpful notice and you can't compile or deploy your product and have to revert to bisecting your code to detect what might be giving Scala hiccups and guess how to "rephrase" the code to make it work. This can cost hours of work and is very frustrating.

The bug has already been fixed three months ago. I'm wondering when the next stable Scala
release is planned which will include this fix? Is there a roadmap with planned releases?

I think it would be good to catch all exceptions and give as much information as possible to the user about what happened (like the source file that was currently being processed). With that information you have a chance to guess what might be the problem. GHC outputs a nice message ("the impossible happened") and asks you to report the bug. That's better than just a NPE because now you at least know it's a bug in the compiler (and not sbt or other tools).

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 4, 2011

@paulp said:
As I said in the commit message above, the NPE is fixed but the actual issue is not.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 4, 2011

David Leuschner (dleuschner) said:
That depends on the point of view. For the compiler developer the "actual issue" might be something different than for the user. My issue is not that I have to rewrite my code but that I have to spend 2 hours to find out what might be going wrong with the build and which change caused the problem (of course I first think that it's MY fault) and that I have to explain to my team that Scala is a good choice although the compiler crashes without giving a message. The "actual issue" is not a problem at all because there's an easy workaround. (Replace "C().CC()" with "val c = C(); c.CC()")

My suggestion is to "fix" the problem by giving a helpful message to the user (such as: known bug in the compiler, see issue #4859 for suggested workaround"). That shouldn't be much work in the compiler but can save users time and gives a much better impression. Of course a "real fix" would be better but I'd prefer an earlier release with a message that improves the user experience.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@adriaanm said:
this is as specified: "A constructor invocation is a function application x.ctargs...(argsn), where x is a stable identifier (§3.1)"
we could in principle improve on this, but i'm going to say "fixed"

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@paulp said:
There is no constructor invocation in this code. The compiler is the only one throwing around constructor invocations. As far as the author of the code is concerned, it's two method calls. At the point where the compiler should be inserting a constructor call, it does have a stable prefix: "this".

object O {
  // error: C is not a legal prefix for a constructor
  C().CC()
}

(From the example in my most recent comment containing code.)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@paulp said:
...and thusly do I reopen. The ball is in your court, good sir.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 25, 2012

@adriaanm said:
right you are

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Dec 5, 2012

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.