Skip to content

Commit 25a8e77

Browse files
steve-ssteve-s
committed
[GR-24716] Change isMapping{Type} messages to follow PyMapping_Check semantics.
PullRequest: graalpython/1113
2 parents 2157e73 + 2531059 commit 25a8e77

File tree

3 files changed

+47
-46
lines changed

3 files changed

+47
-46
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/PythonAbstractObject.java

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@
4343
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.AttributeError;
4444
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.TypeError;
4545
import static com.oracle.graal.python.nodes.SpecialMethodNames.FILENO;
46+
import static com.oracle.graal.python.nodes.SpecialMethodNames.ITEMS;
47+
import static com.oracle.graal.python.nodes.SpecialMethodNames.KEYS;
48+
import static com.oracle.graal.python.nodes.SpecialMethodNames.VALUES;
4649
import static com.oracle.graal.python.nodes.SpecialMethodNames.__BOOL__;
4750
import static com.oracle.graal.python.nodes.SpecialMethodNames.__CALL__;
4851
import static com.oracle.graal.python.nodes.SpecialMethodNames.__DELETE__;
@@ -136,16 +139,16 @@
136139
import com.oracle.graal.python.runtime.exception.PException;
137140
import com.oracle.graal.python.util.OverflowException;
138141
import com.oracle.truffle.api.CompilerDirectives;
139-
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
140142
import com.oracle.truffle.api.TruffleLanguage;
143+
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
141144
import com.oracle.truffle.api.dsl.Cached;
142-
import com.oracle.truffle.api.dsl.Cached.Exclusive;
143-
import com.oracle.truffle.api.dsl.Cached.Shared;
144145
import com.oracle.truffle.api.dsl.CachedContext;
145146
import com.oracle.truffle.api.dsl.GenerateUncached;
146147
import com.oracle.truffle.api.dsl.ImportStatic;
147148
import com.oracle.truffle.api.dsl.ReportPolymorphism;
148149
import com.oracle.truffle.api.dsl.Specialization;
150+
import com.oracle.truffle.api.dsl.Cached.Exclusive;
151+
import com.oracle.truffle.api.dsl.Cached.Shared;
149152
import com.oracle.truffle.api.interop.ArityException;
150153
import com.oracle.truffle.api.interop.InteropLibrary;
151154
import com.oracle.truffle.api.interop.InvalidArrayIndexException;
@@ -192,6 +195,21 @@ public final void clearNativeWrapper(ConditionProfile hasHandleValidAssumptionPr
192195
nativeWrapper = null;
193196
}
194197

198+
/**
199+
* Checks if the object is a Mapping as described in the
200+
* <a href="https://docs.python.org/3/reference/datamodel.html">Python Data Model</a>. Mappings
201+
* are treated differently to other containers in some interop messages.
202+
*/
203+
private static boolean isAbstractMapping(Object receiver, PythonObjectLibrary lib) {
204+
return lib.isSequence(receiver) && lib.lookupAttribute(receiver, KEYS, true) != PNone.NO_VALUE && //
205+
lib.lookupAttribute(receiver, ITEMS, true) != PNone.NO_VALUE && //
206+
lib.lookupAttribute(receiver, VALUES, true) != PNone.NO_VALUE;
207+
}
208+
209+
private boolean isAbstractMapping(PythonObjectLibrary thisLib) {
210+
return isAbstractMapping(this, thisLib);
211+
}
212+
195213
@ExportMessage
196214
public void writeMember(String key, Object value,
197215
@Exclusive @Cached PInteropSubscriptAssignNode setItemNode,
@@ -219,7 +237,7 @@ public void writeMember(String key, Object value,
219237
return;
220238
}
221239
}
222-
if (dataModelLibrary.isMapping(this)) {
240+
if (isAbstractMapping(dataModelLibrary)) {
223241
setItemNode.execute(this, key, value);
224242
} else {
225243
writeNode.execute(this, key, value);
@@ -278,7 +296,7 @@ public Object readMember(String key,
278296
@ExportMessage
279297
public boolean hasArrayElements(
280298
@CachedLibrary("this") PythonObjectLibrary dataModelLibrary) {
281-
return dataModelLibrary.isSequence(this) && !dataModelLibrary.isMapping(this);
299+
return dataModelLibrary.isSequence(this) && !isAbstractMapping(dataModelLibrary);
282300
}
283301

284302
@ExportMessage
@@ -290,7 +308,7 @@ public Object readArrayElement(long key,
290308
try {
291309
return toForeign.executeConvert(getItemNode.execute(this, key));
292310
} catch (PException e) {
293-
if (dataModelLibrary.isMapping(this)) {
311+
if (isAbstractMapping(dataModelLibrary)) {
294312
throw UnsupportedMessageException.create();
295313
} else {
296314
// TODO(fa) refine exception handling
@@ -505,8 +523,8 @@ public Object getMembers(boolean includeInternal,
505523
}
506524
if (includeInternal) {
507525
// we use the internal flag to also return dictionary keys for mappings
508-
if (dataModelLibrary.isMapping(this)) {
509-
PList mapKeys = castToList.executeWithGlobalState(keysNode.executeObject(this, SpecialMethodNames.KEYS));
526+
if (isAbstractMapping(dataModelLibrary)) {
527+
PList mapKeys = castToList.executeWithGlobalState(keysNode.executeObject(this, KEYS));
510528
int len = lenNode.execute(mapKeys);
511529
for (int i = 0; i < len; i++) {
512530
Object key = getItemNode.execute(mapKeys, i);
@@ -550,7 +568,7 @@ public void removeMember(String member,
550568
return;
551569
}
552570
}
553-
if (dataModelLibrary.isMapping(this) && getDelItemNode.execute(this, __DELITEM__) != PNone.NO_VALUE) {
571+
if (isAbstractMapping(dataModelLibrary) && getDelItemNode.execute(this, __DELITEM__) != PNone.NO_VALUE) {
554572
delItemNode.execute(this, member);
555573
} else {
556574
deleteAttributeNode.execute(this, member);
@@ -645,9 +663,9 @@ public boolean isMapping(@CachedLibrary("this") PythonObjectLibrary plib,
645663
public boolean isSequenceType(
646664
@CachedLibrary("this") PythonObjectLibrary lib,
647665
@Shared("hasGetItemNode") @Cached LookupAttributeInMRONode.Dynamic hasGetItemNode,
648-
@Shared("hasLenNode") @Cached LookupAttributeInMRONode.Dynamic hasLenNode,
666+
@Exclusive @Cached LookupAttributeInMRONode.Dynamic hasLenNode,
649667
@Shared("isLazyClass") @Cached("createBinaryProfile()") ConditionProfile isLazyClass,
650-
@Shared("lenProfile") @Cached("createBinaryProfile()") ConditionProfile lenProfile,
668+
@Exclusive @Cached("createBinaryProfile()") ConditionProfile lenProfile,
651669
@Shared("getItemProfile") @Cached("createBinaryProfile()") ConditionProfile getItemProfile) {
652670
if (isLazyClass.profile(lib.isLazyPythonClass(this))) {
653671
if (lenProfile.profile(hasLenNode.execute(this, __LEN__) != PNone.NO_VALUE)) {
@@ -661,18 +679,10 @@ public boolean isSequenceType(
661679
public boolean isMappingType(
662680
@CachedLibrary("this") PythonObjectLibrary lib,
663681
@Shared("hasGetItemNode") @Cached LookupAttributeInMRONode.Dynamic hasGetItemNode,
664-
@Shared("hasLenNode") @Cached LookupAttributeInMRONode.Dynamic hasLenNode,
665682
@Shared("isLazyClass") @Cached("createBinaryProfile()") ConditionProfile isLazyClass,
666-
@Shared("lenProfile") @Cached("createBinaryProfile()") ConditionProfile lenProfile,
667-
@Shared("getItemProfile") @Cached("createBinaryProfile()") ConditionProfile getItemProfile,
668-
@Exclusive @Cached LookupAttributeInMRONode.Dynamic hasKeysNode,
669-
@Exclusive @Cached LookupAttributeInMRONode.Dynamic hasItemsNode,
670-
@Exclusive @Cached LookupAttributeInMRONode.Dynamic hasValuesNode,
671-
@Exclusive @Cached("createBinaryProfile()") ConditionProfile profile) {
672-
if (isSequenceType(lib, hasGetItemNode, hasLenNode, isLazyClass, lenProfile, getItemProfile)) {
673-
return profile.profile(hasKeysNode.execute(this, SpecialMethodNames.KEYS) != PNone.NO_VALUE &&
674-
hasItemsNode.execute(this, SpecialMethodNames.ITEMS) != PNone.NO_VALUE &&
675-
hasValuesNode.execute(this, SpecialMethodNames.VALUES) != PNone.NO_VALUE);
683+
@Shared("getItemProfile") @Cached("createBinaryProfile()") ConditionProfile getItemProfile) {
684+
if (isLazyClass.profile(lib.isLazyPythonClass(this))) {
685+
return getItemProfile.profile(hasGetItemNode.execute(this, __GETITEM__) != PNone.NO_VALUE);
676686
}
677687
return false;
678688
}
@@ -1505,7 +1515,7 @@ int access(Object object, String fieldName,
15051515
info |= REMOVABLE;
15061516
info |= MODIFIABLE;
15071517
}
1508-
} else if (!isImmutable.execute(object) || dataModelLibrary.isMapping(object)) {
1518+
} else if (!isImmutable.execute(object) || isAbstractMapping(object, dataModelLibrary)) {
15091519
// If the member does not exist yet, it is insertable if this object is mutable,
15101520
// i.e., it's not a builtin object or it is a mapping.
15111521
info |= INSERTABLE;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/object/PythonObjectLibrary.java

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,10 @@
6262
import com.oracle.truffle.api.interop.InteropLibrary;
6363
import com.oracle.truffle.api.interop.UnsupportedMessageException;
6464
import com.oracle.truffle.api.library.GenerateLibrary;
65-
import com.oracle.truffle.api.library.GenerateLibrary.Abstract;
66-
import com.oracle.truffle.api.library.GenerateLibrary.DefaultExport;
6765
import com.oracle.truffle.api.library.Library;
6866
import com.oracle.truffle.api.library.LibraryFactory;
67+
import com.oracle.truffle.api.library.GenerateLibrary.Abstract;
68+
import com.oracle.truffle.api.library.GenerateLibrary.DefaultExport;
6969
import com.oracle.truffle.api.nodes.Node;
7070
import com.oracle.truffle.api.nodes.NodeCost;
7171
import com.oracle.truffle.api.profiles.ConditionProfile;
@@ -808,10 +808,10 @@ public int lengthWithFrame(Object receiver, ConditionProfile hasFrameProfile, Vi
808808
}
809809

810810
/**
811-
* Checks whether the receiver is a Python mapping. As described in the
812-
* <a href="https://docs.python.org/3/reference/datamodel.html">Python Data Model</a> and
813-
* <a href="https://docs.python.org/3/library/collections.abc.html">Abstract Base Classes for
814-
* Containers</a>
811+
* Checks whether the receiver is a Python mapping. This message is supposed to be an equivalent
812+
* of CPython's {@code PyCheck_Mapping}. Note that such object does not have to conform to the
813+
* definition of mapping as described in
814+
* <a href="https://docs.python.org/3/reference/datamodel.html">Python Data Model</a>.
815815
*
816816
* <br>
817817
* See {@link #isMappingType(Object)}
@@ -846,21 +846,14 @@ public boolean isSequenceType(Object receiver) {
846846
}
847847

848848
/**
849-
* Checks whether the receiver is a Python mapping type. As described in the
850-
* <a href="https://docs.python.org/3/reference/datamodel.html">Python Data Model</a> and
851-
* <a href="https://docs.python.org/3/library/collections.abc.html">Abstract Base Classes for
852-
* Containers</a>
849+
* Checks whether the receiver is a Python mapping. This message is supposed to be an equivalent
850+
* of CPython's {@code PyCheck_Mapping}. Note that such object does not have to conform to the
851+
* definition of mapping as described in
852+
* <a href="https://docs.python.org/3/reference/datamodel.html">Python Data Model</a>.
853853
*
854854
* <br>
855-
* Specifically the default implementation checks whether the receiver
856-
* {@link #isSequenceType(Object)} and for the implementation of the following special methods:
857-
* <b>
858-
* <ul>
859-
* <li>keys</li>
860-
* <li>items</li>
861-
* <li>values</li>
862-
* </ul>
863-
* </b>
855+
* Specifically the default implementation checks whether the receiver has the {@code __items__}
856+
* special method.
864857
*
865858
* @param receiver the receiver Object
866859
* @return True if a mapping type

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/formatting/FormatProcessor.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
*/
77
package com.oracle.graal.python.runtime.formatting;
88

9-
import static com.oracle.graal.python.nodes.SpecialMethodNames.__GETITEM__;
109
import static com.oracle.graal.python.nodes.SpecialMethodNames.__INDEX__;
1110
import static com.oracle.graal.python.nodes.SpecialMethodNames.__INT__;
1211
import static com.oracle.graal.python.runtime.exception.PythonErrorType.MemoryError;
@@ -18,7 +17,6 @@
1817
import java.math.MathContext;
1918

2019
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
21-
import com.oracle.graal.python.builtins.objects.PNone;
2220
import com.oracle.graal.python.builtins.objects.PythonAbstractObject;
2321
import com.oracle.graal.python.builtins.objects.floats.PFloat;
2422
import com.oracle.graal.python.builtins.objects.function.PKeyword;
@@ -232,8 +230,8 @@ protected boolean isString(Object args1, Object lazyClass) {
232230
return PGuards.isString(args1) || isSubtype(lazyClass, PythonBuiltinClassType.PString);
233231
}
234232

235-
protected boolean isMapping(Object args1) {
236-
return lookupAttribute(args1, __GETITEM__) != PNone.NO_VALUE;
233+
protected static boolean isMapping(Object args1) {
234+
return PythonObjectLibrary.getUncached().isMapping(args1);
237235
}
238236

239237
protected static boolean isSubtype(Object lazyClass, PythonBuiltinClassType clazz) {

0 commit comments

Comments
 (0)