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

Ignore specific errors #11051

Closed
basarat opened this issue Sep 21, 2016 · 39 comments
Closed

Ignore specific errors #11051

basarat opened this issue Sep 21, 2016 · 39 comments

Comments

@basarat
Copy link
Contributor

basarat commented Sep 21, 2016

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:

  • Ignore errors for regions (using inline comments /** */).
  • Ignore errors per file (configured using tsconfig or command line)
  • Ignore certain error codes (configured using tsconfig or command line)

There needs to be 🚲 🏠 here so have at it 🌹

More

@mhegazy
Copy link
Contributor

mhegazy commented Sep 22, 2016

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.

@yortus
Copy link
Contributor

yortus commented Sep 22, 2016

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
Copy link
Contributor

yortus commented Sep 22, 2016

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
Copy link
Member

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
Copy link
Contributor

yortus commented Sep 22, 2016

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

@RyanCavanaugh
Copy link
Member

            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
Copy link
Contributor

kitsonk commented Sep 22, 2016

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
Copy link
Contributor

yortus commented Sep 22, 2016

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

@yortus
Copy link
Contributor

yortus commented Sep 22, 2016

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
Copy link
Contributor

kitsonk commented Sep 22, 2016

...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
Copy link
Contributor

yortus commented Sep 22, 2016

@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
Copy link
Contributor

mhegazy commented Sep 22, 2016

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
Copy link

webia1 commented Sep 26, 2016

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
Copy link
Contributor

mhegazy commented Sep 29, 2016

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
Copy link

webia1 commented Oct 1, 2016

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
Copy link

dryajov commented Oct 6, 2016

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
Copy link

@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
Copy link

webia1 commented Nov 3, 2016

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
Copy link

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
Copy link

webia1 commented Nov 10, 2016

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

@rostacik
Copy link

@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
Copy link

108adams commented Nov 18, 2016

@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
Copy link
Contributor

mhegazy commented Nov 18, 2016

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

@KiaraGrouwstra
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

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
Copy link

studds commented Feb 16, 2017

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.. (#11051 (comment))

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 palantir/tslint#661 deprecates no-unreachable and palantir/tslint#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
Copy link
Member

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
Copy link

studds commented Feb 17, 2017

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
Copy link

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
Copy link

grovesNL commented Mar 1, 2017

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
Copy link
Contributor

mhegazy commented Apr 21, 2017

Seems to be the same issue as #9448. Closing in favor of #9448.

@sebdoucet
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

mhegazy commented Apr 25, 2018

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
Copy link

thw0rted commented Apr 26, 2018

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
Copy link
Contributor

mhegazy commented Apr 26, 2018

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
Copy link

thw0rted commented Apr 27, 2018

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:

  • Keep everything purely TS, and disable strict checking (but miss obvious errors in my own code)
  • Keep everything TS, with strict checking, and hand-jam fixes into the TS generated by jsweet (time consuming)
  • Compile the library TS to JS and refer to that, using --noLibCheck (and maybe have sourcemap complications with ts-node)
  • Show up here and request a feature to selectively enforce strictness (very little downside for yours truly 😁 )

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests