Skip to content

[Implement] ReturnType #735

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

Closed
wants to merge 4 commits into from
Closed

Conversation

jtenner
Copy link
Contributor

@jtenner jtenner commented Jul 29, 2019

fixes #728

Questions:

  1. How can we test this better?
  2. Any way to optimize this? (I copied from valueof<T> and adapted it)
  3. Can we make the error code better?

@dcodeIO
Copy link
Member

dcodeIO commented Jul 29, 2019

Makes me wonder if we should rename

  • native<T> to NativeType<T>
  • indexof<T> to KeyType<T>
  • valueof<T> to ValueType<T>

and maybe introduce

  • ParameterCount<T>() ?!
  • ParameterType<T,index>() ?!
  • ParameterName<T>(index: i32) ?!
  • ThisType<T>, maybe

As for the error, perhaps

TS2757: Type '{0}' has no call signatures.

@jtenner
Copy link
Contributor Author

jtenner commented Jul 30, 2019

Yeah. I can work on those separately. They are all within my ability.

Are there any other improvements I could make on this feature beside the error description?

@jtenner
Copy link
Contributor Author

jtenner commented Jul 30, 2019

Also if we implement parameter count, won't that make emit() safer and faster with compile time constants?

src/resolver.ts Outdated
if (!signatureReference) {
if (reportMode == ReportMode.REPORT) {
this.error(
DiagnosticCode.Index_signature_is_missing_in_type_0,
Copy link
Member

Choose a reason for hiding this comment

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

Index signature is for something[xy] overloads. Might be that the necessary error code is not in diagnosticMessages.json yet. If so, the process is to add it there and run scripts/build-diagnostics to update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. What should the first error be? The same thing?

I've submitted a commit that has both errors exactly the same.

src/resolver.ts Outdated
@@ -565,7 +565,7 @@ export class Resolver extends DiagnosticEmitter {
if (!(typeArgumentNodes && typeArgumentNodes.length == 1)) {
if (reportMode == ReportMode.REPORT) {
this.error(
DiagnosticCode.Expected_0_type_arguments_but_got_1,
DiagnosticCode.Type_0_has_no_call_signatures,
Copy link
Member

Choose a reason for hiding this comment

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

Previous error message was correct in that it reports missing type arguments on the builtin itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Switched back.

@jtenner
Copy link
Contributor Author

jtenner commented Jul 30, 2019

Okay. I know I've submitted a lot of pull requests recently. I'm very flexible and can work on each one individually until everything is perfect for each one. Can you help me clean them off one by one?

@dcodeIO
Copy link
Member

dcodeIO commented Jul 30, 2019

Of course, these are really useful PRs. The one thing I'm unsure about, as mentioned, is whether we should rename existing type-builtins to match the UpperCase naming scheme used by TS. In particular, type-builtins might use the uppercase TS scheme, while code-builtins (like parameterCount) would use the lowercase Wasm scheme, like so:

Type builtins

  • NativeType<T> (was native)
  • KeyType<T> (was indexof)
  • ValueType<T> (was valueof)
  • ReturnType<T>
  • ParameterType<T,index> (with index being an integer literal, might need special code)
  • ThisType<T>

(are there any other special types TS provides, like ReturnType, that we might want to implement? perhaps: InstanceType, NonNullable?)

Code builtins

  • parameterCount<T>
  • parameterName<T,index>(orIndex?: i32) (either index argument)

With all the renaming going on, it might make sense to join all of this to a single breaking PR instead of breaking just some stuff while waiting for additional PRs. Alternatively, this PR could do the necessary renaming together with ReturnType and we merge it first, before adding other new builtins. Or we do just the renaming first, and then continue with ReturnType etc. What would you prefer?

@jtenner
Copy link
Contributor Author

jtenner commented Jul 30, 2019

I don't think we need to break anything at all. We can simply add some aliases for each type that was mis-named and support both. For instance, it would be nice not to have to break the entire @as-pect/core library on a whim because you felt like the name was wrong.

As for combining all these pull requests into a single request, if you prefer it that way, let's go ahead and close these in favor of a single request with a bunch of builtins, types, and aliases.

Here are the ones I propose we get out of the way first:

  • ReturnType<T>
  • NativeType<T>
  • KeyType<T> added as an alias instead
  • ValueType<T> added as an alias instead
  • parameterCount<T>() definitely useful

Some of the ones we should probably think about in seperate prs with some guidance:

  • parameterName(): I'm not this is useful. Got an example?
  • ThisType: cannot be used meaningfully because prototype functions can't be used as expressions. We might also need typeof keyword support to make this worth while.
  • ParameterType<T, index> as a type with a numeric literal should be possible but would require a lot more effort, but would be very useful

@jtenner
Copy link
Contributor Author

jtenner commented Jul 30, 2019

closed in favor of #739

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.

[help] Get return types?
2 participants