powdr-labs / powdr

A modular stack for zkVMs, with a focus on productivity, security and performance.
Apache License 2.0
316 stars 55 forks source link

Add source location to expression #1293

Open chriseth opened 3 weeks ago

chriseth commented 3 weeks ago

For improved error messages, the Expression type should have a source reference.

There are two options to implement that: 1) Add it as a field to every expression variant and have a trait that can be used to retrieve the source ref. 2) Add an intermediate type such that Expression is a struct that contains a source ref and the current Expression type.

I'm slightly leaning towards option 1, but am also open for option 2.

In the future, we might want to have different expression types that might contain more or less variants, but I think this is compatible with both ways to do it.

We might also want to have a source ref for AlgebraicExpression. For that, it might be better to implement option 1, so that the handling is uniform using the trait.

gzanitti commented 2 weeks ago

I'm leaning towards option one as well. Even in that case, if Expression enumerates parsed expressions and the internal structs represent the expression itself, I would directly rewrite them as

pub enum Expression<Ref = NamespacedPolynomialReference> {
    Reference(Ref, SourceRef),
    PublicReference(PublicRefence, SourceRef),
    Number(Number, SourceRef),
    String(String_, SourceRef),
    Tuple(Tuple, SourceRef),
    LambdaExpression(LambdaExpression<Self>, SourceRef),
    ArrayLiteral(ArrayLiteral<Self>, SourceRef),
    BinaryOperation(BinaryOperation<Self>, SourceRef),
    UnaryOperation(UnaryOperator<Self>, SourceRef),
    IndexAccess(IndexAccess<Self>),
    FunctionCall(FunctionCall<Self>),
    FreeInput(FreeInput<Self>, SourceRef),
    MatchExpression(MatchExpression<Self>, SourceRef),
    IfExpression(IfExpression<Self>, SourceRef),
    BlockExpression(BlockExpression<Self>, SourceRef),
}

where every entry (especially Number, String, BinaryOperation, etc) becomes

    ExpressionEnumerated(ExpressionStruct, SourceRef)

Let's take advantage of the fact that we are going to add breaking changes in the open PRs of everyone in the team anyway :joy:

What do you think?