regressometer is wriggling

The only changes I made for NotNull is r21664 and r21951. Note, that
while investigating #3440 I found a serious bug in Typer - basically we
neglect the NotNullType relatively early because of a typo. It is
possible that my changes have woken up the NotNull beast that was
lurking around for some time now.
Martin suggested that we fix it in 2.8.1 since it can affect a lot of
people, so I will revert the patches.

hubert

Paul Phillips wrote:
> https://lampsvn.epfl.ch/trac/scala/ticket/3568
>
> This one looks like a problem. I can reproduce it, including the fact
> that it worked in RC3 but not RC5. And does anyone have a clear picture
> of why NotNull has become a regular presence in the error messages of
> people who aren't using it?
>
> src/buffer/Buffer.scala:37: error: class ViewFloat1 needs to be abstract, since:
> method flatMap in trait FilterMonadic of type [B,That](f: (buffer.Float1#Element with NotNull) => Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[IndexedSeq[Float],B,That])That is not defined
> method map in trait FilterMonadic of type [B,That](f: (buffer.Float1#Element with NotNull) => B)(implicit bf: scala.collection.generic.CanBuildFrom[IndexedSeq[Float],B,That])That is not defined
> class ViewFloat1 extends BaseFloat1 with DataView[Float1] {
> one error found
>
>

On Tue, Jun 15, 2010 at 11:10 AM, Hubert Plociniczak <hubert.plociniczak@epfl.ch> wrote:

The only changes I made for NotNull is r21664 and r21951. Note, that while investigating #3440 I found a serious bug in Typer - basically we neglect the NotNullType relatively early because of a typo. It is possible that my changes have woken up the NotNull beast that was lurking around for some time now.
Martin suggested that we fix it in 2.8.1 since it can affect a lot of people, so I will revert the patches.

Yes, we need to revert. these. It also means we need a new RC. It should be clear by now that
we need to come to a fixpoint now -- this has been dragging on for too long. So, let's fix what really needs fixing, but no more risky fixes!

What needs fixing?

 - NotNull code needs to be reverted
 - @inline wanrnings should be reduced
 - the specialization bug that Iulian mentioned
 - Paul's groupBy patch (? not sure, it's not critical but looks riskless)

Anything else?

Thanks

 -- Martin

On Tue, Jun 15, 2010 at 10:23 AM, martin odersky wrote:
> Anything else?

Maybe I missed the reasoning for this (if so, I was not the only one
as Iulian also seemed unsure), but is the change that caused ticket
#3536[1] intended? I was under the impression that methods would be
strict by default and having Set.toSeq return a lazy collection goes
against that. Maybe there's a good reason, but I'd like to make sure
since we're so close to 2.8.0 now.

Best,
Ismael

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

On Tue, Jun 15, 2010 at 2:49 PM, Ismael Juma <mlists@juma.me.uk> wrote:

On Tue, Jun 15, 2010 at 10:23 AM, martin odersky <martin.odersky@epfl.ch> wrote:
> Anything else?

Maybe I missed the reasoning for this (if so, I was not the only one
as Iulian also seemed unsure), but is the change that caused ticket
#3536[1] intended? I was under the impression that methods would be
strict by default and having Set.toSeq return a lazy collection goes
against that. Maybe there's a good reason, but I'd like to make sure
since we're so close to 2.8.0 now.

I did not realize until now it was a recent regression. Paul, can you comment? It seems we should override toSeq in Traversable to be toList, what it was before.

Cheers

 -- Martin

I found that my NotNull commits aren't the reason for #3568. I did some
manual checking and I found that the problem occurs for revisions
between r22174 and r22181. I guess r22178 is the culprit. Martin?
This doesn't explain the existence of NotNull that Paul mentioned but I
think I have a rough feeling why it is there.

hubert

martin odersky wrote:
>
>
> On Tue, Jun 15, 2010 at 11:10 AM, Hubert Plociniczak
> > wrote:
>
> The only changes I made for NotNull is r21664 and r21951. Note,
> that while investigating #3440 I found a serious bug in Typer -
> basically we neglect the NotNullType relatively early because of a
> typo. It is possible that my changes have woken up the NotNull
> beast that was lurking around for some time now.
> Martin suggested that we fix it in 2.8.1 since it can affect a lot
> of people, so I will revert the patches.
>
>
> Yes, we need to revert. these. It also means we need a new RC. It
> should be clear by now that
> we need to come to a fixpoint now -- this has been dragging on for too
> long. So, let's fix what really needs fixing, but no more risky fixes!
>
> What needs fixing?
>
> - NotNull code needs to be reverted
> - @inline wanrnings should be reduced
> - the specialization bug that Iulian mentioned
> - Paul's groupBy patch (? not sure, it's not critical but looks riskless)
>
> Anything else?
>
> Thanks
>

On Tue, Jun 15, 2010 at 5:58 PM, Hubert Plociniczak <hubert.plociniczak@epfl.ch> wrote:

I found that my NotNull commits aren't the reason for #3568. I did some manual checking and I found that the problem occurs for revisions between r22174 and r22181. I guess r22178 is the culprit. Martin?

It seems that the addition of FilterMonadic in 22178 triggered the error. But it can't be the error, that most likely is elsewhere. I still find the NonNull occurrences suspicious. It would be good to get rid of them. Can somebody else please investigate this further? I am running out of time to do it.

Thanks

 -- Martin

On Tue, Jun 15, 2010 at 11:23:56AM +0200, martin odersky wrote:
> What needs fixing?
>
> - NotNull code needs to be reverted
> - @inline wanrnings should be reduced
> - the specialization bug that Iulian mentioned
> - Paul's groupBy patch (? not sure, it's not critical but looks riskless)

I endorse the approximately riskless groupBy change. I feel very good
about my inliner patch but there's no reason to commit something so big
to 2.8.x: I can (and will right now) create a much smaller patch which
contains only the one fix.

> Anything else?

Only to have an aggressive schedule for 2.8.1! And I'm kind of deflated
that every parameter name in the entire library is about to enter the
2.8.0 API because of named arguments. If we are really committed to
some kind of backward compatibility for 2.8.0 can we perhaps fence off
some areas - like that one - where we admit to being less committed?

On Tue, Jun 15, 2010 at 04:24:08PM +0200, martin odersky wrote:
> I did not realize until now it was a recent regression. Paul, can you
> comment? It seems we should override toSeq in Traversable to be
> toList, what it was before.

The whole scene down there in the bottom layers is somewhat internally
conflicted. The motivation for that specific change was that calling
toStream on an Iterator would force the Iterator, which is way uncool
given that it starts lazy and ends lazy. It sounds like I ran into a
doorway with the actual implementation, and actually Hubert was already
out there waving a flag, and I've just been doing too many things.

Actually I propose that Hubert if he is up for it straighten out the
situation with toStream and friends, and I could look at the NotNull
slash FilterMonadic situation.

I can confirm that I can fix #3568 by attacking NotNull. Here is my
vote: I can turn NotNull completely off by default and throw in a -Y
compiler option to enable it. Its current usefulness is questionable at
best, we don't know what other bugs still lie dormant, and I see no good
reason to try to finesse it at this stage. We can render it bug free
with the stroke of a key except for the -Y masochists.

On Tue, Jun 15, 2010 at 06:10:57PM +0200, martin odersky wrote:
> It seems that the addition of FilterMonadic in 22178 triggered the
> error. But it can't be the error, that most likely is elsewhere. I
> still find the NonNull occurrences suspicious. It would be good to get
> rid of them. Can somebody else please investigate this further? I am
> running out of time to do it.

I have a patch all ready for this if my proposed remedy is acceptable.

Good to hear that you found it. I double checked that my recent changes
weren't the culprit. So the reason is just that NotNull is fundamentally
broken atm (as in the example of #3440).

I know that there are places at least in sbt which use NotNull. So I am
guessing that would need to be updated if we go ahead with your patch.

hubert

Paul Phillips wrote:
> I can confirm that I can fix #3568 by attacking NotNull. Here is my
> vote: I can turn NotNull completely off by default and throw in a -Y
> compiler option to enable it. Its current usefulness is questionable at
> best, we don't know what other bugs still lie dormant, and I see no good
> reason to try to finesse it at this stage. We can render it bug free
> with the stroke of a key except for the -Y masochists.
>

On Tue, Jun 15, 2010 at 10:43:32PM +0200, Hubert Plociniczak wrote:
> Good to hear that you found it. I double checked that my recent
> changes weren't the culprit. So the reason is just that NotNull is
> fundamentally broken atm (as in the example of #3440).
>
> I know that there are places at least in sbt which use NotNull. So I
> am guessing that would need to be updated if we go ahead with your
> patch.

No, I took the transparent route. This is the whole proposed change, at
def notNull:

- if (isNotNull || phase.erasedTypes) this else NotNullType(this)
+ if (!settings.Ynotnull.value || isNotNull || phase.erasedTypes) this
+ else NotNullType(this)

It is the compiler's attempts to propagate NotNull (and inject it when
calculating lub and glb and such) which cause all the trouble. No code
should have to be changed, although it might notice null with even less
vigilance than before.

+1

Paul Phillips wrote:
> No, I took the transparent route. This is the whole proposed change, at
> def notNull:
>
> - if (isNotNull || phase.erasedTypes) this else NotNullType(this)
> + if (!settings.Ynotnull.value || isNotNull || phase.erasedTypes) this
> + else NotNullType(this)
>
> It is the compiler's attempts to propagate NotNull (and inject it when
> calculating lub and glb and such) which cause all the trouble. No code
> should have to be changed, although it might notice null with even less
> vigilance than before.
>
>

Yes, let's do this. Hubert, can you promote the patch into 2.8? Glad we coud solve this.

Cheers

 -- Martin

On Tue, Jun 15, 2010 at 11:01 PM, Hubert Plociniczak <hubert.plociniczak@epfl.ch> wrote:

+1

Paul Phillips wrote:

No, I took the transparent route.  This is the whole proposed change, at def notNull:

-      if (isNotNull || phase.erasedTypes) this else NotNullType(this)
+      if (!settings.Ynotnull.value || isNotNull || phase.erasedTypes) this
+      else NotNullType(this)

It is the compiler's attempts to propagate NotNull (and inject it when calculating lub and glb and such) which cause all the trouble.  No code should have to be changed, although it might notice null with even less vigilance than before.