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

Fix 'as const'-like behavior in JSDoc type cast #45464

Merged
merged 1 commit into from Aug 28, 2021
Merged

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Aug 16, 2021

We almost support as const like casts in JavaScript files, but end up reporting errors because we either pick up the /** @type {} */ from the parenthesized expression when looking for effective type nodes, or by not handling it at all for contextual types. This fixes both scenarios.

Fixes #45463
Fixes #30445

@@ -25858,7 +25858,9 @@ namespace ts {
case SyntaxKind.ParenthesizedExpression: {
// Like in `checkParenthesizedExpression`, an `/** @type {xyz} */` comment before a parenthesized expression acts as a type cast.
const tag = isInJSFile(parent) ? getJSDocTypeTag(parent) : undefined;
return tag ? getTypeFromTypeNode(tag.typeExpression.type) : getContextualType(parent as ParenthesizedExpression, contextFlags);
return !tag ? getContextualType(parent as ParenthesizedExpression, contextFlags) :
Copy link
Member Author

Choose a reason for hiding this comment

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

I inverted the order here so that we have one condition per branch (i.e., !a ? x : b ? y : z) instead of nested conditions (i.e., a ? b ? y : x : z).

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Conceptually this looks OK, except for the odd baseline changes. But I could swear we discussed weather we should support const casts in JS back when we first added const casts, and the original answer was "no" (meaning the fix would have been properly parsing const as a type reference/erroring on the keyword if we wanted to stick with that).

tests/baselines/reference/importTypeInJSDoc.types Outdated Show resolved Hide resolved
tests/baselines/reference/jsdocTypeTagCast.types Outdated Show resolved Hide resolved
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.

Couple of things in the tests need to be changed, plus I didn't understand one branch of filterOwnedJSDocTags

src/compiler/utilities.ts Show resolved Hide resolved
tests/baselines/reference/importTypeInJSDoc.types Outdated Show resolved Hide resolved
tests/baselines/reference/jsdocTypeTagCast.types Outdated Show resolved Hide resolved
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Love it, thanks - I'll get this added to the JSDoc reference when it's in 👍🏻

@orta
Copy link
Contributor

orta commented Aug 24, 2021

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2021

Heya @orta, I've started to run the tarball bundle task on this PR at 6504a34. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2021

Hey @orta, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/109187/artifacts?artifactName=tgz&fileId=040AC2E0D39470A1CBEB58F9F08DDA3348CDB0ACA6F228B610CAEC0A4F6C001802&fileName=/typescript-4.5.0-insiders.20210824.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.5.0-pr-45464-2".;

@orta
Copy link
Contributor

orta commented Aug 24, 2021

As this is definitely going to get asked in the future, is it possible for both of these to work? e.g. sans-parens

const isConst = /** @type {const} */ ({
    hello: "world, not string"
})

isConst
// ^? - { readonly hello: "world, not string" }

const isString = /** @type {const} */ {
    hello: "world, not string"
}

isString
// ^? - { hello: string }

Playground

@sandersn
Copy link
Member

Probably yes -- but it's an expansion of the Clojure cast syntax that no other tool would recognise, so we should really consider the entire design space if we decide to make casts easier to write.

@sandersn sandersn added this to Not started in PR Backlog Aug 25, 2021
@sandersn sandersn moved this from Not started to Needs merge in PR Backlog Aug 25, 2021
@rbuckton
Copy link
Member Author

The parens are also useful for performance reasons, otherwise we'd have to check every leading JSDoc comment of every expression in a JS file.

What I'd like is a shorter cast syntax (even with the parens). Something like /** {type} */(expr) (removing the @type). I know we allow you to write /** @type type */(expr) (dropping the {}), but that still feels awkward.

I'd also like a way to emulate postfix-! in JSDoc, because the only options currently are a cast (which could be difficult to write from an inferred type), or a generic identity function that removes null | undefined. Not sure what a good jsdoc syntax might be for that though, since there really isn't precedent in Clojure or jsdoc.app.

Something like one of these:

/** @type {!} */(foo.bar).baz; // just the `!` prefix
/** @notnull */(foo.bar).baz; // a keyword-like tag
/** @! */(foo.bar).baz; // or some type of shorthand
/** {!} */(foo.bar).baz;

I'll have to see if there's an open issue for this and, if not, create one.

@rbuckton
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this

Checking rwc and community tests, though I don't think this should affect anything currently.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 27, 2021

Heya @rbuckton, I've started to run the extended test suite on this PR at 6504a34. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 27, 2021

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 6504a34. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@rbuckton
Copy link
Member Author

user test baselines are fine (changes are minor and unrelated)

@rbuckton
Copy link
Member Author

rwc baselines look fine.

@rbuckton rbuckton merged commit 107c556 into main Aug 28, 2021
@mohd-akram
Copy link

Any idea on when this will be available in TypeScript? It's very useful and hopefully could be released soon.

@orta
Copy link
Contributor

orta commented Sep 13, 2021

TypeScript has a set release schedule, the next release details are here: #45418

@mohd-akram
Copy link

Ah, I was hoping this could be released as part of a 4.4 bugfix release.

@phil294
Copy link

phil294 commented Sep 15, 2021

What's the reasoning for not leveraging JSDoc's (currently unused) @constant (short: @const) identifier for this?

@sandersn
Copy link
Member

TS treats Closure cast syntax as the JSDoc cast syntax, so it's feature parity with TS as const casts.

If @const is truly unused in JSDoc, and likely to remain that way, it might be worth adding it as an alternate syntax. But you'd need to open a new issue with a proposal and exploration of the design problems. A couple I can think of:

  • would @const only apply to declarations, like most tags?
  • do people have existing uses of @const that would break in TS?
  • will JSDoc ever want to use @const for some other meaning?

@gaurav5430
Copy link

TypeScript has a set release schedule, the next release details are here: #45418

can't see it in the plans for 4.5
is that right or am I missing something ?

@sandersn
Copy link
Member

@gaurav5430 Fixes don't show up in the iteration plan as far as I know.

@chunjin666
Copy link

Not working in v4.7.4, how can i get this feature.

@orta
Copy link
Contributor

orta commented Jul 2, 2022

Confirming that it is working in 4.7.4

image

I'd recommend filing a full bug report if you have a case where it is not working as expected

@chunjin666
Copy link

chunjin666 commented Jul 2, 2022 via email

colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Dec 19, 2022
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Dec 19, 2022
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Dec 19, 2022
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 9, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 11, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 11, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 16, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 18, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 18, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 19, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 19, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Jan 19, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Feb 2, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
colinrotherham added a commit to alphagov/govuk-frontend that referenced this pull request Feb 6, 2023
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

JSDoc Type cast to const does not work in variable-like initializers const assertions in JSDoc
9 participants