Remove GetOptionsDiagnostics

[jakebailey]

"Options diagnostics" were just config file diagnostics + global diagnostics.

This was odd, because we then took that in places and combined it with... config file diags and global diags. And then not deduping it, sometimes.

This PR just removes GetOptionsDiagnostics altogether, instead leaving behind just GetConfigFileParsingDiagnostics and GetGlobalDiagnostics.

The language server still does not ever call GetGlobalDiagnostics. I am not sure where such a thing would go, since global diagnostics are generated as a side effect of doing checking (e.g., when we figure out that AsyncIterable was never declared or something).

There's a baseline change, but I think it's correct and positive.

[Copilot]

Pull request overview

This PR removes the GetOptionsDiagnostics method, which redundantly combined config file diagnostics and global diagnostics. The code now directly uses GetConfigFileParsingDiagnostics and GetGlobalDiagnostics instead, eliminating unnecessary duplication and improving diagnostic handling clarity.

Key changes:

• Removed GetOptionsDiagnostics method and its helper from the Program struct
• Updated diagnostic collection logic to call GetConfigFileParsingDiagnostics and GetGlobalDiagnostics directly
• Added a safety check in GetGlobalDiagnostics to handle editor scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
testdata/baselines/reference/tsbuildWatch/configFileErrors/reports-syntax-errors-in-config-file.js	Updated test baseline to reflect removal of semanticDiagnosticsPerFile field and changes to diagnostic caching behavior
testdata/baselines/reference/tsbuild/configFileErrors/reports-syntax-errors-in-config-file.js	Updated test baseline with same changes as watch variant
internal/execute/incremental/program.go	Removed GetOptionsDiagnostics method wrapper and its usage in error state checking
internal/compiler/program.go	Removed GetOptionsDiagnostics and helper methods, added type assertion safety check, updated GetDiagnosticsOfAnyProgram to call diagnostics methods directly

[jakebailey]

Whenever we get around to revamping how the checkers are allocated, I think the fix is going to be to (in the editor) collect global diags from diagnostic-producing checkers, and then re-collect them whenever we check a new file, then expose those diags there.

[jakebailey]

This used to be here because we could fail to get file parsing diags. But then we made ParsedCommandLine mandatory, so this call always succeeds, so there's no reason to special case anything.

[jakebailey]

Note above how we get config file diags, then get options diags, and then get global diags. This means we could have the same diags repeated twice!

(This func does not sort and dedupe, for some reason)

[jakebailey]

This list already contains GetConfigFileParsingDiagnostics and GetGlobalDiagnostics, so this is a noop.

[sheetalkamat]

Strada use to add them to semantic Diagnostics - this ensured that all callers of this API didnt have to worry about it?

[jakebailey]

We can do that, if and only if the caller passes in a nil file, theoretically.

[jakebailey]

Pushed a change which I think works to delete GetGlobalDiagnostics too.

[sheetalkamat]

well - strada did the exact opposite. It added to "per file semanticDiagnostics" because thats how editor gets diagnostics and it shouldnt miss them?

function getDiagnosticsWorker(sourceFile: SourceFile, nodesToCheck: Node[] | undefined): Diagnostic[] {
        if (sourceFile) {
            ensurePendingDiagnosticWorkComplete();
            // Some global diagnostics are deferred until they are needed and
            // may not be reported in the first call to getGlobalDiagnostics.
            // We should catch these changes and report them.
            const previousGlobalDiagnostics = diagnostics.getGlobalDiagnostics();
            const previousGlobalDiagnosticsSize = previousGlobalDiagnostics.length;

            checkSourceFileWithEagerDiagnostics(sourceFile, nodesToCheck);
            const semanticDiagnostics = diagnostics.getDiagnostics(sourceFile.fileName);
            if (nodesToCheck) {
                // No need to get global diagnostics.
                return semanticDiagnostics;
            }
            const currentGlobalDiagnostics = diagnostics.getGlobalDiagnostics();
            if (currentGlobalDiagnostics !== previousGlobalDiagnostics) {
                // If the arrays are not the same reference, new diagnostics were added.
                const deferredGlobalDiagnostics = relativeComplement(previousGlobalDiagnostics, currentGlobalDiagnostics, compareDiagnostics);
                return concatenate(deferredGlobalDiagnostics, semanticDiagnostics);
            }
            else if (previousGlobalDiagnosticsSize === 0 && currentGlobalDiagnostics.length > 0) {
                // If the arrays are the same reference, but the length has changed, a single
                // new diagnostic was added as DiagnosticCollection attempts to reuse the
                // same array.
                return concatenate(currentGlobalDiagnostics, semanticDiagnostics);
            }

            return semanticDiagnostics;
        }

        // Global diagnostics are always added when a file is not provided to
        // getDiagnostics
        forEach(host.getSourceFiles(), file => checkSourceFileWithEagerDiagnostics(file));
        return diagnostics.getDiagnostics();
    }

[sheetalkamat]

Not suire if we have enough coverage for diagnostics in fourslash but all this logic was for editor to not miss reporting errors as for command line you can always get all semantic diagnostics and then global diagnsotics?

[jakebailey]

Not suire if we have enough coverage for diagnostics in fourslash but all this logic was for editor to not miss reporting errors as for command line you can always get all semantic diagnostics and then global diagnsotics?

The current LS can miss these global diagnostics, yes. Though, that isn't new...

[jakebailey]

The last commit I did was bad, though, because it locks off ever asking for global diags in the LS where we never ask for all files, so I'll undo that

[sheetalkamat]

Not suire if we have enough coverage for diagnostics in fourslash but all this logic was for editor to not miss reporting errors as for command line you can always get all semantic diagnostics and then global diagnsotics?

The current LS can miss these global diagnostics, yes. Though, that isn't new...

It is new right, given we use to add the new diagnostics to semantic diagnostics and send it back to client ?

getBindAndCheckDiagnosticsForFile

It shouldnt be part of this as these are the ones that are cached in buildInfo and need to be consistent

[jakebailey]

I'm confused enough with how to make this work that I wish we just didn't have global diags at all, they just really do depend on whether or not a file was checked and in which order, which does not feel reasonable to manage?

[sheetalkamat]

Havent checked how diagnostics are handled in corsa but with push diagnostics we can control the order but not sure how to handle that as part of pull diagnostics esp since global diagnostics are checker diagnostics without any file info. (may be we just need to make sure that checker cannot generate global diagnostics) ?