Skip to content
This repository has been archived by the owner on Feb 23, 2023. It is now read-only.

numbers: Add proper safety to operator overloading #204

Merged
merged 2 commits into from
Sep 9, 2021

Conversation

evaporei
Copy link
Contributor

@evaporei evaporei commented Sep 8, 2021

AssemblyScript (0.19.10) has a bug that allows operator overloading functions to be called with nullable values only when they are class properties, like this:

class BigInt extends Uint8Array {
    @operator('+')
    plus(other: BigInt): BigInt {
        // ...
    }
}

class Wrapper {
    public constructor(
        public n: BigInt | null
    ) {}
}

let x = BigInt.fromI32(2);
let y: BigInt | null = null;

// x + y; // give compile time error about nullability

let wrapper = new Wrapper(y);

wrapper.n = wrapper.n + x; // doesn't give compile time errors as it should

To fix this, we must change the type signature of the operator overloading methods, like this:

@operator('+')
plus(other: BigInt | null): BigInt {
  // Do null checks
}

I would like to write a test for each of these operator overloading changes, however it would be very difficult to test, since I would have to mock the host-exports function for each of them (implemented in the Rust/graph-node side).

Related PR in graph-node: graphprotocol/graph-node#2780

Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that silently coercing null to 0 is the right thing to do - if these get called with a null it's probably some coding error, and 0 is probably not the value that was intended to be used in the calculation, leading to calculations that are silently wrong.

@evaporei
Copy link
Contributor Author

evaporei commented Sep 8, 2021

Ok @lutter , I'll throw an error instead! I thought about this following this path before, but then I thought it would be easier for subgraph developers to upgrade the AS version.

The only problem right now is that they can only know this at runtime. I'll open an issue in the AS compiler so that we can have this error at compile time as well (better feedback loop for subgraph devs 🙂 ).

Also, I'll document this in the Migration Guide since it's a known issue, at least they can read it there and try to fix before it happens at runtime.

@evaporei evaporei force-pushed the otavio/safe-operator-overloading branch 2 times, most recently from 928f986 to d17bd95 Compare September 9, 2021 00:10
@lutter
Copy link
Collaborator

lutter commented Sep 9, 2021

Yes, I think doing it this way is safer; and this should absolutely be a compile-time error. But at least, using asserts here, the current workaround is safe in terms of not doing math that might be wrong.

When operator overloading functions are called, then unfortunately
work when the left hand side of the operation is nullable (only inside
class properties), like this:

```ts
class BigInt extends Uint8Array {
    @operator('+')
    plus(other: BigInt): BigInt {
        // ...
    }
}

class Wrapper {
    public constructor(
        public n: BigInt | null
    ) {}
}

let x = BigInt.fromI32(2);
let y: BigInt | null = null;

// x + y; // give compile time error about nullability

let wrapper = new Wrapper(y);

wrapper.n = wrapper.n + x; // doesn't give compile time errors as it should
```

In our case it would break in graph-node's runtime side because these
operator implementations are written there in the host-exports.

These checks only exist while the compiler doesn't catch this itself.
@evaporei evaporei force-pushed the otavio/safe-operator-overloading branch from d17bd95 to 2c37b6b Compare September 9, 2021 10:48
@evaporei evaporei merged commit 6bff1ab into master Sep 9, 2021
@evaporei evaporei deleted the otavio/safe-operator-overloading branch September 9, 2021 11:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants