2.8.0.RC3 => 2.8.0.RC4 : override def with var behaviour changed ?

On Sat, Jun 12, 2010 at 10:28:25PM +0200, Francois wrote:
> I just tested RC4 in my application, and that code snippet used to
> work with previous version:

Yikes. It looks like something has gone off the beam.

trait Bip { def foo: String }
// this works
def f = new Bip { var foo = "abc" }
// this fails
def f: Bip = new Bip { var foo = "abc" }

The new is coming back typed as:

Object with Bip{def foo_=(x$1: java.lang.String): Unit}

Oh, this must have to do with getters, since only the setter is present
in the refinement.

On Sat, Jun 12, 2010 at 1:46 PM, Paul Phillips <paulp@improving.org> wrote:

On Sat, Jun 12, 2010 at 10:28:25PM +0200, Francois wrote:
> I just tested RC4 in my application, and that code snippet used to
> work with previous version:

Yikes.  It looks like something has gone off the beam.

Should we hold off for an RC5?
 

trait Bip { def foo: String }
// this works
def f = new Bip { var foo = "abc" }
// this fails
def f: Bip = new Bip { var foo = "abc" }

The new is coming back typed as:

 Object with Bip{def foo_=(x$1: java.lang.String): Unit}

Oh, this must have to do with getters, since only the setter is present
in the refinement.

--
Paul Phillips      | It is hard to believe that a man is
Stickler           | telling the truth when you know that you
Empiricist         | would lie if you were in his place.
pal, i pill push   |     -- H. L. Mencken

--
Lift, the simply functional web framework http://liftweb.net
Beginning Scala http://www.apress.com/book/view/1430219890
Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im
Surf the harmonics

On Sat, Jun 12, 2010 at 01:54:18PM -0700, David Pollak wrote:
> Should we hold off for an RC5?

I do not know, only martin can say. At least the patch where this bug
was introduced was astutely labelled a "quick hack":

https://lampsvn.epfl.ch/trac/scala/changeset/22184

It was introduced to deal with a pretty unimportant bug:

https://lampsvn.epfl.ch/trac/scala/ticket/3174

so I suspect we'll just revert it. It doesn't bode well for the life
span of RC4 that francois hit it that quickly.

On Sat, Jun 12, 2010 at 11:16 PM, Paul Phillips <paulp@improving.org> wrote:

On Sat, Jun 12, 2010 at 01:54:18PM -0700, David Pollak wrote:
> Should we hold off for an RC5?

Yes, please do. I meant to fix a 2.7 regression which I thought was seriously wrong. The cure was worse than the desease. We'll publish an RC5 on Monday.

Cheers

 -- Martin

On Sun, Jun 13, 2010 at 02:05:38PM +0200, martin odersky wrote:
> Yes, please do. I meant to fix a 2.7 regression which I thought was
> seriously wrong. The cure was worse than the desease. We'll publish an
> RC5 on Monday.

OK, in that case (and I'm not looking to be the guy dragging things out
here, only trying to solve the most visible issues, and my sense is any
RC may be the last) I just checked in two patches which I propose belong
in RC5. The tickets they address are 3420 and 3550. I'm not worried
about the groupBy patch causing any troubles, but iulian should take a
look at the inliner patch to make sure I didn't restrain inlining more
than I was supposed to. Since we're shipping optimized RC builds, and
this patch cuts down on a ton of noise, I think it would be popular.

On Sun, Jun 13, 2010 at 6:24 PM, Paul Phillips <paulp@improving.org> wrote:

On Sun, Jun 13, 2010 at 02:05:38PM +0200, martin odersky wrote:
> Yes, please do. I meant to fix a 2.7 regression which I thought was
> seriously wrong. The cure was worse than the desease. We'll publish an
> RC5 on Monday.

OK, in that case (and I'm not looking to be the guy dragging things out
here, only trying to solve the most visible issues, and my sense is any
RC may be the last) I just checked in two patches which I propose belong
in RC5.  The tickets they address are 3420 and 3550.  I'm not worried
about the groupBy patch causing any troubles, but iulian should take a
look at the inliner patch to make sure I didn't restrain inlining more
than I was supposed to.  Since we're shipping optimized RC builds, and
this patch cuts down on a ton of noise, I think it would be popular.

Hi Paul,

Did you merge them into 2.8.x or only leave them in trunk? There are several issues at work here

 - we want to get to 2.8.0 final, quickly
 - we want to avoid having a broken RC out there for any length of time.

Toni already built RC5 internally. He can build again if we decide it's necessary, but that would only happen tomorrow I guess. I have no issues with the groupedBy patch, but the inline patch at this moment of the release process scares me. I think it's much better in 2.8.1. I don't yet understand the full problem, though. If ->
cannot be inlined (and we get a warning), why is it marked @inline? Can we not fix this by removing the @inline?

Cheers

 -- Martin

On Sun, Jun 13, 2010 at 06:55:54PM +0200, martin odersky wrote:
> Did you merge them into 2.8.x or only leave them in trunk?

I'm leaving everything in trunk at this point - if I check it in I am
indicating I think it should be in 2.8.0 (this is true for all patches I
have in trunk which aren't merged) but I don't want to impede the
release process in any way, so they are all "recommendation only." Also,
I'm kind of allergic to svn, having gotten this far using git almost
exclusively. But mainly the first reason.

> I have no issues with the groupedBy patch, but the inline patch at
> this moment of the release process scares me. I think it's much better
> in 2.8.1.

That is fine, but in that case I should at least submit a patch which
suppresses that warning so it doesn't bury any legitimate warnings.

> I don't yet understand the full problem, though. If -> cannot be
> inlined (and we get a warning), why is it marked @inline?

(I should preface this explanation by admitting I am learning about
optimization as I go. Hopefully this is accurate. I encourage iulian
to correct me if I have anything wrong.)

It can be inlined. Looking at the class:

final class ArrowAssoc[A](val x: A) {
@inline def -> [B](y: B): Tuple2[A, B] = Tuple2(x, y)
def →[B](y: B): Tuple2[A, B] = ->(y)
}

The issue only arises when scalac is itself built with -optimise. Since
getters are inlined, this leaves -> accessing the private field for x.
Then when the time comes to inline ->, scalac is confused and frightened
because -> is apparently working with private data.

So the only behavior changing part of my patch is to tweak the heuristic
about what can be inlined to treat the presence of getters as equivalent
to the presence of private members. As I understand it, the method will
still be inlined later, at the caller site, and iulian says he is going
to review whether it is useful to inline getters at all. But for now
this seemed like the compromise.

> Can we not fix this by removing the @inline?

Presumably this too would fix it, but only in this one spot, and we'd
lose this useful inlining entirely.

martin odersky wrote:
> There are several issues at work here
>
> - we want to get to 2.8.0 final, quickly
> - we want to avoid having a broken RC out there for any length of time.

I pushed RC5 out immediately in order to avoid having the broken RC4 out
for too long. If there are further improvements that prove to be really,
really necessary, we can always include them in a further cycle.
Toni

I would perfer to do it on Monday and include Paul's patch. Having spurious warnings is serious enough to merit delaying an RC by 12 hours or so.
iulian

On Sun, Jun 13, 2010 at 8:31 PM, Antonio Cunei <antonio.cunei@epfl.ch> wrote:

martin odersky wrote:

There are several issues at work here

 - we want to get to 2.8.0 final, quickly
 - we want to avoid having a broken RC out there for any length of time.

I pushed RC5 out immediately in order to avoid having the broken RC4 out for too long. If there are further improvements that prove to be really, really necessary, we can always include them in a further cycle.
Toni

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

On Sun, Jun 13, 2010 at 9:33 PM, iulian dragos <iulian.dragos@epfl.ch> wrote:

I would perfer to do it on Monday and include Paul's patch. Having spurious warnings is serious enough to merit delaying an RC by 12 hours or so.

Seems like you missed the window of opportunity :/
 

iulian

On Sun, Jun 13, 2010 at 8:31 PM, Antonio Cunei <antonio.cunei@epfl.ch> wrote:

martin odersky wrote:

There are several issues at work here

 - we want to get to 2.8.0 final, quickly
 - we want to avoid having a broken RC out there for any length of time.

I pushed RC5 out immediately in order to avoid having the broken RC4 out for too long. If there are further improvements that prove to be really, really necessary, we can always include them in a further cycle.
Toni

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

--
Viktor Klang
| "A complex system that works is invariably
| found to have evolved from a simple system
| that worked." - John Gall

Akka - the Actor Kernel: Akkasource.org
Twttr: twitter.com/viktorklang

On Sun, Jun 13, 2010 at 7:31 PM, Antonio Cunei wrote:
> I pushed RC5 out immediately in order to avoid having the broken RC4 out for
> too long. If there are further improvements that prove to be really, really
> necessary, we can always include them in a further cycle.

Assuming that -optimise is meant to be production-ready, I'd say that
suppressing the warning one way or another (with the proper fix or by
removing @inline from ArrowAssoc) is very important as builds are
currently very noisy (using -> for tuples is very common).

A specialization issue that looks to be nasty (code that should be
executed doesn't get executed) was also filed recently, so it may be
worth checking if that's a blocker as it may make the decision on
whether to have another RC easier:

https://lampsvn.epfl.ch/trac/scala/ticket/3558

Best,
Ismael

Further specialization tickets are certainly for 2.8.1, not the next RC. I agree the warning is annoying, and it is really pity we could not fix it earlier. But now I think it is too late unless something else causes a new RC. It only appears when -optimize is set and it is only a spurious warning, not an error. Fix it in 2.8.1 I'd say.

Cheers

 -- Martin

On Sun, Jun 13, 2010 at 10:34 PM, martin odersky wrote:
> Further specialization tickets are certainly for 2.8.1, not the next RC.

Fair enough. Given that issues about this continue to be filed and
there are no plans to fix them before 2.8.1 (including the mentioned
case of incorrect code execution), it is perhaps a good idea to
include something in the release notes alerting users about using the
annotation in their own code. Particularly since the option is enabled
by default and there is no hint about the maturity of the feature in
the scaladoc for the annotation.

> I agree the warning is annoying, and it is really pity we could not fix it
> earlier. But now I think it is too late unless something else causes a new
> RC. It only appears when -optimize is set and it is only a spurious warning,
> not an error. Fix it in 2.8.1 I'd say.

The main issue is that it makes warnings useless since there are so
many of them. Disabling -optimise is a fair workaround (particularly
when compared to the state of affairs in 2.7.x) and it's what I'll do.

Best,
Ismael

On Mon, Jun 14, 2010 at 12:00 AM, Ismael Juma <mlists@juma.me.uk> wrote:

On Sun, Jun 13, 2010 at 10:34 PM, martin odersky <martin.odersky@epfl.ch> wrote:
> Further specialization tickets are certainly for 2.8.1, not the next RC.

Fair enough. Given that issues about this continue to be filed and
there are no plans to fix them before 2.8.1 (including the mentioned
case of incorrect code execution), it is perhaps a good idea to
include something in the release notes alerting users about using the
annotation in their own code. Particularly since the option is enabled
by default and there is no hint about the maturity of the feature in
the scaladoc for the annotation.

Agreed. We should put text into the release note saying that this is a new and
somewhat immature feature that will be improved in following releases.
 

> I agree the warning is annoying, and it is really pity we could not fix it
> earlier. But now I think it is too late unless something else causes a new
> RC. It only appears when -optimize is set and it is only a spurious warning,
> not an error. Fix it in 2.8.1 I'd say.

The main issue is that it makes warnings useless since there are so
many of them. Disabling -optimise is a fair workaround (particularly
when compared to the state of affairs in 2.7.x) and it's what I'll do.

You mean, not compiling with -optimize, right? AFAIK, -optimize is off by default.

Cheers

 -- Martin

On Sun, Jun 13, 2010 at 11:07 PM, martin odersky wrote:
> You mean, not compiling with -optimize, right? AFAIK, -optimize is off by
> default.

Yes, that's what I mean. Sorry for not being clear. I had enabled it
in our codebase one or two months ago in order to help find bugs since
the Scala distribution is now compiled with -optimise (for what is
worth, with the exception of ticket #3476[1] which was not hard to
workaround and the mentioned #3420, things seemed to work fine).

Best,
Ismael

[1] https://lampsvn.epfl.ch/trac/scala/ticket/3476