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

feat: Constructor generic type inference #2894

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mattjohnsonpint
Copy link
Contributor

Fixes #2893.

Changes proposed in this pull request:

Enables type inference for constructors of generic classes by:

  • Refactoring to extract the existing type inference logic for functions that was in maybeInferCall, into its own private method inferGenericTypeArguments.

  • Calling that new method from resolveClassInclTypeArguments when a generic class's constructor is called without any type arguments.

  • Expanding the unit tests to verify working functionality and no regressions.

  • I've read the contributing guidelines

  • I've added my name and email to the NOTICE file

@mattjohnsonpint mattjohnsonpint changed the title Type inference feat: Constructor generic type inference Jan 12, 2025
@mattjohnsonpint mattjohnsonpint marked this pull request as draft January 12, 2025 06:15
@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review January 12, 2025 06:45
@mattjohnsonpint
Copy link
Contributor Author

Note - I didn't change any of the actual existing logic for type inference that was used for functions. I just extracted it for re-use by constructor "new expressions". The PR is best reviewed with the "hide whitespace" box checked.

@CountBleck
Copy link
Member

CountBleck commented Jan 12, 2025

@mattjohnsonpint Thanks, that diff looks so much more manageable to review now.

@CountBleck
Copy link
Member

Note: it doesn't handle this case:

class Foo<T> {
    constructor(public x: T) {}
}

class Bar<T> extends Foo<T> {}

const x = new Bar("hi")

That can probably be figured out at a later date.

Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, probably.

src/resolver.ts Outdated Show resolved Hide resolved
@mattjohnsonpint
Copy link
Contributor Author

Note: it doesn't handle this case:

class Foo<T> {
    constructor(public x: T) {}
}

class Bar<T> extends Foo<T> {}

const x = new Bar("hi")

That can probably be figured out at a later date.

I might be able to address that. I'll take a look tomorrow. Good catch!

@mattjohnsonpint
Copy link
Contributor Author

It should handle that case now. Thanks!

@mattjohnsonpint
Copy link
Contributor Author

Also, FWIW, I don't think either of the previously existing test cases marked TODO are valid. They aren't in regular TypeScript anyway, so I don't think they should be in AssemblyScript.

Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good.

@CountBleck
Copy link
Member

They aren't in regular TypeScript anyway, so I don't think they should be in AssemblyScript.

I don't know what those are for either. I'm guessing it's something related to omitting : T and inferring the return type (I think a is a placeholder for some value dependent on the generic T).

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.

Constructor type inference
2 participants