Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheUnlocked
Copy link

@TheUnlocked TheUnlocked commented Jun 4, 2022

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 microsoft/TypeScript#22815 (comment) 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.

* Generate `visitX?(ctx: XContext): Result` syntax rather than `visitX?: (ctx: XContext) => Result` for listener and visitor interface members.
* Also fix prepare script ("target/src" was being interpreted as a github repository by more recent versions of NPM)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant