tunnelvisionlabs / antlr4ts

Optimized TypeScript target for ANTLR 4
Other
624 stars 106 forks source link

Use optional method syntax for enter/exit/visit #531

Open TheUnlocked opened 2 years ago

TheUnlocked commented 2 years ago

What does it do?

Generate visitX?(ctx: XContext): Result syntax rather than visitX?: (ctx: XContext) => Result for generated listener and visitor interface members.

Why would we want this?

Usually, method-style implementations work fine on implementing classes even when the method is declared as a property in the interface.

// generated/LangVisitor.ts
...
visitFoo?: (ctx: FooContext) => Result;
...

// MyVisitor.ts
class MyVisitor implements LangVisitor<string> {
    // ok!
    visitFoo(ctx: FooContext): string {
        return 'foo';
    }
    ...
}

However, in some edge cases, it does not work so cleanly. Consider this construct in which AbstractParseTreeVisitor and LangVisitor are combined into AbstractLangVisitor (see https://github.com/microsoft/TypeScript/issues/22815#issuecomment-375766197 for context on the shenanigans going on here)

// MyVisitor.ts
abstract class AbstractLangVisitor<T> extends AbstractParseTreeVisitor<T> implements LangVisitor<T> {};
interface AbstractLangVisitor<T> extends LangVisitor<T> {};

class MyVisitor extends AbstractLangVisitor<string> {
    // error ts(2425): Class 'AbstractLangVisitor<string>' defines instance member property 'visitFoo',
    //                 but extended class 'MyVisitor' defines it as instance member function.
    visitFoo(ctx: FooContext): string {
        return 'foo';
    }
}

However, if we generate the member differently so that it looks like a method rather than like a property...

// generated/LangVisitor.ts
...
visitFoo?(ctx: FooContext): Result;
...

// MyVisitor.ts
abstract class AbstractLangVisitor<T> extends AbstractParseTreeVisitor<T> implements LangVisitor<T> {};
interface AbstractLangVisitor<T> extends LangVisitor<T> {};

class MyVisitor extends AbstractLangVisitor<string> {
    // ok! We can even use the new override annotation if we want!
    override visitFoo(ctx: FooContext): string {
        return 'foo';
    }
}

Importantly, this alternative form still works if you declare it as a property rather than a method.

class MyVisitor extends AbstractLangVisitor<string> {
    // ok!
    visitFoo = (ctx: FooContext): string => {
        return 'foo';
    };
}

One other small benefit...

This alternative form also gives better autocomplete when you ask the editor to generate a stub.

// generated/LangVisitor.ts
visitFoo?: (ctx: FooContext) => Result;

// Autocomplete "visitFoo" generates
visitFoo?: (ctx: FooContext) => string;
// generated/LangVisitor.ts
visitFoo?(ctx: FooContext): Result;

// Autocomplete "visitFoo" generates
visitFoo(ctx: FooContext): string {

}

Drawbacks

As far as I can tell this shouldn't break any existing code. However, I'm also not familiar with the precise semantics of matching members with interface definitions, so It's possible that there is some edge case where this fails that I haven't thought of.

Also, if someone actually prefers the old style of autocomplete, this could be slightly more annoying for them.