AnyDSL / artic

The AlteRnaTive Impala Compiler
MIT License
25 stars 6 forks source link

Traits implementation #5

Closed madmann91 closed 3 years ago

madmann91 commented 3 years ago

This PR tracks the progress of the traits implementation.

s8starie commented 3 years ago

I think this can be made better still. I agree, but I am unsure how to implement it

Why not do as I suggested and introduce a PolyDecl class from which all classes inherit from? That also sounds more consistent with what you introduced in the type system.

There currently is no place where it fits nicely because of the NamedDecl class. Some NamedDecls are polytypes, some are not and some PolyTypes are NamedDecls while others are not. I would need two superclasses since the features are pretty much orthogonal :/

(correct me if I'm wrong, but AST nodes that accept where clauses always accept type parameters).

You are right and I thought about adding a separate construct to the classes that consists of the two lists. Something like:

struct Poly{
   Ptr<TypeParamList> params;
   Ptr<WhereClauseList> clauses
}
....
struct FnDecl : NamedDecl {
 ....
 struct Poly poly;
...
}

The problem with that is that this node can neither be parsed nor printed without context. Sometimes there is a name between the lists, sometimes a type and sometimes nothing.

I agree that they TypePArams and Where clauses belong together, but I (currently) do not see how to best implement that abstraction

EDIT: I just found out that CPP supports multiple inheritance. Since I never used it: is it a desirable way of implementing this superclass?

madmann91 commented 3 years ago

Why not just have PolyDecl inherit from NamedDecl? I think AST nodes that accept type parameters and where clauses all have a name. Then you don't even need multiple inheritance.

s8starie commented 3 years ago

All except ImplDecl, which is not a NamedDecl

madmann91 commented 3 years ago

I see. Well why not place where_clauses inside TypeParamList? After all, where clauses are always paired with an (optional) type parameter list.

s8starie commented 3 years ago

Because of parsing / printing. When printing the new "super-node" (type params + where clauses) one would need to do so knowing in which Decl one is. Example:

fn test[A]() where Add[A]  //param list between type params and clauses
struct S[A] where Add[A] //nothing between type params and clauses
impl[A] T[A] where Add[A] // type between type params and clauses

The composition of the two element is the same in the two cases, but it gets parsed and printed differently. This means that one would need to "unpack" it every time in order to use it. Also, when reaching the type checking phase one can use the types (which now have proper abstrctions).

I total I do not see any benefit to it (regarding the rest of the code), except that it is nice to pack them together. But maybe I am overlooking something

madmann91 commented 3 years ago

I total I do not see any benefit to it (regarding the rest of the code), except that it is nice to pack them together. But maybe I am overlooking something

Well to start with one obvious benefit is that there's no need to add a where_clause anywhere anymore except inside TypeParamsList. That's already a nice property.

Furthermore, those two things belong together since they are always together. That they are not parsed/printed together is irrelevant actually. Grouping should be about functionality, not about the order in which we print or parse things. Particularly since returning those together allows to clean more of the crust you added:

const  std::vector<const Type*> where_clauses() const override;
const ast::TypeParamList* type_params() const override {
    return decl.type_params.get();
}

can become just:

const ast::TypeParamList* type_params() const override { return decl.type_params.get(); }

Since now you can get the where clause types by doing type_params()->where_clauses.clauses[i]->type. You on top of this can avoid creating a temporary std::vector to hold the results.

s8starie commented 3 years ago

If we do that I think that we should also treat trait bounds on declarations without type parameters as parsing errors. Otherwise there are annoying cases in which one needs to have a typeparamlist with no parameters just to store the clauses somewhere. I also do not think that this is a big loss. Alternatively one could also introduce a separate structure that contains the typeparamlist and the trait bounds as fields.

madmann91 commented 3 years ago

The design you see there (using a Ptr<TypeParamList> instead of a PtrVector<Type> directly) is a remnant of an old implementation of artic where bounds were packed in the type parameter list [T, U, V where ...]. It should be clear to you that the fact that we place the where clause somewhere else is purely aesthetic (it was possible to write [where ... ] before, if you were wondering).

There are other nodes in which the AST structure is different from the way we parse/print things (take FnDecl/ForExpr for instance). The AST should be a representation that's simple to work with, not really a literal translation of the syntax. Here, and as pointed above, it makes the code simpler to combine type parameters and where clauses, so I think it's the most logical move. It's also easier to manage than inheritance (as you pointed out, not all declarations with type parameters have a name). In general, inheritance is a tricky business and I prefer using composition whenever possible (which is what I'm suggesting here).

Of course that means the name TypeParamList is a bit weird now, since the node also contains where clauses. That means the logical course of action is to change it. Choose a good name that means that this node has both type parameters and bounds, or either. I can give a few suggestions:

madmann91 commented 3 years ago

Closed in favor of #6.