implemented treat warning as errors commandline option. #966

DickvdBrink · 2014-10-25T17:28:13Z

Same name as csc.exe uses: msdn
Issue #828

Feedback appreciated but feel free to close when you don't want this functionality at this point :)

RyanCavanaugh · 2014-10-26T00:25:55Z

src/compiler/types.ts

@@ -1092,6 +1092,7 @@ module ts {
        sourceRoot?: string;
        target?: ScriptTarget;
        version?: boolean;
+        warnaserror?: boolean;


Please camelCase this

CyrusNajmabadi · 2014-10-26T07:22:43Z

I"m not sure what the purpose of this is. Do we have any actual warnings in typescript today?

DickvdBrink · 2014-10-26T13:49:11Z

@CyrusNajmabadi Not real warnings but when would type this:

var foo: number = "test";

It would still create a file with output with valid JavaScript. I added this piece of code because the issue I linked to is having the label "Accepting PR".
DanQuirk is right about the EmitReturnStatus enum so this code might not be really needed but than I think that issue should be closed as well?

mhegazy · 2014-10-26T16:15:33Z

I think we need that for some scenarios. Incremental build with errors for instance does not work wiper simthing like this.

I do not like the name of the flag; users will have the same reaction as @CyrusNajmabadi. I can not find a good name though, may be suppressEmitWithErrors or early..

DanielRosenwasser · 2014-10-26T22:55:58Z

I'm also not big on the name, but I'm at a loss for significantly better ones.

emitOnCleanBuild
emitCleanBuildsOnly
noEmitOnAnyErrors

RyanCavanaugh · 2014-10-27T02:51:26Z

I like (or at least tolerate better) noEmitOnError[s] or emitCleanOnly. We're consistent in terminology about type errors being "errors" and not "warnings".

danquirk · 2014-10-27T21:52:28Z

yeah I think noEmitOnErrors is sufficiently clear and concise

mhegazy · 2014-10-27T22:38:22Z

src/compiler/diagnosticInformationMap.generated.ts

@@ -364,6 +364,7 @@ module ts {
        Specifies_the_location_where_debugger_should_locate_TypeScript_files_instead_of_source_locations: { code: 6004, category: DiagnosticCategory.Message, key: "Specifies the location where debugger should locate TypeScript files instead of source locations." },
        Watch_input_files: { code: 6005, category: DiagnosticCategory.Message, key: "Watch input files." },
        Redirect_output_structure_to_the_directory: { code: 6006, category: DiagnosticCategory.Message, key: "Redirect output structure to the directory." },
+        Do_not_emit_JavaScript_on_error: { code: 6007, category: DiagnosticCategory.Message, key: "Do not emit JavaScript on error." },


I would change this to: "Do not emit outputs if any type checking errors were reported"

“type checking” is a very compiler-team specific term. Really, all we’re trying to say is “Do not emit any outputs if any errors were reported.”

-- Cyrus

From: Mohamed Hegazy [mailto:notifications@github.com]
Sent: Monday, October 27, 2014 3:38 PM
To: Microsoft/TypeScript
Cc: Cyrus Najmabadi
Subject: Re: [TypeScript] implemented treat warning as errors commandline option. (#966)

In src/compiler/diagnosticInformationMap.generated.ts:

@@ -364,6 +364,7 @@ module ts {

Specifies_the_location_where_debugger_should_locate_TypeScript_files_instead_of_source_locations: { code: 6004, category: DiagnosticCategory.Message, key: "Specifies the location where debugger should locate TypeScript files instead of source locations." }, Watch_input_files: { code: 6005, category: DiagnosticCategory.Message, key: "Watch input files." }, Redirect_output_structure_to_the_directory: { code: 6006, category: DiagnosticCategory.Message, key: "Redirect output structure to the directory." },

Do_not_emit_JavaScript_on_error: { code: 6007, category: DiagnosticCategory.Message, key: "Do not emit JavaScript on error." },

I would change this to: "Do not emit outputs if any type checking errors were reported"

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/966/files#r19442813.

mhegazy · 2014-10-28T16:40:29Z

The logic on whether to emit or not is used in three places, 1. tsc.ts (batch compiler), and you got that, 2. emitter.ts (used by compile on save), and 3. harness.ts (used by the tests to mimic the batch compiler behavior). we need to ensure that all 3 do the same thing.
I would change the name of hasEarlyError to isEmitBlocked and add the check for the option and the error as well. i would also check global errors and syntax errors (program.getDiagnostics().length === 0) to be safe.

something like this:

function isEmitBlocked(sourceFile?: SourceFile): boolean {
    return program.getDiagnostics(sourceFile).length !== 0 ||
        hasEarlyErrors(sourceFile) ||
        (compilerOptions.noEmitOnError  && getDiagnostics(sourceFile));
}

Note: sourceFile is needed here, but it is optional. in case of batch compiler, sourceFile is not specified, as we want to do the check globally. but for compile on save, we want to do the check on one file only.

One more thing, we will need a unit test for this. Unit tests use reads "// @: value" from the beginning of the file, and uses them to set compilation settings. look at the switch statement in: https://github.com/Microsoft/TypeScript/blob/master/src/harness/harness.ts#L672

Then add a test that has "// @noEmitOnErrors: true" and ensure that there are no outputs. i would also add a @sourcemap and @declaration

DickvdBrink · 2014-10-28T21:16:39Z

Added some new commits.
Also renamed the hasEarlyErrors method in the interface to is isEmitBlocked (which is what you meant right? or does this break things with VS maybe?)
Changed some things in harness.ts to make it work.

For future reference the link to harness with SHA in the url harness.ts. (You can create this url by clicking on a line number and press 'y' and it will always be the right one)

DanielRosenwasser · 2014-10-29T19:30:20Z

src/harness/compilerRunner.ts

@@ -175,7 +175,11 @@ class CompilerBaselineRunner extends RunnerBase {
            it('Correct sourcemap content for ' + fileName, () => {
                if (options.sourceMap) {
                    Harness.Baseline.runBaseline('Correct sourcemap content for ' + fileName, justName.replace(/\.ts$/, '.sourcemap.txt'), () => {
-                        return result.getSourceMapRecord();
+                        var record = result.getSourceMapRecord();
+                        if (options.noEmitOnError && result.errors.length !== 0 && record === undefined) {


Is there a situation in which record can be undefined but the condition options.noEmitOnError && result.errors.length !== 0 evaluates to false? Perhaps a || is more appropriate?

Not sure if || is more appropriate here, when for example noEmitOnError is true and there are errors and record is not undefined (because getSourceMapRecord returned a sourcemap).. then it should actually try to compare them right?

I'm curious, do you actually get an empty sourcemap file if you don't perform this check?

Sorry, I'm personally not familiar enough with the harness code.

I believe it did, runBaseline calls generateActual: https://github.com/Microsoft/TypeScript/blob/5ce3baf339e0424263069c70f0ea239bc3cea96a/src/harness/harness.ts#L1307 which creates the file when it is something different than null. It throws an error when it is undefined.

yuit · 2014-10-29T19:47:43Z

Comments for isEarly and hasEarlyError will make it clearer.

DickvdBrink · 2014-10-29T22:01:59Z

Thanks for feedback everyone. Will add some comments in compilerRunner.ts
@yuit Comments for isEarly and hasEarlyError will make it clearer
Sorry, don't know what you mean here (where do you want such comments?)

yuit · 2014-10-29T23:52:46Z

For some reasons I can't see where you defined hasEarlyError in the change list. However, what I mean is that could you add comment at the when you declare function hasEarlyError and property isEarly in the Diagnostics. I am not sure what is the property is for

DickvdBrink · 2014-10-30T07:54:15Z

hasEarlyError was already there, the messages which have isEarly are in diagnosticMessages.json
I'm actually not really sure why those have isEarly so I can't really add a comment for it.

mhegazy · 2014-10-30T17:50:15Z

👍

mhegazy · 2014-11-16T18:28:45Z

@DickvdBrink extremely sorry for the delay. This totally fell off my radar. The PR is ready to go, can you refresh the PR by merging from main.

Conflicts: src/compiler/diagnosticInformationMap.generated.ts src/compiler/diagnosticMessages.json src/compiler/types.ts src/harness/harness.ts

DickvdBrink · 2014-11-16T21:13:11Z

@mhegazy no problem, better late than never right ;)
Updated PR and after the merge I ran the tests again and noticed that my baseline had changed so fixed that in another commit

implemented treat warning as errors commandline option.

mhegazy · 2014-11-17T01:42:43Z

thanks, and sorry again for the delay.

RyanCavanaugh reviewed Oct 26, 2014
View reviewed changes

implemented treat warning as errors commandline option (warnaserror).

417555c

DickvdBrink force-pushed the warnaserror branch from 5623f77 to 417555c Compare October 26, 2014 13:54

Changed name to noEmitOnError

bd2c596

mhegazy reviewed Oct 27, 2014
View reviewed changes

addressed feedback

e4f5756

DanielRosenwasser reviewed Oct 29, 2014
View reviewed changes

Added test for noEmitOnError

4c1397b

DickvdBrink force-pushed the warnaserror branch from e9df900 to 4c1397b Compare October 29, 2014 23:11

DickvdBrink force-pushed the warnaserror branch from 12eb5c0 to 4c1397b Compare November 16, 2014 20:33

DickvdBrink added 2 commits November 16, 2014 22:02

Merge branch 'master' into warnaserror

e7bb2e0

Conflicts: src/compiler/diagnosticInformationMap.generated.ts src/compiler/diagnosticMessages.json src/compiler/types.ts src/harness/harness.ts

Update baseline

c90fb4a

mhegazy added a commit that referenced this pull request Nov 17, 2014

Merge pull request #966 from DickvdBrink/warnaserror

d5cfceb

implemented treat warning as errors commandline option.

mhegazy merged commit d5cfceb into microsoft:master Nov 17, 2014

DickvdBrink deleted the warnaserror branch November 23, 2014 23:49

microsoft locked and limited conversation to collaborators Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented treat warning as errors commandline option. #966

implemented treat warning as errors commandline option. #966

DickvdBrink commented Oct 25, 2014

RyanCavanaugh Oct 26, 2014

CyrusNajmabadi commented Oct 26, 2014

DickvdBrink commented Oct 26, 2014

mhegazy commented Oct 26, 2014

DanielRosenwasser commented Oct 26, 2014

RyanCavanaugh commented Oct 27, 2014

danquirk commented Oct 27, 2014

mhegazy Oct 27, 2014

CyrusNajmabadi Oct 29, 2014

mhegazy commented Oct 28, 2014

DickvdBrink commented Oct 28, 2014

DanielRosenwasser Oct 29, 2014

DickvdBrink Oct 29, 2014

DanielRosenwasser Oct 30, 2014

DickvdBrink Oct 30, 2014

yuit commented Oct 29, 2014

DickvdBrink commented Oct 29, 2014

yuit commented Oct 29, 2014

DickvdBrink commented Oct 30, 2014

mhegazy commented Oct 30, 2014

mhegazy commented Nov 16, 2014

DickvdBrink commented Nov 16, 2014

mhegazy commented Nov 17, 2014

implemented treat warning as errors commandline option. #966

implemented treat warning as errors commandline option. #966

Conversation

DickvdBrink commented Oct 25, 2014

RyanCavanaugh Oct 26, 2014

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 26, 2014

DickvdBrink commented Oct 26, 2014

mhegazy commented Oct 26, 2014

DanielRosenwasser commented Oct 26, 2014

RyanCavanaugh commented Oct 27, 2014

danquirk commented Oct 27, 2014

mhegazy Oct 27, 2014

Choose a reason for hiding this comment

CyrusNajmabadi Oct 29, 2014

Choose a reason for hiding this comment

mhegazy commented Oct 28, 2014

DickvdBrink commented Oct 28, 2014

DanielRosenwasser Oct 29, 2014

Choose a reason for hiding this comment

DickvdBrink Oct 29, 2014

Choose a reason for hiding this comment

DanielRosenwasser Oct 30, 2014

Choose a reason for hiding this comment

DickvdBrink Oct 30, 2014

Choose a reason for hiding this comment

yuit commented Oct 29, 2014

DickvdBrink commented Oct 29, 2014

yuit commented Oct 29, 2014

DickvdBrink commented Oct 30, 2014

mhegazy commented Oct 30, 2014

mhegazy commented Nov 16, 2014

DickvdBrink commented Nov 16, 2014

mhegazy commented Nov 17, 2014