Refactoring: inheritance, unions, compiler stages

[acl-cqc]

Current State

1. The guppy codebase has quite a bit of inheritance (I'm just stating, not expressing an opinion for and against).
   a. Some of this is for abstract methods to be implemented by a range of subclasses; for example, def load_with_args. (I'm not clear whether there is any overriding of a superclass implementation.)
   b. In other cases it's so a dataclass can inherit a big bunch of fields - a typical example would be a class representing a checked thing (e.g. function def) inherits from the class for a parsed thing, then adds new information figured out by checking. This is attempting to inherit implementation (storage) but kind-of-accidentally also inheriting interface (potentially, unwanted methods).

2. As well as subclass/protocol polymorphism, many of the types of variables/function arguments/return values are union types - with lots of isinstance to handle all cases - specifically (these change in feat!: Monomorphise everything during type checking  #1441):

RawDef: TypeAlias = "ParsableDef | ParsedDef"
ParsedDef: TypeAlias = "CheckableDef | CheckedDef"
CheckedDef: TypeAlias = "CompilableDef | MonomorphizableDef | CompiledDef"

Not inherently opposed to having both virtual method dispatch for things with common interface and isinstance for things without, but I do wonder if isinstance combined with the inheritance of methods from earlier stages, might lead to accidents?

Proposal

Firstly I wonder if the union types would be more readable if they were instead common protocols like get_parsed or get_checked (where ParsedDef or CheckedDef would just return self)

More broadly we might want to think about defining:

• POD (plain old data) records with the common fields that we were inheriting above; these could easily be transferred between:
• wrapper classes for representing each kind of entity at each different stage (parsing, checking, etc.) - sharing PODs where helpful, adding new fields if necessary, but not inheriting methods or fields from earlier stages
• protocols, implemented by those wrapper classes, capturing commonality between different entities at each stage, e.g. a Parsed protocol might define a method check that returns something implementing a Checked protocol.
  • Maybe each subclass refines the return type to a concrete thing that is both Checked and the desired kind of entity, or maybe (more likely) there are multiple protocols at each stage.

Goals

Besides generally wanting to avoid "accidentally picking the wrong method", one goal would be to strengthen the separation between stages of parsing/checking/compiling. (I.e. by better separating the objects and interfaces.)

I think we will also want to separate phases in time: we'll need to finish checking (including comptime/tracing, an issue in itself...somewhere?) in order to build a callgraph and analyse that before can compile much to Hugr. So we might need to think about get_checked as well.

[tatiana-s]

Just checking I understand your proposal, so to roughly sketch out the idea with function definitions you'd have something like the below?

# PODs
@dataclass
class DefinitionRecord
	id: ...
	name: ...
	...

@dataclass
class FunctionRecord
	link_name: ...
	...

# Protocols
class Parsed(Protocol)
	def compile(self) -> Compiled:
	...

# Wrapper classes
@dataclass
class ParsedFunctionDef()
	definition_record: DefinitionRecord
	function_record: FunctionRecord 
	metadata: FunctionMetadata
	...

	def compile(self) -> Compiled:
	...

I think using protocols is a good idea, though passing around the records and accessing fields on them might become a bit unwieldy? I also like the idea of the different stages being a bit more separated, however I am not sure I see how exactly this proposal achieves that (besides a big refactor like this being a good time to do it).

[acl-cqc]

Codewise roughly that, I think FunctionRecord might inherit DefinitionRecord in order to have strictly more data in it, and we could declare class ParsedFunctionDef(Parsed) to check Protocol conformance.

I admit how this separates phases is a little vague. I think what I'm aiming at is more enforcement and clarity as to what happens, rather than changing what happens (as you say, that's a separate thing) - we make sure that Checked things don't implement e.g. Parsed so that objects can only belong to one stage.