Add ability to type operation parameters & return values about ohm HOT 11 OPEN

@TheOnlyTails Sorry for the late reply — thanks very much for looking into this! I'd be happy to try to fix this for the next major release. Before committing to anything, I need to find time to think about this more deeply, and would like to get some other eyes on it too.

from ohm.

@TheOnlyTails This is already supported — please see the documentation on Using Ohm with TypeScript.

If there's something missing from that, or you find any problems, please let me know!

from ohm.

I read through the docs again, and I think you misunderstood what I meant: the type-checking when declaring semantic operations is great - the problem arises when it comes to accessing them - they're all typed as any. I'd like something like this:

grammar.createSemantics({
  // this object allows declaring semantic actions while type-checking their params and return type when called
  eval: (semanticParam: string) => ({
    Expression: (content) => //etc
  })
})

from ohm.

Sorry for the confusion, guess I was a bit distracted when I read this and didn't fully understand.

To confirm I understand what you're asking about, let's take this example:

const s = g.createSemantics().addOperation('myOp(a, b)`, {
  ...
})

You want to be able to specify both (1) the return type of the myOp operation, and (2) the type(s) of the parameters (a, b)?

It's already possible to specify the return type, see arithmetic.ts in the TypeScript example.

You're right that we're missing an ability to type the parameters — I'll repurpose this issue for that.

And thanks for the offer to help and for your suggested fix. I'll have to think about bit more this and how we can best solve it.

from ohm.

While parameters are one big missing thing about the types, I'm talking mostly about the typing of actions being called. For example:

semantics.addOperation<Expression>("eval()", {
  Expression_parens: (_, expr, _1) => expr.eval() // currently returns any, should return Expression
})

While this still compiles because of the any, it's still vulnerable to misspellings and confusing one operation with another.

My proposed solution would allow typescript to infer types of declared operations/attributes directly from their declarations, allowing for safer, less error-prone code.

from ohm.

Oh, I see! Yes, that's a very good point. I was missing the distinction between the return values of the semantic actions themselves (which ultimately get used as the result of the operation) and the result of the operation.

from ohm.

I've started working on this issue myself, diving head-first into the codebase, and so far these are my plans:
The most important goal is to avoid breaking the old way of defining semantic ops/attrs, so this will mostly make changes to generated types.

Here are some prototypes I've came up with:

// a utility types - gets rid of the [index: string]: any on the original Node type
type NoIndexNode = {
  [K in keyof Node as string extends K ? never : K]: Node[K];
};

// a custom node type that adds all of the attributes and operations to the original Node, given action dicts for each.
export type TestNode<Ops, Attrs> = NoIndexNode & {
  [op in keyof Ops]: Ops[op] extends (
    ...args: infer Args
  ) => TestActionDict<infer Ret>
    ? (...args: Args) => Ret
    : never;
} & {
  [attr in keyof Attrs]: Attrs[attr] extends TestActionDict<infer AttrType>
    ? AttrType
    : never;
};

export interface TestGrammar<
  Ops = { [index: string]: (...any) => AuraActionDict<any> },
  Attrs = { [index: string]: AuraActionDict<any> }
> extends Grammar<Ops, Attrs> {
  createSemantics(operations?: Ops, attributes?: Attrs): TestSemantics;
  extendSemantics(
    superSemantics: TestSemantics,
    operations?: Ops,
    attributes?: Attrs
  ): TestSemantics;
}

The main concept here is passing around the ops and attrs objects from createSemantics, then constructing a Node type from it and re-using it in the action dictionary.

To preserve back-compat, we could check if both the attrs and ops objects passed to createSemantics are undefined, and then use the regular Node to allow for the old system to still be used.

from ohm.

I've been working on and off on this for a while, and unfortunately, I think there's only 3 options for this:

1. Keeping things the way they are now, which is not ideal;
2. We could provide back-compat for the old system, but that would make the DX much worse for users of both the new and old systems;
3. Completely remove the old system, and only allow using the new one, which would be a major breaking change.

IMHO, if this is still something that's worth doing, only option no. 3 is worth it.
Trying to keep compatibility with the current way of doing things would be too much work to keep maintaining a flawed system that essentially opts-out of typechecking.

from ohm.