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

"If this was intentional, convert the expression to 'unknown' first" diagnostic should come with a codefix #28067

Closed
ghost opened this issue Oct 22, 2018 · 8 comments · Fixed by #28281
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@ghost
Copy link

ghost commented Oct 22, 2018

TypeScript Version: 3.2.0-dev.20181020

Code

0 as string

Expected behavior:

Codefix to convert to 0 as unknown as string.

Actual behavior:

No codefix.

@ghost ghost added Suggestion An idea for TypeScript Domain: Quick Fixes Editor-provided fixes, often called code actions. labels Oct 22, 2018
@DanielRosenwasser DanielRosenwasser added Help Wanted You can do this Good First Issue Well scoped, documented and has the green light labels Oct 24, 2018
@DanielRosenwasser DanielRosenwasser added this to the Community milestone Oct 24, 2018
@ryanclarke
Copy link
Contributor

ryanclarke commented Oct 25, 2018

I'm interested in taking this issue. Any pointers you think would be helpful for a first-time Typescript contributor?

@ghost
Copy link
Author

ghost commented Oct 26, 2018

Look at some existing simple codefixes like addMissingInvocationForDecorator.
The code fix should be registered corresponding the the error code for "If this was intentional, convert the expression to 'unknown' first" (search for the error text in diagnosticMessages.json). Then to make the change all you have to do is replace the type assertion's in expression with another node from createTypeAssertion or createAsExpression (depending on what kind the original type assertion is). You can use changeTracker.replaceNode for that.
For an example of a test, see tests/cases/fourslash/codeFixAddMissingInvocationForDecorator01.ts.

@ryanclarke
Copy link
Contributor

ryanclarke commented Oct 29, 2018

My fix for this issue is coming along nicely. In fact, I had finished and tested it before I noticed you wanted this to apply to TypeAssertions as well as AsExpressions. So I'm working on adding that functionality. Just to verify, <string>0 should codefix to <string><unknown>0?

@ghost
Copy link
Author

ghost commented Oct 29, 2018

<string> <unknown> 0

@itmayziii
Copy link

@andy-ms

I'm hoping you can clear up some confusion I'm having with this solution. I don't find the double assertion first to unknown and then to the type we want to be very elegant or even consistent with documentation.

The basic basic types docs mentions in the type assertion section

Type assertions are a way to tell the compiler “trust me, I know what I’m doing.” A type assertion is like a type cast in other languages, but performs no special checking or restructuring of data. It has no runtime impact, and is used purely by the compiler. TypeScript assumes that you, the programmer, have performed any special checks that you need.

Doing a double cast seems to go against the whole "trust me, I know what I'm doing" theory behind type assertions. Is the reason for this a technical limitation with the typescript compiler?

@DanielRosenwasser
Copy link
Member

It's "trust me I know what I'm doing" within means. We try to ensure there's some overlap, and if not you can do jump up and down the type hierarchy.

@itmayziii
Copy link

Thanks for the response. I hope your team will reconsider this approach.

  1. The syntax is ugly and would turn me away from typescript if I was a beginner
  2. Violates the idea the programmer knows what they are doing over the compiler.

This seems to be the equivalent to having a double confirmation dialog in a UI. In this analogy I imagine a pop up box asking a user if they accept terms and conditions, and when they hit accept they are prompted again if they are sure they accept the terms and conditions.

Just as that double confirmation is not a good user experience in a UI, having to double assert what you want is not a good user experience when programming.

@ackvf
Copy link

ackvf commented Feb 24, 2019

I hope we can switch this off in tsconfig.

@microsoft microsoft locked as resolved and limited conversation to collaborators Feb 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants