Merge pull request #554 in G/fastr from ~LUKAS.STADLER_ORACLE.COM/fastr:setAttrFixCheck to master

* commit 'a57c10f2':
  add canStore and canSet checks to SetFixedAttributeNode
  A small fix in SetAttributeNode

com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/attributes/AttributeAccessNode.java

@@ -68,4 +68,24 @@ public abstract class AttributeAccessNode extends RBaseNode {
         return shape != null && shape.check(attrs);
     }
 
+    protected static Shape defineProperty(Shape oldShape, Object name, Object value) {
+        return oldShape.defineProperty(name, value, 0);
+    }
+
+    /**
+     * There is a subtle difference between {@link Location#canSet} and {@link Location#canStore}.
+     * We need {@link Location#canSet} for the guard of {@code setExistingAttrCached} because there
+     * we call {@link Location#set}. We use the more relaxed {@link Location#canStore} for the guard
+     * of {@code setNewAttrCached} because there we perform a shape transition, i.e., we are not
+     * actually setting the value of the new location - we only transition to this location as part
+     * of the shape change.
+     */
+    protected static boolean canSet(Location location, Object value) {
+        return location.canSet(value);
+    }
+
+    /** See {@link #canSet} for the difference between the two methods. */
+    protected static boolean canStore(Location location, Object value) {
+        return location.canStore(value);
+    }
 }

com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/attributes/SetAttributeNode.java

@@ -78,9 +78,9 @@ public abstract class SetAttributeNode extends AttributeAccessNode {
                     @Cached("lookupShape(attrs)") Shape shape,
                     @Cached("lookupLocation(shape, name, value)") Location location) {
         try {
-            location.set(attrs, value);
+            location.set(attrs, value, shape);
         } catch (IncompatibleLocationException | FinalLocationException ex) {
-            RInternalError.reportError(ex);
+            throw RInternalError.shouldNotReachHere(ex);
         }
     }
 
@@ -105,7 +105,7 @@ public abstract class SetAttributeNode extends AttributeAccessNode {
         try {
             newLocation.set(attrs, value, oldShape, newShape);
         } catch (IncompatibleLocationException ex) {
-            RInternalError.reportError(ex);
+            throw RInternalError.shouldNotReachHere(ex);
         }
     }
 
@@ -189,26 +189,4 @@ public abstract class SetAttributeNode extends AttributeAccessNode {
 
         return location;
     }
-
-    protected static Shape defineProperty(Shape oldShape, Object name, Object value) {
-        return oldShape.defineProperty(name, value, 0);
-    }
-
-    /**
-     * There is a subtle difference between {@link Location#canSet} and {@link Location#canStore}.
-     * We need {@link Location#canSet} for the guard of {@link #setExistingAttrCached} because there
-     * we call {@link Location#set}. We use the more relaxed {@link Location#canStore} for the guard
-     * of {@link #setNewAttrCached} because there we perform a shape transition, i.e., we are not
-     * actually setting the value of the new location - we only transition to this location as part
-     * of the shape change.
-     */
-    protected static boolean canSet(Location location, Object value) {
-        return location.canSet(value);
-    }
-
-    /** See {@link #canSet} for the difference between the two methods. */
-    protected static boolean canStore(Location location, Object value) {
-        return location.canStore(value);
-    }
-
 }

com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/attributes/SetFixedAttributeNode.java

@@ -86,7 +86,7 @@ public abstract class SetFixedAttributeNode extends FixedAttributeAccessNode {
     public abstract void execute(Object attr, Object value);
 
     @Specialization(limit = "3", //
-                    guards = {"shapeCheck(shape, attrs)", "location != null"}, //
+                    guards = {"shapeCheck(shape, attrs)", "location != null", "canSet(location, value)"}, //
                     assumptions = {"shape.getValidAssumption()"})
     protected void setAttrCached(DynamicObject attrs, Object value,
                     @Cached("lookupShape(attrs)") Shape shape,
@@ -94,12 +94,12 @@ public abstract class SetFixedAttributeNode extends FixedAttributeAccessNode {
         try {
             location.set(attrs, value, shape);
         } catch (IncompatibleLocationException | FinalLocationException ex) {
-            RInternalError.reportError(ex);
+            throw RInternalError.shouldNotReachHere(ex);
         }
     }
 
     @Specialization(limit = "3", //
-                    guards = {"shapeCheck(oldShape, attrs)", "oldLocation == null"}, //
+                    guards = {"shapeCheck(oldShape, attrs)", "oldLocation == null", "canStore(newLocation, value)"}, //
                     assumptions = {"oldShape.getValidAssumption()", "newShape.getValidAssumption()"})
     protected static void setNewAttrCached(DynamicObject attrs, Object value,
                     @Cached("lookupShape(attrs)") Shape oldShape,
@@ -109,7 +109,7 @@ public abstract class SetFixedAttributeNode extends FixedAttributeAccessNode {
         try {
             newLocation.set(attrs, value, oldShape, newShape);
         } catch (IncompatibleLocationException ex) {
-            RInternalError.reportError(ex);
+            throw RInternalError.shouldNotReachHere(ex);
         }
     }