Skip to content

Conversation

sheetalkamat
Copy link
Member

#1628 started adding program diagnostics to semantic diagnostic reporting like Strada
This handles incremental scenarios like Strada where we don't store program diagnostics to file because we don't know if they change or not if program structure changes and semantic diagnostics from cache are used

@sheetalkamat sheetalkamat requested review from Copilot, gabritto and jakebailey and removed request for jakebailey August 28, 2025 21:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses incremental build scenarios by preventing include processing diagnostics from being stored in buildInfo files. The change ensures that program diagnostics are handled like Strada's approach, where semantic diagnostics from cache are used without storing uncertain program diagnostics to disk.

Key changes:

  • Moved include processor diagnostics from being stored in buildInfo to being added dynamically during semantic diagnostic retrieval
  • Added error tracking for include processing diagnostics to maintain proper build state
  • Updated test baselines to reflect the removal of semanticDiagnosticsPerFile from buildInfo files

Reviewed Changes

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

File Description
internal/execute/incremental/program.go Refactored semantic diagnostics handling to add include processor diagnostics dynamically and track errors properly
internal/compiler/program.go Added new method for include processor diagnostics and integrated them into semantic diagnostics flow
testdata/baselines/reference/* Updated baseline files showing removal of stored semantic diagnostics from buildInfo

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sheetalkamat sheetalkamat added this pull request to the merge queue Aug 29, 2025
Merged via the queue into main with commit 5d1d69a Aug 29, 2025
22 checks passed
@sheetalkamat sheetalkamat deleted the semanticDiag branch August 29, 2025 05:13
@gabritto
Copy link
Member

I thought I had left a review yesterday but apparently I forgot to submit it, sorry. But one thing I think may be missing is that getSemanticDiagnostics also filters out diagnostics according to // @ts-ignore, and in Strada that included the processing diagnostics, whereas I think in this PR it now doesn't.

@jakebailey
Copy link
Member

I actually thought that this change preserved that, given it doesn't seem like a related test changed, but maybe I've misread the PR?

@gabritto
Copy link
Member

I actually thought that this change preserved that, given it doesn't seem like a related test changed, but maybe I've misread the PR?

Do we have a test for this?

@jakebailey
Copy link
Member

Maybe I'm hallucinating that your PR had one 😅

@gabritto
Copy link
Member

Maybe I'm hallucinating that your PR had one 😅

No 🙁

@sheetalkamat
Copy link
Member Author

Sorry that i missed this part when porting this. #1649 adds the filtering

zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
zshannon pushed a commit to zshannon/typescript-go that referenced this pull request Oct 6, 2025
)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants