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

Allows emitting buildInfo when --noEmit is specified #39122

Merged
merged 2 commits into from
Jun 18, 2020

Conversation

sheetalkamat
Copy link
Member

This change emits buildinfo even if noEmit is specified
Now referencing composite project with noEmit setting is error instead of erroring on project itself.
This allows to typecheck only run on composite projects just like any other project.
Also as part of this change, there were checker errors that were skipped if noEmit is on. Instead now those errors are generated but filtered when someone asks for semanticDiagnostics instead. This allows to preserver the diagnostics across emit and noEmit builds and having to invalidate cache/emit files/make extra pass on the code to get those diagnostics
Fixes #38440

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look reasonable, although I'm not an expert on how noEmit should interact with buildinfo.

@sheetalkamat sheetalkamat merged commit b977f86 into master Jun 18, 2020
@sheetalkamat sheetalkamat deleted the noEmitIncremental branch June 18, 2020 18:05
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I had some trouble understanding the implementation.

@@ -462,7 +462,6 @@ namespace ts {
{
name: "noEmit",
type: "boolean",
affectsEmit: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I find this surprising enough that I'd leave it present but commented out and with an explanation.

@@ -1013,6 +1013,12 @@ namespace ts {
}
}

function errorSkippedOn(key: keyof CompilerOptions, location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble wrapping my head around what this does and would find an explanation helpful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

location, message, arg0, ... are the existing error logging parameters.

key: keyof CompilerOptions is a typesafe way to assert that key, while a string, actually refers to a property in CompilerOptions. That way the string can safely be used as a property name of CompilerOptions in filterSemanticDiagnostics:

filter(diagnostics, d => !d.skippedOn || !options[d.skippedOn])

This keeps diagnostics that

  1. were not issued by errorSkippedOn, so didn't set skippedOn
    -OR-
  2. do have skippedOn, which is guaranteed to be the name of a property in compiler options, say "noEmit". For the "noEmit" example, !options[d.skippedOn] is equivalent to !options.noEmit.

This meta-programming JS would require an enum plus a switch-case conversion in C# I think. Or the meta-programming facilities there, though the result wouldn't be as typesafe as in Typescript.

@@ -3675,6 +3678,11 @@ namespace ts {
return { diagnostics, sourceMaps: undefined, emittedFiles, emitSkipped: true };
}

/*@internal*/
export function filterSemanticDiagnotics(diagnostic: readonly Diagnostic[], option: CompilerOptions): readonly Diagnostic[] {
Copy link
Member

@sandersn sandersn Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be named diagnostics and options

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a typo in the function name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants