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

Typer inlines constants and makes dependency analysis impossible #7173

Closed
scabug opened this issue Feb 23, 2013 · 10 comments
Closed

Typer inlines constants and makes dependency analysis impossible #7173

scabug opened this issue Feb 23, 2013 · 10 comments
Labels
Milestone

Comments

@scabug
Copy link

scabug commented Feb 23, 2013

The problem has been mentioned here:
https://groups.google.com/d/topic/scala-internals/b95Y-GbXVGA/discussion

The gist is that typer inlines compile time constants (e.g. references to static fields defined in Java) and forgets about the original tree that referred to a constant (e.g. to a field) which makes dependency analysis impossible.

According to Adriaan pattern matcher does not need constants to be inlined because it works with types. I couldn't readily find a reason why we need to inline those constants in typer so we probably should move this all the way to erasure.

I'm logging a ticket to not forget to investigate if we can do that for 2.10.2.

@scabug
Copy link
Author

scabug commented Feb 23, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7173?orig=1
Reporter: @gkossakowski
Affected Versions: 2.10.0

@scabug
Copy link
Author

scabug commented May 20, 2013

@JamesIry said:
2.10.2 is about to be cut. Kicking down the road and un-assigning to foster work stealing.

@scabug
Copy link
Author

scabug commented Aug 28, 2013

@gkossakowski said:
It turns out that this is causing problems not only for dependency analysis.

The improved incremental compiler I'm working on relies on extracting used names in a given compilation unit. Once constant gets inlined we do not have access to the field that defined it therefore we can't extract its name.

I thought I could work-around it by inspecting symbol names of symbols in CompilationUnit.depends but we have only top-level classes recorded there, see:
https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/typechecker/Infer.scala#L263

Therefore I believe there's no other way than just fix the underlaying issue.

According to Adriaan we could perform constant inlining and folding based purely on types and leave trees intact. We still want constants to be folded in the bytecode so we could replace trees having constant types with actual constants in some later phase like erasure. This is an interesting idea that I would like to try out at some point.

@scabug
Copy link
Author

scabug commented Feb 4, 2017

@jvican said (edited on Feb 4, 2017 9:45:41 AM UTC):
Does this still affect 2.11 and 2.12 versions? I think it does.

The following test in Zinc is failing because of it. Could someone confirm this hasn't been fixed in new Scala versions and set those versions in the ticket?

This constant inlining could be perjudicial for a lot of Zinc users that make intense use of final vals. I think it would make sense to fix this and I'd be happy to take a stab at it. I'd like to know if @adriaanm or @retronym would accept such a PR for next 2.12.x and 2.11.x series (if there's to a be a new version for 2.11). A time estimate to fix this would also be very useful to pick the appropriate moment to implement it. I think this could be a blocker for a fully stable Zinc 1.0 release (though we could probably live with it since it depends on the compiler).

@scabug
Copy link
Author

scabug commented Feb 21, 2017

@milessabin said:
This will probably interact with the literal types PR ... please take a close look at that if/when working on this.

@scabug
Copy link
Author

scabug commented Feb 21, 2017

@dwijnand said:
scala/scala#4880 was an attempt at resolving this.

@scabug
Copy link
Author

scabug commented Feb 21, 2017

@milessabin said:
I think at least some of pull/4880 was folded into the literal types PR.

@scabug
Copy link
Author

scabug commented Feb 25, 2017

@jvican said:
I have a partial workaround for this here.
In the long term, I'll try to follow up on Adriaan's work to only use types to inline constants.

@scabug scabug added the typer label Apr 7, 2017
@scabug scabug added this to the Backlog milestone Apr 7, 2017
jvican added a commit to jvican/scala that referenced this issue Jul 29, 2017
Adds an original tree attachment that allows external tools (compiler
plugins like scalameta & zinc) to keep track of the previous, unadapted
tree. The reason why this is required is explained in SD-340: scala/scala-dev#340.

This change enables the incremental compiler to detect changes in `final
val`s: sbt/zinc#227.

It also enables a fix for scala/bug#10426 by allowing the scaladoc
compiler to let the compiler adapt literals without losing the tree that
will be shown in the Scaladoc UI.

To maintainers: I was thinking of the best way to test this, but
couldn't come up with an elegant one. Do you suggest a way I could write
a test for this? Is there a precedent in testing information carried in
the trees?

I think @lrytz is the right person to review this, since he suggested
this fix in the first place.
jvican added a commit to jvican/scala that referenced this issue Jul 29, 2017
Adds an original tree attachment that allows external tools (compiler
plugins like scalameta & zinc) to keep track of the previous, unadapted
tree. The reason why this is required is explained in SD-340: scala/scala-dev#340.

This change enables the incremental compiler to detect changes in `final
val`s: sbt/zinc#227.

It also enables a fix for scala/bug#10426 by allowing the scaladoc
compiler to let the compiler adapt literals without losing the tree that
will be shown in the Scaladoc UI.

To maintainers: I was thinking of the best way to test this, but
couldn't come up with an elegant one. Do you suggest a way I could write
a test for this? Is there a precedent in testing information carried in
the trees?

I think @lrytz is the right person to review this, since he suggested
this fix in the first place.
jvican added a commit to jvican/scala that referenced this issue Sep 25, 2017
Adds an original tree attachment that allows external tools (compiler
plugins like scalameta & zinc) to keep track of the previous, unadapted
tree. The reason why this is required is explained in SD-340: scala/scala-dev#340.

This change enables the incremental compiler to detect changes in `final
val`s: sbt/zinc#227.

It also enables a fix for scala/bug#10426 by allowing the scaladoc
compiler to let the compiler adapt literals without losing the tree that
will be shown in the Scaladoc UI.

To maintainers: I was thinking of the best way to test this, but
couldn't come up with an elegant one. Do you suggest a way I could write
a test for this? Is there a precedent in testing information carried in
the trees?

I think @lrytz is the right person to review this, since he suggested
this fix in the first place.

Fixes scala/bug#7173.
jvican added a commit to jvican/scala that referenced this issue Sep 25, 2017
Adds an original tree attachment that allows external tools (compiler
plugins like scalameta & zinc) to keep track of the previous, unadapted
tree. The reason why this is required is explained in SD-340: scala/scala-dev#340.

This change enables the incremental compiler to detect changes in `final
val`s: sbt/zinc#227.

It also enables a fix for scala/bug#10426 by allowing the scaladoc
compiler to let the compiler adapt literals without losing the tree that
will be shown in the Scaladoc UI.

To maintainers: I was thinking of the best way to test this, but
couldn't come up with an elegant one. Do you suggest a way I could write
a test for this? Is there a precedent in testing information carried in
the trees?

I think @lrytz is the right person to review this, since he suggested
this fix in the first place.

Fixes scala/bug#7173.
@retronym
Copy link
Member

Here's another symptom of the fact that we constant fold term trees:

class C {
  final def foo = 1
  val s: String = {class D extends C { def foo = 2 }; ""}
}

-Xprint:typer shows val s: String = "";, and the invalid override of def foo is not flagged.

@lrytz
Copy link
Member

lrytz commented Sep 27, 2017

See also #10340 (comment)

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

No branches or pull requests

4 participants