Skip to content

SI-1909 SI-3832 SI-7007 SI-7223 Improved handling of larval objects#2884

Merged
retronym merged 5 commits intoscala:masterfrom
retronym:ticket/3832
Sep 15, 2013
Merged

SI-1909 SI-3832 SI-7007 SI-7223 Improved handling of larval objects#2884
retronym merged 5 commits intoscala:masterfrom
retronym:ticket/3832

Conversation

@retronym
Copy link
Member

Reclaims some ground lost in the first swing at SI-6666, and solves a few more issues to boot.

I'm hopeful that I'll be able to knock off the residuals of SI-6666 too, so I might push one or two more commits.

Review by @JamesIry

@ghost ghost assigned JamesIry Aug 28, 2013
@retronym
Copy link
Member Author

I'll close this temporarily until I address the failures.

@retronym retronym closed this Aug 28, 2013
@retronym retronym reopened this Aug 28, 2013
@retronym
Copy link
Member Author

Reopened for business.

@retronym
Copy link
Member Author

I'm done with this one. I can't actually find anything residual from SI-6666, so we can close that one too.

@gkossakowski
Copy link
Contributor

@JamesIry: ping

@magarciaEPFL
Copy link
Contributor

What's the status of this PR? In particular, will it make to M5?

@gkossakowski
Copy link
Contributor

@magarciaEPFL: I hope so. I'll try to steal @JamesIry's attention tomorrow.

@magarciaEPFL
Copy link
Contributor

@gkossakowski Did you get @JamesIry 's attention yesterday?

@adriaanm
Copy link
Contributor

adriaanm commented Sep 6, 2013

I apologize on behalf of the reviewer for this not making the deadline.

@retronym
Copy link
Member Author

retronym commented Sep 6, 2013

This one isn't super urgent.

@gkossakowski
Copy link
Contributor

@retronym: please fix a19e05371fb0a57263b68c1ced0ba88e830e4a61 (both commit message and test file names).

Other than that it can go in. Related to this PR: wouldn't make sense to perform all those checks at some late phase (genicode) when we see the final shape of trees and we perform checks that are very backend specific?

When we're in the neighbourhood of VerifyErrors, it's better to run
the code.

This change is leading up to a fix for SI-3832, which regressed
with fix for SI-1909.
In order to reduce the noise in OuterPathTransformer.
SI-1909 modified LambdaLift to lift in auxiliary constructors methods as STATIC
so they could be called before the self-constructor was called.

That allowed for:

     class Foo (x: Int) {
       def this() = this( {
         def bar() = 5
         bar
       })
    }

However, if the method is in a statement that trails the self constructor call,
this is unnecessary and in fact incorrect as it robs the lifted method of `this`.

This commit uses the machinery established in SI-6666 to limit the STATIC-ness
of lifted methods to those used in arguments for self-constructor calls.

This is used exclusively; the `isAuxillaryConstructor` check wasn't the right
way to solve this, as was seen by the regression it caused in SI-3832.
A new test case shows that we can statically lift methods in super-constructor
calls, rather than just self-constructor calls.

We also have to avoid statically lifting objects in these positions. For now,
I just emit a dev warning that a VerifyError is in your future. With some more
thought we could escalate that to a implementation restriction and emit an error.
Rather than the old behaviour, which compiled successfully
but led us into the jaws of a LinkageError.

Related to SI-6666.
@retronym
Copy link
Member Author

I've just rebased and fixed the last commit message and test file name.

It's a clunky flag used to determine very early on whether
we're in the self-call, super-call or early-init section.

In SI-6666 / fd61254, the check was improved to consider nesting.
But, that caused this regression, as Function's haven't been
translated to classes yet, so our check for enclosing non-term
owners failed wrongly flagged definitins body of a anonymous function
as INCONSTRUCTOR.

With this patch, we correctly flag:

    class C extends D {
      // INCONSTRUCTOR
      () => {
        !INCONSTRUCTOR
      }
      // INCONSTRUCTOR
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants