Skip to content

Commit

Permalink
Add warnings for serializers interfaces (chapel-lang#23381)
Browse files Browse the repository at this point in the history
This PR adds warnings if users implement methods with the following
names/arguments without adding an appropriate interface:
- serialize, with arguments "writer" and "serializer": interface
writeSerializable
- deserialize, with arguments "reader" and "deserializer": interface
readSerializable
- init, with arguments "reader" and "deserializer": interface
initDeserializable

The catch-all "serializable" interface can also be applied to quiet all
three kinds of warnings. The compiler currently does not do any further
kinds of checking on these interfaces.

Testing changes were almost entirely made via scripting by @DanilaFe 

[reviewed-by @DanilaFe , @jeremiah-corrado]
  • Loading branch information
benharsh authored Sep 14, 2023
2 parents 124e0fb + ce0d46e commit cf22b97
Show file tree
Hide file tree
Showing 156 changed files with 743 additions and 250 deletions.
12 changes: 12 additions & 0 deletions compiler/AST/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@

InterfaceSymbol* gHashable = nullptr;
InterfaceSymbol* gContextManager = nullptr;
InterfaceSymbol* gWriteSerializable = nullptr;
InterfaceSymbol* gReadDeserializable = nullptr;
InterfaceSymbol* gInitDeserializable = nullptr;
InterfaceSymbol* gSerializable = nullptr;

static Symbol* isInterfaceFormalSymbol(Symbol* sym) {
if (TypeSymbol* var = toTypeSymbol(sym))
Expand Down Expand Up @@ -74,6 +78,14 @@ DefExpr* InterfaceSymbol::buildDef(const char* name,
gHashable = isym;
} else if (gContextManager == nullptr && strcmp("contextManager", name) == 0) {
gContextManager = isym;
} else if (gWriteSerializable == nullptr && strcmp("writeSerializable", name) == 0) {
gWriteSerializable = isym;
} else if (gReadDeserializable == nullptr && strcmp("readDeserializable", name) == 0) {
gReadDeserializable = isym;
} else if (gInitDeserializable == nullptr && strcmp("initDeserializable", name) == 0) {
gInitDeserializable = isym;
} else if (gSerializable == nullptr && strcmp("serializable", name) == 0) {
gSerializable = isym;
}

for_alist(formal, formals->argList) {
Expand Down
4 changes: 4 additions & 0 deletions compiler/include/symbol.h
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,10 @@ class InterfaceSymbol final : public Symbol {

extern InterfaceSymbol* gHashable;
extern InterfaceSymbol* gContextManager;
extern InterfaceSymbol* gWriteSerializable;
extern InterfaceSymbol* gReadDeserializable;
extern InterfaceSymbol* gInitDeserializable;
extern InterfaceSymbol* gSerializable;

/************************************* | **************************************
* *
Expand Down
7 changes: 4 additions & 3 deletions compiler/passes/buildDefaultFunctions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1916,11 +1916,12 @@ static void buildDefaultReadWriteFunctions(AggregateType* ct) {
}

forv_Vec(FnSymbol, method, ct->methods) {
int n = method ? method->numFormals() : 0;
if (method != nullptr &&
method->isInitializer() &&
method->numFormals() == 4 &&
strcmp(method->getFormal(3)->name, "reader") == 0 &&
strcmp(method->getFormal(4)->name, "deserializer") == 0) {
n >= 4 &&
strcmp(method->getFormal(n-1)->name, "reader") == 0 &&
strcmp(method->getFormal(n)->name, "deserializer") == 0) {
readerInit = method;
break;
}
Expand Down
71 changes: 65 additions & 6 deletions compiler/resolution/functionResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11263,30 +11263,76 @@ static AggregateType* getBaseTypeForInterfaceWarnings(Type* ts) {
return toReturn;
}

static bool matchesSerializeShape(FnSymbol* fn,
const char* first, const char* second) {
// Initializer needs at least 4 : this, _mt, <generics>, reader, deserializer
int n = fn->numFormals();
if (fn->isInitializer()) {
if (n < 4) return false;
} else if (n != 4) {
// (de)serialize have only 4: this, _mt, <channel>, <(de)serializer>
return false;
}

bool ret = strcmp(fn->getFormal(n-1)->name, first) == 0 &&
strcmp(fn->getFormal(n)->name, second) == 0;
return ret;
}

static bool isSerdeSingleInterface(InterfaceSymbol* isym) {
return isym == gWriteSerializable ||
isym == gReadDeserializable ||
isym == gInitDeserializable;
}

static void checkSpeciallyNamedMethods() {
static const std::unordered_map<const char*, InterfaceSymbol*> reservedNames = {
{ astr("hash"), gHashable },
{ astr("enterContext"), gContextManager },
{ astr("exitContext"), gContextManager },
{ astr("exitContext"), gContextManager },
{ astr("serialize"), gWriteSerializable },
{ astr("deserialize"), gReadDeserializable },
{ astr("init"), gInitDeserializable },
};

SpecialMethodMap flagged;

for_alive_in_Vec(FnSymbol, fn, gFnSymbols) {
if (!fn->isMethod()) continue;
if (fn->isCompilerGenerated() || fn->hasFlag(FLAG_FIELD_ACCESSOR)) continue;
// The parent class must have this function, and would've been flagged.
// the user will already get the warning.
if (fn->hasFlag(FLAG_OVERRIDE)) continue;
auto reservedIter = reservedNames.find(fn->name);
if (reservedIter == reservedNames.end()) continue;
auto found = reservedIter->second;

if (found == gHashable || found == gContextManager) {
// The parent class must have this function, and would've been flagged.
// the user will already get the warning.
if (fn->hasFlag(FLAG_OVERRIDE)) continue;
}

auto receiverType = fn->getReceiverType();
auto at = getBaseTypeForInterfaceWarnings(receiverType);
if (!at || (reservedIter->second == gHashable && at == dtObject)) continue;
if (!at || (found == gHashable && at == dtObject)) continue;

auto key = SpeciallyNamedMethodKey(reservedIter->second, at);
// _iteratorRecord currently doesn't work with 'implements' statements at
// the moment (likely due to its typeclass-like nature), so we skip all
// iterator records manually here for the 'writeSerializable' case.
if (at->symbol->hasFlag(FLAG_ITERATOR_RECORD) &&
found == gWriteSerializable) continue;

if ((found == gInitDeserializable || found == gReadDeserializable) &&
!matchesSerializeShape(fn, "reader", "deserializer")) continue;

if (found == gWriteSerializable &&
!matchesSerializeShape(fn, "writer", "serializer")) continue;

auto key = SpeciallyNamedMethodKey(found, at);
flagged[key].speciallyNamedMethods[fn->name] = fn;

if (isSerdeSingleInterface(found)) {
auto key = SpeciallyNamedMethodKey(gSerializable, at);
flagged[key].speciallyNamedMethods[fn->name] = fn;
}
}

for_alive_in_Vec(ImplementsStmt, istm, gImplementsStmts) {
Expand All @@ -11299,6 +11345,14 @@ static void checkSpeciallyNamedMethods() {
// flagged set, because we found an instance.
auto isym = istm->iConstraint->ifcSymbol();
flagged.erase(SpeciallyNamedMethodKey(isym, at));

if (isym == gSerializable) {
flagged.erase(SpeciallyNamedMethodKey(gWriteSerializable, at));
flagged.erase(SpeciallyNamedMethodKey(gReadDeserializable, at));
flagged.erase(SpeciallyNamedMethodKey(gInitDeserializable, at));
} else if (isSerdeSingleInterface(isym)) {
flagged.erase(SpeciallyNamedMethodKey(gSerializable, at));
}
}

// the things now left in flagged, we did not find any implements statements
Expand All @@ -11307,6 +11361,11 @@ static void checkSpeciallyNamedMethods() {
auto ifc = flaggedTypeIfc.first.first;
auto at = flaggedTypeIfc.first.second;

// Don't recommend 'serializable' so that there are not duplicates
if (ifc == gSerializable) {
continue;
}

USR_WARN(at,
"the type '%s' defines methods that previously had special meaning. "
"These will soon require '%s' to implement the '%s' interface to "
Expand Down
1 change: 1 addition & 0 deletions doc/rst/language/spec/iterators.rst
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ typically made by iterating over it in a loop.
.. BLOCK-test-chapelnoprint
Tree implements writeSerializable;
override proc Tree.serialize(writer, ref serializer)
{
var first = true;
Expand Down
8 changes: 4 additions & 4 deletions modules/dists/BlockCycDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ executes each iteration on the locale where that iteration's index
is mapped to.
*/
class BlockCyclic : BaseDist {
class BlockCyclic : BaseDist, writeSerializable {
param rank: int;
type idxType = int;

Expand Down Expand Up @@ -447,7 +447,7 @@ proc BlockCyclic.dsiPrivatize(privatizeData) {
////////////////////////////////////////////////////////////////////////////////
// BlockCyclic Local Distribution Class
//
class LocBlockCyclic {
class LocBlockCyclic : writeSerializable {
param rank: int;
type idxType;

Expand Down Expand Up @@ -747,7 +747,7 @@ proc BlockCyclicDom.dsiReprivatize(other, reprivatizeData) {
////////////////////////////////////////////////////////////////////////////////
// BlockCyclic Local Domain Class
//
class LocBlockCyclicDom {
class LocBlockCyclicDom : writeSerializable {
param rank: int;
type idxType;
param strides: strideKind;
Expand Down Expand Up @@ -1111,7 +1111,7 @@ iter BlockCyclicDom.dsiLocalSubdomains(loc: locale) {
////////////////////////////////////////////////////////////////////////////////
// BlockCyclic Local Array Class
//
class LocBlockCyclicArr {
class LocBlockCyclicArr : writeSerializable {
type eltType;
param rank: int;
type idxType;
Expand Down
8 changes: 4 additions & 4 deletions modules/dists/BlockDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ and array:
*/

pragma "ignore noinit"
record blockDist {
record blockDist : writeSerializable {
param rank: int;
type idxType = int;
type sparseLayoutType = unmanaged DefaultDist;
Expand Down Expand Up @@ -492,7 +492,7 @@ type Block = blockDist;


@chpldoc.nodoc
class BlockImpl : BaseDist {
class BlockImpl : BaseDist, writeSerializable {
param rank: int;
type idxType = int;
var boundingBox: domain(rank, idxType);
Expand Down Expand Up @@ -582,7 +582,7 @@ class BlockArr: BaseRectangularArr(?) {
// locDom: reference to local domain class
// myElems: a non-distributed array of local elements
//
class LocBlockArr {
class LocBlockArr : writeSerializable {
type eltType;
param rank: int;
type idxType;
Expand Down Expand Up @@ -2147,7 +2147,7 @@ config param debugBlockScan = false;
* suitable for general use since there are races with when the value gets
* written to, but safe for single writer, single reader case here.
*/
class BoxedSync {
class BoxedSync : writeSerializable {
type T;
var s: sync int; // int over bool to enable native qthread sync
var res: T;
Expand Down
6 changes: 3 additions & 3 deletions modules/dists/CyclicDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ This distribution has not been tuned for performance.
*/

pragma "ignore noinit"
record cyclicDist {
record cyclicDist : writeSerializable {
param rank: int;
type idxType = int;

Expand Down Expand Up @@ -351,7 +351,7 @@ operator =(ref a: cyclicDist(?), b: cyclicDist(?)) {
type Cyclic = cyclicDist;

@chpldoc.nodoc
class CyclicImpl: BaseDist {
class CyclicImpl: BaseDist, writeSerializable {
param rank: int;
type idxType = int;

Expand Down Expand Up @@ -1231,7 +1231,7 @@ proc CyclicArr.setRADOpt(val=true) {
if doRADOpt then setupRADOpt();
}

class LocCyclicArr {
class LocCyclicArr : writeSerializable {
type eltType;
param rank: int;
type idxType;
Expand Down
2 changes: 1 addition & 1 deletion modules/dists/DSIUtil.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ proc bulkCommConvertCoordinate(ind, bView:domain, aView:domain)
return result;
}

record chpl_PrivatizedDistHelper {
record chpl_PrivatizedDistHelper : writeSerializable {
// type instanceType;
var _pid:int; // only used when privatized
pragma "owned"
Expand Down
2 changes: 1 addition & 1 deletion modules/dists/DimensionalDist2D.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ class DimensionalArr : BaseRectangularArr(?) {
unmanaged LocDimensionalArr(eltType, allocDom.locDdescType);
}

class LocDimensionalArr {
class LocDimensionalArr : writeSerializable {
type eltType;
const locDom; // a LocDimensionalDom
pragma "local field" pragma "unsafe"
Expand Down
6 changes: 3 additions & 3 deletions modules/dists/HashedDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ The `Hashed` domain map initializer is defined as follows:
targetLocales: [] locale = Locales)
*/
class Hashed : BaseDist {
class Hashed : BaseDist, writeSerializable {

// GENERICS:

Expand Down Expand Up @@ -572,7 +572,7 @@ class UserMapAssocDom: BaseAssociativeDom {
//
// the local domain class
//
class LocUserMapAssocDom {
class LocUserMapAssocDom : writeSerializable {

// GENERICS:

Expand Down Expand Up @@ -964,7 +964,7 @@ class UserMapAssocArr: AbsBaseArr(?) {
//
// the local array class
//
class LocUserMapAssocArr {
class LocUserMapAssocArr : writeSerializable {

// GENERICS:

Expand Down
2 changes: 1 addition & 1 deletion modules/dists/PrivateDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ do not provide some standard domain/array functionality.
This distribution may perform unnecessary communication
between locales.
*/
class Private: BaseDist {
class Private: BaseDist, writeSerializable {
override proc dsiNewRectangularDom(param rank: int, type idxType,
param strides: strideKind, inds) {
for i in inds do
Expand Down
4 changes: 2 additions & 2 deletions modules/dists/ReplicatedDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ when the initializer encounters an error.


pragma "ignore noinit"
record replicatedDist {
record replicatedDist : writeSerializable {
forwarding const chpl_distHelp: chpl_PrivatizedDistHelper(unmanaged ReplicatedImpl);

proc init(targetLocales: [] locale = Locales,
Expand Down Expand Up @@ -514,7 +514,7 @@ proc _array.replicand(loc: locale) ref {
//
// local array class
//
class LocReplicatedArr {
class LocReplicatedArr : writeSerializable {
// these generic fields let us give types to the other fields easily
type eltType;
param rank: int;
Expand Down
2 changes: 1 addition & 1 deletion modules/dists/SparseBlockDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ class SparseBlockArr: BaseSparseArr(?) {
// locDom: reference to local domain class
// myElems: a non-distributed array of local elements
//
class LocSparseBlockArr {
class LocSparseBlockArr : writeSerializable {
type eltType;
param rank: int;
type idxType;
Expand Down
6 changes: 3 additions & 3 deletions modules/dists/StencilDist.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ config param disableStencilLazyRAD = defaultDisableLazyRADOpt;
*/
pragma "ignore noinit"
record stencilDist {
record stencilDist : writeSerializable {
param rank: int;
type idxType = int;
param ignoreFluff = false;
Expand Down Expand Up @@ -436,7 +436,7 @@ operator =(ref a: stencilDist(?), b: stencilDist(?)) {
type Stencil = stencilDist;


class StencilImpl : BaseDist {
class StencilImpl : BaseDist, writeSerializable {
param rank: int;
type idxType = int;
param ignoreFluff: bool;
Expand Down Expand Up @@ -540,7 +540,7 @@ class StencilArr: BaseRectangularArr(?) {
// locDom: reference to local domain class
// myElems: a non-distributed array of local elements
//
class LocStencilArr {
class LocStencilArr : writeSerializable {
type eltType;
param rank: int;
type idxType;
Expand Down
4 changes: 2 additions & 2 deletions modules/internal/Atomics.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ module Atomics {

pragma "atomic type"
pragma "ignore noinit"
record AtomicBool {
record AtomicBool : writeSerializable {
// Support `valType` on atomic bool type and instances for symmetry with
// numeric atomics
@chpldoc.nodoc
Expand Down Expand Up @@ -431,7 +431,7 @@ module Atomics {

pragma "atomic type"
pragma "ignore noinit"
record AtomicT {
record AtomicT : writeSerializable {
@chpldoc.nodoc
type valType;

Expand Down
Loading

0 comments on commit cf22b97

Please sign in to comment.