Outer sends need to check both super classes and mixins #189

ghost · 2017-10-10T03:35:43Z

Classes created via mixin application should preserve access to variables and methods bound in enclosing scopes.

Before these changes classes created via mixin application were not able to preserve this access; in particular, SOMns would fail to specialize ResolvingImplicitReceiverSend when one of the applied mixin's methods accessed something defined in its enclosing activation.

For example in the following program (before these changes) SOMns would fail to specialize the implicit request for bar (from inside A.foo):

class MixinAppTest = ()(
  
  public class A = ()(
    public foo = ( ^ bar )
  )

  public class B = Object <: A ()()
  public bar = ( ^ 1 )

  public main: args = (
    (B new) foo.
  )
)

These changes enable SOMns to correctly resolve implicit requests when made by the methods of an applied mixin. In particular, logic has been added to OuterObjectRead.getEnclosingClass(MixinDefinitionId) to make this possible. As before, each super class (from shallowest to deepest) is checked to see if it matches the parameter MixinDefinitionId. If no viable super class is found, then each of the mixin's applied to each super class (again from shallowest to deepest) are checked to see if they match the parameter MixinDefinitionId. In both cases, the first superclass to meet the condition is selected as the class who should resolve the outer read.

With the above changes, SOMns is able to recognise that the B contains within it a mixin that is based on A and, consequently, can specialize the implicit request for bar with B as the receiver. Therefore, with these changes SOMns can preserve access to variables and methods bound in enclosing scopes for the methods of applied mixins.

smarr

I have no idea whether the code is correct, haven't even tried to think about it. Sorry.

But, it looks good :)
Just some minor things.

Not sure what's going on with CI, we might need to update mx to get it working first. Would be worth a try.

smarr · 2017-10-10T08:47:43Z

core-lib/TestSuite/MixinApplicationTests.ns

@@ -0,0 +1,54 @@
+(* Copyright (c) 2001-2017 see AUTHORS file


You don't like: https://github.com/smarr/SOMns/blob/master/core-lib/TestSuite/MixinTests.ns ?
Please place the tests next to the existing ones.
Please also check whether this resolves any of the issues with the currently deactivated tests in there.

I've examined the deactivated tests. Some success - two are fixed, three remain broken. Out of the three broken tests:

(testConflictingSlotsImmutable) I don't understand which answer is correct

(TestInitializer1) A slot (that should exist) is not found. Unsure how to debug

(TestInitializer3) I don't whether a class resulting from a mixin application be able to access mixed in methods annotated as private?

Some more details below:

testConflictingSlotsImmutable - Broken

c is an instance of ConflictingSlotsImmutable, a class that inherits from Object and applies a mixin X that declares a public slot x. The initer method for ConflictingSlotsImmutable defines a new public slot x with initer expression x = 1.

After the initer method runs, x is equal to 1. The test asserts that slot x is nil. I'm unsure which answer is correct.

testConflictingMethods - Fixed

c is an instance of ConflictingMethods, a class that inherits from Y and applies a mixin Y2. Both Y and Y2 define a method foo. The test now passes with the correct result (Y2's foo wins).

TestInitializer1 - Broken

c is an instance of C, which inherits from object and applies mixins X (first) and X2 (second). Both X and X2 define a public slot x with an initer expression. SOMns throws an assertion in this case (of which I'm unsure how to debug):

java.lang.AssertionError at som.vmobjects.SObject.getLocation(SObject.java:485) at som.vmobjects.SObject.writeSlot(SObject.java:509)

TestInitializer2 - Fixed

c is an instance of C, which inherits from object and applies mixins X (first) and X2 (second). Both X and X2 define a public slot x. Both X and X2 define an initer that assigns x a different number. The test now passes with the correct result (X2's initer wins).

TestInitializer3 - Broken

c is an instance of C, which inherits from object and applies mixins X (first) and X2 (second). X defines a private method cnt and X's initer method uses this variable to set a public field x. When the tests runs, c does not understand the message cnt.

Should a class resulting from a mixin application be able to access mixed in methods annotated as private?

Should a class resulting from a mixin application be able to access mixed in methods annotated as private?

This is a question for the spec. I don't know by hard, but I would suspect so.

About the slot related issues. I won't be able to dive into that at the moment, so, let's just ignore those for now. Except if you need that to work for Grace.

Let's ignore it for now. The current implementation is at least a step in the right direction (at least, its sufficient for Grace since I won't use slots at the moment). I'd be happy to readdress the slot issues next month and make any changes to fix up three tests that remain broken.

smarr · 2017-10-10T08:54:21Z

src/som/interpreter/nodes/OuterObjectRead.java

@@ -87,6 +87,9 @@ public boolean isSuperSend() {

  protected final SClass getEnclosingClass(final SObjectWithClass rcvr) {
    SClass lexicalClass = rcvr.getSOMClass().getClassCorrespondingTo(mixinId);


This is silly nitpicking, but, I just had issues reading this code, so, could we use a rcvrClass local variable?
Then we also avoid twice the .getSOMClass(). Not that it matter performance-wise, but I have to think of one indirection less.

smarr · 2017-10-10T08:55:58Z

src/som/interpreter/objectstorage/ClassFactory.java

@@ -120,6 +121,15 @@ public boolean hasOnlyImmutableFields() {
    return superclassAndMixins;
  }

+  public boolean isBasedOn(final MixinDefinitionId mixinId) {
+    for (SClass cls : getSuperclassAndMixins()) {


Could you add a call to VM.callerNeedsToBeOptimized("Result of isBasedOn should be cached") before the loop?

smarr · 2017-10-10T08:56:31Z

src/som/interpreter/objectstorage/ClassFactory.java

@@ -120,6 +121,15 @@ public boolean hasOnlyImmutableFields() {
    return superclassAndMixins;
  }

+  public boolean isBasedOn(final MixinDefinitionId mixinId) {


Could you add a comment what the semantics of this now is?

smarr · 2017-10-10T08:57:12Z

src/som/vmobjects/SClass.java

@@ -173,7 +173,7 @@ public void initializeStructure(final MixinDefinition mixinDef,
    // assert instanceClassGroup != null || !ObjectSystem.isInitialized();
  }

-  private boolean isBasedOn(final MixinDefinitionId mixinId) {
+  public boolean isBasedOn(final MixinDefinitionId mixinId) {


Same here, could you add a comment what the semantics of this now is?
(not yet awake, and I haven't read this code in ages, so not sure what I was thinking here back then. sorry)

smarr · 2017-10-10T08:57:42Z

src/som/vmobjects/SClass.java

@@ -204,6 +204,16 @@ public SClass getClassCorrespondingTo(final MixinDefinitionId mixinId) {
    return cls;
  }

+  public SClass getClassWithMixinCorrespondingTo(final MixinDefinitionId mixinId) {
+    VM.callerNeedsToBeOptimized(
+        "This should not be on the fast path, specialization/caching needed?");


smarr · 2017-10-10T09:00:52Z

src/som/vmobjects/SClass.java

@@ -204,6 +204,16 @@ public SClass getClassCorrespondingTo(final MixinDefinitionId mixinId) {
    return cls;
  }

+  public SClass getClassWithMixinCorrespondingTo(final MixinDefinitionId mixinId) {


hmm, I don't like the name.
If I ask clazz.getClass*(), I think I expect the class of the class with some property.
How about just getMixin(..)?

Also see my comment for getClassCorrespondingTo()

getClassWithMixinCorrespondingTo returns the receiver class or one of its superclasses (not the mixin's class). The class returned is that one who has had the given mixin applied. For example if we have an class C defined as:

Mixin = class()() B = class Object <: Mixin ()() C = class B ()()

Then the result of C. getClassWithMixinCorrespondingTo(Mixin) is B, because the mixin was applied to B. Does that make sense? Do you have a suggestion for the method name, given this example?

Wait, let's just talk about the java level and the naming, not the functionality on the language level.

The first thing that getClassWithMixinCorrespondingTo(.) can return is this (line 210).
The name of the method does not make me expect that.

If I ask any object in Java this.getClass(), I'll never get this returned, right?

Perhaps the get is bad. Perhaps what this is here is actually a lookup.
Perhaps just lookupClass(mixinId)?

smarr · 2017-10-10T09:03:06Z

src/som/vmobjects/SClass.java

+    }
+    return cls;
+  }
+
  @ExplodeLoop
  public SClass getClassCorrespondingTo(final int superclassIdx) {


And I suppose you were just following my example? Well, looks like i changed my mind here, didn't I?
Hmmm, well, don't know. The CorrespondingTo seems superfluous. Maybe it isn't.
Ok, I'll leave it up to you.

I agree that CorrespondingTo is not needed. I will remove. See my previous comment regarding the naming for getClassWithMixinCorrespondingTo and we we can go from there.

smarr · 2017-10-10T09:29:48Z

Looks like the CI issue was temporary.

ghost · 2017-10-12T19:46:28Z

I've adapted the PR to meet the requested changes. I'm not sure that I've done a great job of the new docstrings (ClassFactory.isBasedOn, SClass.isBasedOn, and OuterObjectRead.getEnclosingClass). Please review before accepting!

- publicized access to SClass.isBasedOn() - added isBasedOn method to ClassFactory - outer object read now checks both classes and mixins - renamed method for looking up class

- added test where mixin invoke outer method - added test where mixin accesses outer slots

smarr · 2017-10-19T09:15:57Z

As you might have seen, I just briefly checked for the expected behavior with the Newspeak experts. Just to avoid reading the spec carefully, I suppose 😆

I also compress the history a bit. Seemed useful to have the reason for the refactorings in the same commit. But, I probably have a different opinion on it every day...

Will merge once CI passed.

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr requested changes Oct 10, 2017

View reviewed changes

smarr changed the base branch from master to dev October 18, 2017 21:32

smarr added the bug Fixes an issue, incorrect implementation label Oct 18, 2017

smarr added this to the v0.6.0 - Black Diamonds milestone Oct 18, 2017

Richard Roberts added 2 commits October 18, 2017 22:40

Fix sends to outer methods from mixins

283f16c

- publicized access to SClass.isBasedOn() - added isBasedOn method to ClassFactory - outer object read now checks both classes and mixins - renamed method for looking up class

Added tests and activated fixes ones

a2b19c6

- added test where mixin invoke outer method - added test where mixin accesses outer slots

smarr changed the title ~~Outer reads check both super classes and mixins~~ Outer sends need to check both super classes and mixins Oct 19, 2017

smarr approved these changes Oct 19, 2017

View reviewed changes

smarr merged commit 986fa56 into smarr:dev Oct 19, 2017

smarr added a commit that referenced this pull request Nov 5, 2017

Enable Lookup.testInner18, was fixed with PR #189

e6526ea

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr mentioned this pull request Nov 5, 2017

Issue with Basic Interpreter Test "Lookup", "testInner18" #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outer sends need to check both super classes and mixins #189

Outer sends need to check both super classes and mixins #189

ghost commented Oct 10, 2017 •

edited by smarr

Loading

smarr left a comment

smarr Oct 10, 2017

ghost Oct 11, 2017

smarr Oct 12, 2017

ghost Oct 12, 2017 •

edited by ghost

Loading

smarr Oct 10, 2017

smarr Oct 10, 2017

smarr Oct 10, 2017

smarr Oct 10, 2017

smarr Oct 10, 2017

smarr Oct 10, 2017

ghost Oct 11, 2017 •

edited by ghost

Loading

smarr Oct 12, 2017

smarr Oct 12, 2017

smarr Oct 10, 2017

ghost Oct 11, 2017

smarr commented Oct 10, 2017

ghost commented Oct 12, 2017 •

edited by ghost

Loading

smarr commented Oct 19, 2017

		@@ -87,6 +87,9 @@ public boolean isSuperSend() {

		protected final SClass getEnclosingClass(final SObjectWithClass rcvr) {
		SClass lexicalClass = rcvr.getSOMClass().getClassCorrespondingTo(mixinId);

Outer sends need to check both super classes and mixins #189

Outer sends need to check both super classes and mixins #189

Conversation

ghost commented Oct 10, 2017 • edited by smarr Loading

smarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 12, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 11, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarr commented Oct 10, 2017

ghost commented Oct 12, 2017 • edited by ghost Loading

smarr commented Oct 19, 2017

ghost commented Oct 10, 2017 •

edited by smarr

Loading

ghost Oct 12, 2017 •

edited by ghost

Loading

ghost Oct 11, 2017 •

edited by ghost

Loading

ghost commented Oct 12, 2017 •

edited by ghost

Loading