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

Don't store @template constraint in a TypeParameterDeclaration node #26283

Merged
3 commits merged into from
Aug 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 7, 2018

A node needs to contain its children, but in /** @template {number} T */, {number} was made a child of T despite not being contained in its range. Changed this so that {number} would be a child of the @template node which spans all the way to the end of the line.

@@ -29763,6 +29776,13 @@ namespace ts {
}
return false;
}

function getEffectiveConstraintOfTypeParameter(node: TypeParameterDeclaration): TypeNode | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this function exposed as part of the public API to align with getEffectiveTypeParameterDeclarations?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Needs one more test plus I agree that the new accessor should be public.

@@ -759,6 +759,7 @@ namespace ts {
kind: SyntaxKind.TypeParameter;
parent: DeclarationWithTypeParameterChildren | InferTypeNode;
name: Identifier;
// Note: Consider calling `getEffectiveConstraintOfTypeParameter`
Copy link
Member

Choose a reason for hiding this comment

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

Hm. We should have these notes as jsdoc for lots of things now. Something for a separate PR though.

@@ -2363,6 +2364,7 @@ namespace ts {

export interface JSDocTemplateTag extends JSDocTag {
kind: SyntaxKind.JSDocTemplateTag;
constraint: TypeNode | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Only the first type parameter in a list is constrained by the constraint. Can you add a test for that? That is, /** @template {number} T,U */ only constrains T.

Copy link
Author

Choose a reason for hiding this comment

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

That was done in #24600 for the reason that reusing a node would be bad -- maybe not necessary now?

Copy link
Author

Choose a reason for hiding this comment

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

The test jsdocTemplateTag3, added in #24600, will have fewer errors if this is changed, so it's already being tested.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like applying the constraint to multiple parameters even if there's no implementation reason that it's easier.

@ghost ghost merged commit a73161e into master Aug 10, 2018
@ghost ghost deleted the editTemplateConstraint branch August 10, 2018 00:39
errendir added a commit to errendir/TypeScript that referenced this pull request Aug 11, 2018
* origin/master: (283 commits)
  Don't error on destructure of private property with computed property syntax (microsoft#26360)
  getDefaultExportInfo: Use `getImmediateAliasedSymbol` instead of `getAliasedSymbol` (microsoft#26364)
  review comments
  restore old algorithm
  Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit (microsoft#26341)
  Update user baselines (microsoft#26358)
  Don't store @template constraint in a TypeParameterDeclaration node (microsoft#26283)
  fixAddMissingMember: Support interface and don't crash on type parameter (microsoft#25995)
  Don't include class getter in spread type (microsoft#26287)
  Don't crash on computed property in destructure (microsoft#26334)
  Check the ambientness of a symbol name before attempting to trim it (microsoft#26312)
  Still generate signatures in SkipContextSensitive mode just to match on return types (microsoft#25937)
  fix handling if there is no commonPrefix
  Actually add sorting of elaboration text to user baselines
  Ping ryan instead of mohammed for user PRs now
  handle failed lookups
  make it work for root directory
  really, really fix test(?)
  add test
  fix commonPrefix handling
  ...
This pull request was closed.
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.

2 participants