Conversation
99db145 to
67a0989
Compare
That's a good enough reason for me 🫡 |
Thinking about it a bit more this is no different than what is already true today. Changing |
src/awst_build/ptypes/index.ts
Outdated
| readonly properties: Record<string, PType> | ||
| readonly singleton = false | ||
| readonly immutable: boolean | ||
| readonly abiSafe: boolean |
There was a problem hiding this comment.
I am unsure about the purpose of this property even now, and I think it will be less clear without the context of this PR in the future so in the very least this needs a comment explaining its purpose.
I question why it needs to be here at all though, as long as we come up with a deterministic way of intersecting the properties I don't see why the intersection types would be any more or less stable than their parts - and if that's the case there's no reason not to allow them as ABI params
There was a problem hiding this comment.
If we want something where A & B is the same as B & A we can sort the property names before building the object type. Maybe move all boolean properties at the start/end so they can benefit from ARC4 packing.
Buuut... since that means we have to devise that mechanism (Am I missing something? should we pack other kinds of properties?) I feel it makes sense to forbid us from storing persisting intersections until that's sorted out.
Then we have to choose between:
- Merging this and starting the design for persistable intersections
- Delaying this until that's designed for and we all feel ok with the results
After thinking about it a bit more "abi safe" is not a good name. I would like to change it to "runtime only" (and negate the value): A "runtime only" type is one whose values can be created during the execution of the program but should not be persisted internally nor user-visible externally.
There was a problem hiding this comment.
I think it's less important to make A & B equal B & A and more important to ensure that A & B is always the same ABI type. Much in the same way that { a: uint64, b: uint8 } is not equal to { b: uint8, a: uint64 } but both of them give you determinstic ABI type resolution. I think this is true even in the case of overlaps. As long as the same type always resolves the same way it is no different than how we treat POJOs
There was a problem hiding this comment.
@joe-p I strongly disagree there. tsc is free to transform the types in any semantically equivalent way. Field order is not part of these semantics.
Stuff like ((A & B) & (B & C)) may be represented as A & B & C instead (I've observed this). AFAIK there's no well-defined contract here.
This is why I'm extremely careful with where intersections should be supported: when the semantic analysis phase is free to reorder the intersection parts the comparison with written tuple types (where we assume field order is declaration of intent) falls flat.
There was a problem hiding this comment.
Right I see what you are saying. Right now we do rely on deterministic field ordering for type resolution, but adding support for intersection types would expose us to a larger surface area of things that may change and break our assumptions.
There was a problem hiding this comment.
I'll go to the tsc playground to commit some type crimes so we can have better examples though.
I feel comfortable with having intersections up to this level of support because improving it from here is easy. The other way around feels a bit more risqué tbh.
There was a problem hiding this comment.
From src/compiler/checker.ts:18649
// We normalize combinations of intersection and union types based on the distributive property of the '&'
// operator. Specifically, because X & (A | B) is equivalent to X & A | X & B, we can transform intersection
// types with union type constituents into equivalent union types with intersection type constituents and
// effectively ensure that union types are always at the top level in type representations.
//
// We do not perform structural deduplication on intersection types. Intersection types are created only by the &
// type operator and we can't reduce those because we want to support recursive intersection types. For example,
// a type alias of the form "type List<T> = T & { next: List<T> }" cannot be reduced during its declaration.
// Also, unlike union types, the order of the constituent types is preserved in order that overload resolution
// for intersections of types with signatures can be deterministic.
function getIntersectionType(types: readonly Type[], flags = IntersectionFlags.None, aliasSymbol?: Symbol, aliasTypeArguments?: readonly Type[]): Type {
This means that the order of intersection parts should be stable. I'll take a look at the implementation.
…ize an intersection-typed tuple
…al/local/box/boxmap definitions
8eeffe5 to
d536ada
Compare
f0deadf to
ab049c7
Compare
| } | ||
| } | ||
|
|
||
| export class IntersectionPType extends TransientType { |
There was a problem hiding this comment.
So, currently this class is not actually used (prior to this PR). Or rather, it is created, but then thrown away immediately and the contents of the class don't matter.
When a class uses polytype multiple inheritance, certain members are resolved into a single intersection type member by the TypeScript type-checker. reflectContractType is the pathway here - when it calls resolveType on the getTypeOfSymbol result. However, the resulting ptype does not pass the subsequent instanceofAny/instanceof checks - so it gets throw away.
My next question was if they're thrown away, how do such methods get resolved? And the answer is in a work-around added in ContractThisBuilder.memberAccess. The result of tsType.getProperties() in reflectContractType should contain all members of the class, including inherited ones. But because the ones with intersection types due to use of polytype being thrown away, a second iteration of all bases has to occur here - if you remove the loop over this.ptype.allBases() in ContractThisBuilder.memberAccess and then run all tests you'll see what I mean.
So in summary, I think you can remove / replace this class entirely, and have proper handling of polytype members in reflectContractType instead.
This is my humble proposal for intersection types. It forbids their usage on any place that has to respect a stable ABI (logs/storage/ARC4 params/ARC4 return values).
The implementation is only for object-shaped intersections (intersections where the backing storage for each part is an object shape).
This PR adds the concept of an object type being "abi safe". This wording may be changed (serialization safe? runtime only?).
This is a first approximation to close #318. Still, the specific contract for that issue will not compile under this PR (since it uses an intersection type as a return value).
On alternative approaches
An alternative approach was suggested by @joe-p: Write an analysis pass that works over the AST and forbid only intersection types with overlapping fields. The core idea behind this suggestion is that overlapping fields are the main reason why ABI stability is difficult to reason about.
My personal stance on this alternative disagrees in two points:
tscredundant. At the same time it is a brittle endeavor: handling all possible ways intersection types may be defined will result in either "this is the syntax subset we support" or "keep in mind intersection types have some small differences with their typescript counterparts"B & AtoA & B). At the runtime level these changes are not visible, so restricting intersection types there seems like a nice tradeoff.Intersection types are already supported
Since we exploit the type analysis made by
tscwe do have ways to use intersection types.tsctypes are not their syntactic descriptions but rather an implementation-defined normal form. This means the following snippet does currently compile (onmain):This happens because
tscis free to simplifyMyAccountWithIdinto{ account: Account, id: Uint32 }. As far as I know there's no real reason (other than implementation constraints) that restrainstscfrom simplifyingIntersectionType.