search

microsoft / TypeScript

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

Ignore specific errors #11051

Closed basarat closed 7 years ago

basarat commented 7 years ago

This keeps getting asked on stackoverflow (just this morning : http://stackoverflow.com/a/39627892/390330) so thought there should be an issue to track it.

Ideation

Lots of linters do this:

There needs to be 🚲 🏠 here so have at it :rose:

More

mhegazy commented 7 years ago

man.. this is one of my all time favorite topics :) As I mentioned in https://github.com/Microsoft/TypeScript/issues/9448#issuecomment-229780542, https://github.com/Microsoft/TypeScript/issues/8855#issuecomment-223160647, https://github.com/Microsoft/TypeScript/issues/6771#issuecomment-178104716, https://github.com/Microsoft/TypeScript/issues/3691#issuecomment-168801858, and i am sure there is more, errors are not the problem, and suppressing them is not the solution; this analogues of switching off your fire alarm. The real problem is you have a faulty fire alarm. So instead of muffling the errors, I would like to know what is the specific error, and why you find it useless. then we can talk how we can make the experience better.

yortus commented 7 years ago

Instead of suppressing compiler errors, it would be interesting to discuss the pros/cons of being able to override compiler options at the file/function level, at least those options that have only local effect.

The closest equivalent to this at the moment is to break up into multiple projects, each with their own tsconfig.json file. Perhaps that's the best solution for major splits (eg a project with both node (target: ES6) and browser (target: ES5) code).

But sometimes I wish I could override an option in a few places without spliting up the project. E.g., I could set the compiler options in tsconfig.json to their strictest setting, and then opt-out of them in a few specific declarations that for whatever reason TypeScript can't analyse properly, or that I need to revisit, but I know are not errors.

I think some people are not adopting the strictest flags at all, because they are all-or-nothing project-wide and bring up huge numbers of errors on their existing codebase.

yortus commented 7 years ago

A couple of examples to go with my previous comment:

Here is a util function from bluebird:

function toFastProperties(obj: any) {
    /*jshint -W027,-W055,-W031*/
    function FakeConstructor() {}
    FakeConstructor.prototype = obj;
    var l = 8;
    while (l--) new FakeConstructor();
    ASSERT("%HasFastProperties", true, obj);
    return obj;
    // Prevent the function from being optimized through dead code elimination
    // or further optimizations. This code is never reached but even using eval
    // in unreachable code causes v8 to not optimize functions.
    eval(obj);
}

It would be nice not to have to set allowUnreachableCode: true project-wide just because of this one special case. That would cause a loss of safety in the rest of the project. What if we could preface just this function with // @allowUnreachableCode: true, thereby preserving unreachability checking everywhere else in the project?

Here is part of a function from the TypeScript compiler:

            switch (searchSpaceNode.kind) {
                case SyntaxKind.MethodDeclaration:
                case SyntaxKind.MethodSignature:
                    if (isObjectLiteralMethod(searchSpaceNode)) {
                        break;
                    }
                // fall through
                case SyntaxKind.PropertyDeclaration:
                case SyntaxKind.PropertySignature:
                case SyntaxKind.Constructor:
                case SyntaxKind.GetAccessor:
                case SyntaxKind.SetAccessor:
                    staticFlag &= getModifierFlags(searchSpaceNode);
                    searchSpaceNode = searchSpaceNode.parent; // re-assign to be the owning class
                    break;
                case SyntaxKind.SourceFile:
                    if (isExternalModule(<SourceFile>searchSpaceNode)) {
                        return undefined;
                    }
                // Fall through
                case SyntaxKind.FunctionDeclaration:
                case SyntaxKind.FunctionExpression:
                    break;
                // Computed properties in classes are not handled here because references to this are illegal,
                // so there is no point finding references to them.
                default:
                    return undefined;
            }

Setting noFallthroughCasesInSwitch: false project-wide would cause a loss of safety in the rest of the project. What if we could preface just this function with // @noFallthroughCasesInSwitch: false, thereby preserving fall-through checking everywhere else in the project?

RyanCavanaugh commented 7 years ago

I really appreciate the examples -- these are what we're looking for, to be sure! But I'm going to break character here and state my strong subjective opinion about user code, which I usually try hard not to do: Both of those examples are horrifying.

Bluebird could just as easily use something like !!false && eval(obj) to avoid that error. Or wrap the whole thing in a meaningless try / catch, which deopts to my knowledge. Anything they do here is just super-suspect anyway as there are no guarantees V8 won't start doing smarter analysis and ignoring their unreachable eval. I would guess a hyper-low-level function like this (the 8-times loop to force a psuedoclass to be created... what?) occurs in extremely few libraries.

Our own use of IFT here is awful -- the second fall-through doesn't even save any LOC compared to just writing break; at the // Fall-through comment! The first instance is somewhat more excusable but I would still prefer we just never do this.

Again, my own opinion. I would like to see more examples and will try not to judge them so hard.

yortus commented 7 years ago

@RyanCavanaugh how would you prefer to rewrite the first fall-through in the tsc example? OR just not use switch at all?

RyanCavanaugh commented 7 years ago 
            switch (searchSpaceNode.kind) {
                case SyntaxKind.PropertyDeclaration:
                case SyntaxKind.PropertySignature:
                case SyntaxKind.Constructor:
                case SyntaxKind.GetAccessor:
                case SyntaxKind.SetAccessor:
                case SyntaxKind.MethodDeclaration:
                case SyntaxKind.MethodSignature:
                    if ((searchSpaceNode.kind === SyntaxKind.MethodDeclaration) || 
                        (searchSpaceNode.kind === SyntaxKind.MethodSignature)) {
                        if (isObjectLiteralMethod(searchSpaceNode)) {
                            break;
                        }
                    }                                            
                    staticFlag &= getModifierFlags(searchSpaceNode);
                    searchSpaceNode = searchSpaceNode.parent; // re-assign to be the owning class
                    break;
kitsonk commented 7 years ago

Also, personal opinion, --noFallthroughCasesInSwitch can easily be turned off and put in the domain of a linter, which is where I think it personally is better suited.

As far unreachable code, I originally felt that it was the domain of a linter, but now seeing what CFA provides, I now feel that when you have a compiler the is evaluating the flow of your code, then unreachable code becomes an error, versus a stylistic issue, even if it is still valid at run-time. It is now clearly in the wheelhouse of TypeScript.

yortus commented 7 years ago

@RyanCavanaugh Oh cool, I thought empty cases also triggered a fall-through error but they don't.

yortus commented 7 years ago

Bluebird could just as easily use something like !!false && eval(obj) to avoid that error.

...sure, until Anders adds some more CFA goodness that statically detects that as unreachable code.

kitsonk commented 7 years ago

...sure, until Anders adds some more CFA goodness that statically detects that as unreachable code.

Well, it could be argued there is a big difference between an unreachable statement and a short circuited expression logic. I think that might be the argument with !!false && eval(obj) in that I know many minifiers evaluate expression logic like that for short circuits and do dead code removal. On the other hand, that is true for unreachable code as well. In any case, it is a dog chasing its tail, put one compiler hint here to avoid another compiler making a mistake.

yortus commented 7 years ago

@kitsonk tsc has already started clamping down on side-effect free (SEF) expressions as per #10814. It could in future (I'm speculating) also complain about !!false && eval(obj) as a side-effect-free (i.e. useless) expression. Interestingly the PR that fixes #10814 uses allowUnreachableCode as the opt-out flag to suppress SEF errors.

mhegazy commented 7 years ago

One philosophical distinction between errors of the like of noImplicitAny, can not find module, or type not assignable to, and ones reported by --allowUnreachableCode, --noFallthroughCasesInSwitch, and --noImplicitReturns, is that the later are more of linter functionality, that we are adding since "we are already there".

The compiler has done the control flow analysis, already so it might as well tell you you have an unreachable statement. these are not core to the type system, and are meant for the 90% case. if you want more customization for these you can switch them off entirely, and use a linter rule. linters already are built with exceptions for specific errors and different levels, etc..

The other errors are really at the core of what the compiler does. for instance, if it does not know what shape a module has, it can not say anything about the imports, or how they are used, further use of these types would not be safe either, since an error type are just any.,the tools can not guarantee that renaming x in one place will be safe, etc.. at the core is the TypeScript value -proposition.

Now, I am not saying all the errors that the compiler emit today are ideal. And this is why we should be having these discussions. and for instance, short-hand modules, and using of wild cards in module names were conceived because of users complaining about a too-stringent error message. if they were to switch that one off, they would have masked the real cause, and instead see a degradation of service in the compiler and the tools.

webia1 commented 7 years ago

The reason, why we would need such of feature is very simple:

We have old applications, which we have to migrate step by step into a new framework. (An example would be migrating old JavaScript/TypeScript/.. Codes from Angular1 to 2)

In the earlier steps we have to suppress some errors like this[foo] = bar and some other ones, so that the app is first runnable. If we can run the app in the new framework, we get the decision from Marketing/Management Department for the refactoring in depth. Marketing department is absolutely not interested in compiler errors.

We have a pre-configured build system (big company) and may only change tsconfig.json. Currently the build sytem drums out our app because of existing errors. And there are a big amount of source codes and we have not the resources to correct every error in a short given time. Therefore we need some feature like to deactive certain errors, which were ok at that time (at the time of writing of that old stuff and the list is long.

As you see, there are certain use cases, that are not considered by the creators of TypeScript :) The world is not perfect and you may not always expect perfect pre-conditions. Therefore the ability to deactivate (be it very important) errors is an essential feature for the real world.

Thank you!

mhegazy commented 7 years ago

As you see, there are certain use cases, that are not considered by the creators of TypeScript :) The world is not perfect and you may not always expect perfect pre-conditions. Therefore the ability to deactivate (be it very important) errors is an essential feature for the real world.

The typescript code base does not have --noImplicitThis or --strictNullChecks enabled for the same reason. so is not too strange of a scenario to the core team. what we have done instead is try with the new flag on, try to fix issues as much as possible, then iterate. once all issues have been fixed flip the switch. that has worked in the past for different new switches, we would hope it works with strinctNull as well.

webia1 commented 7 years ago

Dear Mohamed, thank you very much for your feedbacks but I can not emphasize that enough; 

   I want to suppress, the error, which I want. That can be any of them.

I appreciate the work from Microsoft very much, TS has a huge potential to be THE LANGUAGE OF WEB, but there is still no complete list, a kind of itemization of Error-Codes (TSWhatever <--> TSLint-Rule <--> Available Custom Rules <--> ...), we have to fish out them out of the source code. I think, there is still much work to do.

dryajov commented 7 years ago

I have a specific use case from mongoose schemas. In order to have a recursive schema, you reference this in one of the properties in the schema declaration. Example:

MySchema: Schema = new Schema({
  _id: String,
  name: String,
  children: [this], // TODO: figure out how to disable TS from complaining about this
});

I get this error:

Error:(25, 14) TS2683:'this' implicitly has type 'any' because it does not have a type annotation.

It doesn't seem like I can cast this to anything specific, I've tried several possible syntax combinations, like <> or as but it rejects everything I've tried. It also doesn't seem to be possible to reference MySchema from within MySchema because it hasn't been declared yet. What's an appropriate workaround here? It seems like disabling this error on a class/method/line basis would be appropriate in cases like this. That or fixing the compiler to allow casting in situations like this.

alexanderbird commented 7 years ago

@mhegazy, that's exactly it - for partially migrated projects, to use the stricter flags you have this workflow: (as you described)

  1. Develop with the strictness turned down
  2. When finished working, turn the strict flags on
  3. Fix the errors from the extra strict flags, but ignore all the noise of errors for your unmigrated files
  4. Flip the flags back to less strict, and commit

Doesn't that seem like a broken workflow? If we had a way to disable specific errors for specific files, we wouldn't have to do the work around. For example, how about a triple-slash directive: 

/// <compiler-option noImplicitThis=false />
/// <compiler-option strictNullChecks=false />

The typescript code base does not have --noImplicitThis or --strictNullChecks enabled for the same reason.

With some way to ignore specific errors for specific files, maybe the typescript code base could use --noImplicitThis and --strictNullChecks

webia1 commented 7 years ago

man.. this is one of my all time favorite topics :) As I mentioned in #9448 (comment), #8855 (comment), #6771 (comment), #3691 (comment), and i am sure there is more, errors are not the problem, and suppressing them is not the solution; this analogues of switching off your fire alarm. The real problem is you have a faulty fire alarm. So instead of muffling the errors, I would like to know what is the specific error, and why you find it useless. then we can talk how we can make the experience better.

No matter how absurd it sounds:

yes, I want to disable the fire alarm and intentionally set on fire, because I have to test ( = management decision), how the people act, if there is fire, without fire alarm.

rostacik commented 7 years ago

hi all. I just made a small poor man's solutions. basically you might grep the output of TS build from console a not show those you wand to mute. at the end of the day , TS will output JS but you will get just errors you want to see. http://rostacik.net/2016/11/10/how-to-filter-particular-typescript-errors-in-build-result/

webia1 commented 7 years ago

@rostacik 👍 😄 It work's like an Asprin, the headache is still existing, but you don't feel it anymore,..

rostacik commented 7 years ago

@webia1 I am not proud I "fixed" it like this , but it's best I could do with managers breathing down my neck :)

cheers mate

108adams commented 7 years ago

@webia1 This is exactly my case. I'm migrating a HUGE app from Backbone, even worse, keeping global scope and Backbone for a big(ger..) part of the app still in place. Migrating part, checking if it works and then tightening type system and language - correctness is exactly my path I need to follow, as I have no spare year to refactor/rewrite the parts of legacy code (at the moment at least...). All the error messages about the errors I am aware anyway are actually noise blurring the real issues.

I do strongly support @basarat

Adam

mhegazy commented 7 years ago

@108adams what kind of errors are you getting? can you elaborate?

KiaraGrouwstra commented 7 years ago

I used to suppress errors for this:

fn(
  a,
  b, // <- note the trailing comma
)

Why? Because I don't care it wasn't allowed in plain JS; I don't wanna be shuffling commas whenever I add/remove/reorder parameters. Nor do I want to see lines about comma changes in git.

Disclaimer: I don't recall fighting TS over this one recently, probably fixed!

KiaraGrouwstra commented 7 years ago

@RyanCavanaugh: right, that doesn't seem an error anymore -- I appreciate that. I just used it as an (outdated) example to demonstrate how there could be points where users might wish to be able to choose what to adhere by themselves.

DomBlack commented 7 years ago

Being able to turn off flags per file will allow new flags to be switched on at a project level, and then slowly (as time allows) the flag can be switched off per file and that file fixed.

This way the new code being written is tested under the new flags, prevent new bugs being written, while the code base is migrated over time.

I just updated a code base from 1.8 to 2.1 and want to turn on strict null checks, however I have over 1000 issues which I don't have time to fix before having to move onto new code. This has lead me to the unfortunate position of having to turn the flag off again before issuing the PR for the upgrade.

studds commented 7 years ago

if you want more customization for these you can switch them off entirely, and use a linter rule. linters already are built with exceptions for specific errors and different levels, etc.. (https://github.com/Microsoft/TypeScript/issues/11051#issuecomment-248971532)

While I agree with this in principle, in practice it's difficult as the TSLint team is gradually removing linting rules as they get added to tsc - for example https://github.com/palantir/tslint/issues/661 deprecates no-unreachable and https://github.com/palantir/tslint/issues/1481 deprecates no-unused-variable.

I agree that ideally errors should be tuned and not ignored; that less than ideal code should be improved and not papered over; and that a dedicated linter should be used for maximum flexibility.

However, if the compiler is going to stray into the traditional territory of a linter, and especially where this encourages linters to remove rules, wouldn't it be pragmatic to add some small part of the flexibility a linter provides?

RyanCavanaugh commented 7 years ago

For lint-type rules, we either a) do add a configuration option and/or workaround ("no unused" is project-configurable and parameters can be prefixed with _) or b) consider the code to be unjustifiably suspicious (e.g. if (expr); {).

studds commented 7 years ago

I did not mean to overlook the existing configuration options when I suggested the compiler should "add some small part of the flexibility a linter provides". I suppose that I mean specific options should be available per file / function / line, as you are able to do with a linter.

abettadapur commented 7 years ago

Another example, where I need to disable a rule per file.

React requires you to declare this object to use the Context feature, which is never used throughout the file. When NoUnusedLocals is turned on, this throws a compiler error because it is never directly used in the file.

private static childContextTypes: React.ValidationMap<any> = 
{
        functionA: React.PropTypes.func.isRequired,
        functionB: React.PropTypes.func.isRequired,
        functionC: React.PropTypes.func.isRequired
};
grovesNL commented 7 years ago

There are many TypeScript code generation tools that have issues with this as well. In some cases the code generation has to do quite a lot of effort in order to determine which parameters/locals are unused. This may not even be possible depending on which extension points are exposed, because the code generator may not have enough information to make this decision.

With TSLint this was never a problem because the code generator could include /*tslint:disable*/ at the top of generated files. Now that people are moving to the TypeScript rules the code generators have to try to enforce every rule that consumers might happen to use in their projects.

Besides code generation, personally I was in the same position as @DomBlack and wanted to enable every strict rule because they're extremely useful. However I found myself toggling the rules on and off constantly while making commits so it wasn't worth the effort. It's just not worthwhile for existing (sizable) code bases to enable these rules unless the rules can be toggled per file.

mhegazy commented 7 years ago

Seems to be the same issue as https://github.com/Microsoft/TypeScript/issues/9448. Closing in favor of https://github.com/Microsoft/TypeScript/issues/9448.

sebdoucet commented 6 years ago

An other example with code generation with polymer...

@Polymer.decorators.observe("items")
private _onItemListChanged(newList: ListItemSource, oldList: ListItemSource): void {

Compilation fail: '_onItemListChanged' is declared but its value is never read.

Very sad to have no choice but changing the scope of the function.

dyst5422 commented 6 years ago

How about in the case that the compiler does not work correctly?

Specific example being #21699

There needs to be a way to work around these issues temporarily. And saying that it still emits JS we can use is not sufficient as a lot of tooling depends on the compiler information and having red squigglies all over my screen for issues that are not actual problems (neither from a runtime or type-checking standpoint) is incredibly challenging.

Having the ability to disregard a list of error codes doesn't affect the type-checking capabilities either.

thw0rted commented 6 years ago

Hi @mhegazy, I don't think this is strictly the same issue as #9448. This issue asked for:

If this is the best issue to "ignore errors per file" then please re-open it, or direct me to an issue specifically about that capability as I haven't found one (or I could certainly open a new one).

I was hoping to find some kind of an exclude pattern for strict checks, along the lines of:

"compilerOptions": {
  "strict": {
    "exclude": ["./lib/notMyProblem/**", "../someProjectWithGeneratedCode/**"]
  }
}

I was hoping to import from 3rd party or generated TS and not worry about quality checks, while doing strict checking on all the code that is "my responsibility". I found this issue because jsweet makes "pretty good" TS but it lights up the compiler with dozens of strict-mode failures, and as an earlier comment suggests it might not be possible to avoid them.

mhegazy commented 6 years ago

I was hoping to import from 3rd party or generated TS and not worry about quality checks, while doing strict checking on all the code that is "my responsibility".

consider using --noLibCheck to avoid checking and reporting errors in .d.ts files.

thw0rted commented 6 years ago

In my particular case, I'm importing from the TS files directly, rather than having to keep a separate tsc --watch running as I make changes to both the consuming project and the (peer project) library. If I pre-compiled the library and used the resulting .d.ts files, --noLibCheck would help. Maybe my use case (all straight TS, running on ts-node, some of which is "mine" and some of which is "third party") isn't the way it's intended to be used?

mhegazy commented 6 years ago

if you are using .ts files, they really become "your sources". so if the files are auto-generated, and have errors, that seems to me to be a bug in the generator.

thw0rted commented 6 years ago

I'm going to have to think about the cleanest way forward here. It's kind of a perfect storm of edge cases.

I'm porting somebody else's Java library to run in Node, and used jsweet to get a head start. The TS output is "pretty good" but has some linter / strict checking issues, and I think it would unreasonably complicate the conversion logic to make the output pass tsc strict checks. I would compile those generated TS files to JS in a more permissive mode and then use them, but I want to write my consuming code in TS also so I was running everything in ts-node. That's a first for me so I'm not sure how well it would handle imports from TS-to-JS compiled code vs importing directly from the TS, in terms of debugging / sourcemaps.

The way I see it, I can: