microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.94k stars 12.48k forks source link

Support @ts-ignore for specific errors #19139

Open ghost opened 7 years ago

ghost commented 7 years ago

TypeScript Version: 2.6.0-dev.20171011

Code

function countDown(n: number): void {
    switch (n) {
        // @ts-ignore
        case 1:
            console.log("1");
            // intentional fall through
        case 0:
            console.log("0");
    }
}

Expected behavior:

Ability to make ts-ignore apply to the --noFallthroughCasesInSwitch error but not to other errors.

Actual behavior:

case "1": would also compile.

doberkofler commented 1 year ago

Unfortunately this discussion often assumes a new TypeScript Code base and forgets all "legacy code bases". Especially during the migration from JavaScript to TypeScript it would be very helpful to clearly define what error has (temporarily) to be disabled. I had to migrate a very lathe JavaScript application to TypeScript and it would have been very helpful to only disable one specific problem instead of hiding others when incrementally improving the type sanity.

arthur-clifford commented 1 year ago

@jove4015

If you saw my earlier comments you'll know that I am supportive of this request in general. I also agree that there is a linter aspect to coding in TypeScript and that it should work with you not against you.

However, for the particular case where you are passing a possibly undefined value through that gets changed to null, does it work when calling the function to cast your variable as any? I'm sure you likely know how to do that, but for anybody stumbling on this thread:

someCall(myMaybeUndefinedVar as any)

I think that is what the others are referring to about any casts. Rather than implicitly doing it by hiding the error (because JS couldn't care less about the type), you would be explicitly doing the any cast. If it does the trick, that is "better" in so far as it works consistently within the type system; you are explicitly saying this 3rd party function doesn't care about the type and that you are knowingly overriding ignoring type.

JonasDoe commented 1 year ago

@arthur-clifford: Is it really more explicit to "cast" a variable to any than writing an explicit @ts-ignore specifying comment specifying the kind of error to be ignored right at the caller's side? Imo it's easier to find the latter later on, and after transpilation, the any is gone for sure, too. Additionally, any could hide other compile time errors, too, so I don't find it a proper replacement.

arthur-clifford commented 1 year ago

I would say that your observation is a valid scenario for the ts-ignore if your intent is to look for these things later. I was more trying to diffuse a little angst surrounding responses that were expecting changes to third party libraries and making sure it was understood why people were mentioning any casts. If your ide supports converting //TODO comments into tasks I’d think that would be more likely to remind you to do something in the future rather than thinking to clear out ts ignore commands or removing any casts “later”. But I stand by having options. Sent from my iPhoneOn Feb 13, 2023, at 2:30 AM, JonasDoe @.***> wrote: Is it really more explicit to assume/cast a variable as any, then writing an explicit @ts-ignore specifying comment specifying the kind of error to be ignored? Imo it's easier to find the latter later on, and after transpilation, the any is gone for sure, too.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

jove4015 commented 1 year ago

@arthur-clifford - I appreciate what you're saying, but I'm explicitly not trying to disable all type checking on that line. The line in question is ultimately firing a call through a mocked TRPC router, which goes through zod validation, and then after some wrangling winds up before prisma. I don't want to ignore every single kind of type error in that process. I only want to ignore strict null checks in a tiny number of cases where they pose a problem. If the type is number | number[] | undefined, I still want it to throw an error when someone passes a string.

My goal is to take a precision approach to just ignoring the one rule that is causing me issues, without tossing out the rest of the types system and just casting everything to any. I don't even want the word "any" in my code anywhere. I just want to ignore one rule on one line.

nicholasbailey commented 1 year ago

I'm not trying to be overly pedantic here, but Typescript isn't a "linter", it's a type system. I asked for an example because, when you ts-ignore an error, 9 times out of 10 what you're doing under the hood is basically an any-cast -- you tried to do something that the type system doesn't allow, and by ignoring the error and doing it anyway you're telling TS not to enforce types on that line.

That's exactly correct. But Typescript is also an incremental type system. It is explicitly designed to allow gradual migration from Javascript. If you've ever tried to migrate a 500k line plus codebase to Typescript, you've probably noticed that you can't do it all in one go. You have to work incrementally, which means you have to allow any into your codebase. It makes a great deal of sense to allow developers to make specific type violations in narrow contexts. ts-ignore-error and friends are intended for use in hybrid codebases as an aid in migration. Allowing them to target specific errors would better facilitate that purpose.

fatcerberus commented 1 year ago

I only want to ignore strict null checks in a tiny number of cases where they pose a problem. If the type is number | number[] | undefined, I still want it to throw an error when someone passes a string.

I don't think this feature would help, then, as errors aren't that granular. undefined is not assignable to number is the same error as string is not assignable to number. If you're specifically trying to bypass a null check, just write foo!.

magwas commented 1 year ago

I'm not trying to be overly pedantic here, but Typescript isn't a "linter", it's a type system. I asked for an example because, when you ts-ignore an error, 9 times out of 10 what you're doing under the hood is basically an any-cast -- you tried to do something that the type system doesn't allow, and by ignoring the error and doing it anyway you're telling TS not to enforce types on that line.

No difference between the two on high level: both enforces a particular set of rules against the source code. The type system is just specifically enforces that the input and output value constraints should be explicitly spelled out.

magwas commented 1 year ago

Although I agree with those saying that TS should support migration from legacy code, here is a use case with code which never have been JS, and written with all type enforcements turned on:

It is a library to support building things using the fluent pattern. Therefore the type for this contains quite a lot of optional fields which are meant to filled in gradually. And there is one field which will be reset to undefined in one special case. With exactOptionalPropertyTypes turned on, that special case is flagged as an error. I could add |undefined to the field, but I want to ensure that this particular operation is done in exactly one place in the whole codebase.

And yes, I could still add some type magic to that particular function to allow it there specifically.

But from the ease of review standpoint it is much easier to just automatically mark all comments as something needing discussion than put the type magic there, and detect/prevent cut&pastes/reuses of that.

arthur-clifford commented 1 year ago

I’m not sure I perfectly followed what you were saying here. But generally isn’t the idea to export a type a named type like:export typ e myType = number | Array | undefinedOr, whatever you need it to be. then use that type as the type for your optional variable type wherever that combo of types is needed so that you aren’t copying and pasting the long type definition. I don’t think that is type magic as much as it is clearly describing the range of options for your optional variable. And if you name it like OptionalNonString then that would be more meaningful for review’s sake wouldn’t it?I apologize if I misunderstood what you were saying. Im not sure what you are gaining from an override in this case. Sent from my iPhoneOn Mar 30, 2023, at 10:44 PM, Árpád Magosányi @.***> wrote: Although I agree with those saying that TS should support migration from legacy code, here is a use case with code which never have been JS, and written with all type enforcements turned on: It is a library to support building things using the fluent pattern. Therefore the type for this contains quite a lot of optional fields which are meant to filled in gradually. And there is one field which will be reset to undefined in one special case. With exactOptionalPropertyTypes turned on, that special case is flagged as an error. I could add |undefined to the field, but I want to ensure that this particular operation is done in exactly one place in the whole codebase. And yes, I could still add some type magic to that particular function to allow it there specifically. But from the ease of review standpoint it is much easier to just automatically mark all comments as something needing discussion than put the type magic there, and detect/prevent cut&pastes/reuses of that.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

SiddHyper commented 1 year ago

when will they add this feature

typeholes commented 1 year ago

I'll be adding this check to my extension, but the release timeline is somewhere between a long time from now and never so you can use ts-morph and something like this code if you really want to check that casts are handling specific error codes

function checkIgnoreTSC(project: Project) {
   project.getSourceFiles().forEach((file) => {
      console.log(`checking ignore TSC for file ${file.getFilePath()}`);

      file.getDescendantsOfKind(SyntaxKind.AsExpression).forEach((expr) => {
         const castText = expr.getTypeNode()?.getText() ?? 'no type node';
         const holdText = expr.getFullText();
         const exprText = expr.getExpression().getFullText();
         console.log(castText);
         const exprLineNumber = expr.getEndLineNumber();
         if (castText.match(/^ignore_TSC[0-9]+$/)) {
            const code = castText.slice(10);
            const replacedWith = expr.replaceWithText(
               `${exprText} /*${castText}*/`
            );

            const err = file
               .getPreEmitDiagnostics()
               .find(
                  (diagnostic) => diagnostic.getLineNumber() === exprLineNumber
               );
            if (err === undefined) {
               console.log(
                  `No error found at line: ${exprLineNumber} ${castText})`
               );
            } else {
               const errCode = err.getCode().toString();
               if (errCode !== code) {
                  console.log(
                     `Incorrect error ${errCode} vs ${code} at line: ${exprLineNumber} ${castText})`
                  );
               }
            }
            replacedWith.replaceWithText(holdText);
         }
      });
   });
}
type ignore_TSC1234 = any;
type ignore_TSC2322 = any;

const no_error: number = 1 as ignore_TSC1234;

const should_mismatch: string = 1 as ignore_TSC1234;

const should_match: string = 1 as ignore_TSC2322;

ignore_TSC1234
No error found at line: 4 ignore_TSC1234)
ignore_TSC1234
Incorrect error 2322 vs 1234 at line: 6 ignore_TSC1234)
ignore_TSC2322
xsjcTony commented 1 year ago

Any plan for this to be implemented?

tx0c commented 1 year ago

error TS2445 ....

when tsc reports a single error, can ask ignore a single error only? e.g.

// @ts-ignore TS2445
ADTC commented 1 year ago

Multiple errors. Maybe case insensitive too.

// @ts-ignore TS2445 ts1942 Ts552

Right now, this is a pipe dream. It's all or nothing, folks.

I like using @ts-expect-error though. At least if the line no longer has an error, this throws an error and we have to remove this comment and clean up the code. :D

xsjcTony commented 1 year ago

Or even better we can have

// @ts-expect-error TS2445 ts1942 Ts552
minecrawler commented 1 year ago

how would expect-error work? There needs to be an error else it doesn't build? That sounds dangerous, do you have a use-case? I'm not sure if that wouldn't be out of scope of the original request

liamjones commented 1 year ago

@minecrawler @ts-expect-error is already something that exists, it just can't be targeted to specific codes currently.

It's, IMO, often preferable over using @ts-ignore when you encounter something that you have to work around for now but should be fixed in future TS/dependency versions as both directives have some level of danger (being a global error ignore for the subsequent line).

Having the @ts-expect-error error once the problem is fixed (by seeing Unused '@ts-expect-error' directive.(2578) later) means you get rid of the dangerous error silencing behaviour in your codebase sooner.

Contrived but simple example:

// v1 of some module I depend on - their function is typed wrong, it says it takes string not number.
// Assume these two calls are in different parts of a big application:
someFunctionFromAModule(4) // Argument of type 'number' is not assignable to parameter of type 'string'.(2345)
someFunctionFromAModule(4) // Argument of type 'number' is not assignable to parameter of type 'string'.(2345)

// So, we add ts-ignore/ts-expect-error and report the issue to the project owners
// @ts-expect-error types wrong
someFunctionFromAModule(4) // No error
// @ts-ignore types wrong
someFunctionFromAModule(4) // No error

// Upgrade to v2 and they fixed the typing issue, we get prompted about the ts-expect-error no longer applying but not the ts-ignore
// @ts-expect-error types wrong
someFunctionFromAModule(4) // Unused '@ts-expect-error' directive.(2578)
// @ts-ignore types wrong
someFunctionFromAModule(4) // No error

// So we remove the ts-expect-error but forgot about the ts-ignore elsewhere in the codebase
someFunctionFromAModule(4) // No error
// @ts-ignore types wrong
someFunctionFromAModule(4) // No error

// Later we upgrade to v3 of the module, but didn't notice they'd renamed this function
// Since we were prompted to remove the ts-expect-error when our original issue was cleared we have now been prompted that this function doesn't exist anymore
someFunctionFromAModule(4) // Cannot find name 'someFunctionFromAModule'.(2304)
// But, because this one still has a ts-ignore on it we forgot about our code just dies when it reaches this point
// @ts-ignore types wrong
someFunctionFromAModule(4) // No error

Yes, unit tests, e2e testing etc could also pick up the second call's problem but the choice of @ts-expect-error over @ts-ignore allows us to pick up the issue earlier in the process.

xsjcTony commented 1 year ago

Exactly as described above, using @ts-ignore generally considered harmful to the codebase, where you have no idea whether the suppressed line errored because of some unnoticed updates

gautelo commented 1 year ago

Would love to see this. I was actually a bit surprised that there wasn't already support for this.

There is prior art to use as inspiration in eslint. Things like this:

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const unusedVar = 'hello';

Would love to see this implemented for @ts-expect-error and @ts-ignore. :pray:

ehoops-cz commented 1 year ago

We did (most of) a typescript migration and are now going through a major refactor. We would really like to enable strictNullChecks and noImplicitAny to ensure the quality of our refactored code going forward. However, our codebase has ~1500 errors for each rule and we aren't going to be able to fix all of them immediately. It's worrisome to completely ignore 3000 lines of code and we would much prefer to ignore only these specific errors.

Please consider adding this feature.

tylerlaprade commented 1 year ago

@ehoops-cz, I would also like to see this feature added, but I don't expect it any time soon. In the meantime, you should use a ratchet

wscourge commented 11 months ago

Is there any work being done on this?

doberkofler commented 11 months ago

@wscourge I have been using TypeScript for years and it is the single most annoying problem that bites us again and again. Unfortunately I don't think this will ever happen given all the comments by members of the TypeScript team.

wscourge commented 11 months ago

@doberkofler there's so much to read through, could you please show which comments do you mean? I'd like to argue against them.

wscourge commented 11 months ago

https://github.com/microsoft/TypeScript/issues/22011 here I found "no conclusion yet", so I suspect there was sth further?

sarimarton commented 11 months ago

@wscourge I clearly remember that they argued with something like that the TS error codes are assigned to their message text, not the actual technical error. I.e., if they change the text of one error, they are gonna assign a new TS-number to that, leaving the previous number unused. However silly this sounds, this, according to them, ensures some kind of googling consistency, and hence, the numbers aren't meaningful representation of the errors, hence, they don't want to encourage us to rely on it.

doberkofler commented 11 months ago

@wscourge Have a look at the PR #21602 and SR #19139

standbyoneself commented 5 months ago

PR is closed. What now? Any plans on this?

kicumkicum commented 3 months ago

+1 I need this feature

laverdet commented 3 months ago

This would be nice to have. This isn't just about legacy codebases or incorrect types. TypeScript isn't perfect and has bugs of its own. I ran into this today via #54302

function overload(vals: [ val?: number ]): unknown;
// ^ This overload signature is not compatible with its implementation signature.ts(2394)
function overload(vals: Iterable<number>): undefined {
}

This is perfectly sound under exactOptionalPropertyTypes but it raises an error which cannot be safely suppressed.

Notenlish commented 1 month ago

PR is closed. What now? Any plans on this?

why was the pr even closed?

mifi commented 6 days ago

the lack of specific ts-ignore support makes options like noUnusedLocals noUnusedParameters not very useful, compared to their eslint counterparts which can be specifically disabled on a line-by-line basis

cyberixae commented 5 days ago

Making sure noUnusedLocals is disabled is among the first things I do when I start working on a new TypeScript project. Disabling the rule speeds up development a lot since I don't have to worry about unused locals during development. For the feature to make sense you would need to have separate tsconfig files for production builds and development. ESLint has a separate "warning" severity that can be used to indicate that a rule is not relevant during development. Not sure if it makes sense for TypeScript to compete with ESLint. In my opinion limiting scope of ts-ignore would be more useful in situations where no good ESLint rule exist for doing the same job.

ADTC commented 5 days ago

@cyberixae I can't look at your codebase! I can't stand unused locals. I must annihilate them immediately.