Skip to content

Rust: limit number of diagnostics to 100 per trap file #19774

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Jun 16, 2025

This pull request limits the number of error/warning/info diagnostics we store in the CodeQL database to 100 per file. This should avoid SARIF from growing too large.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 16, 2025
@aibaars aibaars force-pushed the aibaars/limit-diagnostics branch from bd214b4 to c960af8 Compare June 16, 2025 10:11
@aibaars aibaars marked this pull request as ready for review June 18, 2025 08:03
@Copilot Copilot AI review requested due to automatic review settings June 18, 2025 08:03
@aibaars aibaars requested a review from a team as a code owner June 18, 2025 08:03
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 adds a hard cap of 100 diagnostics per trap file to prevent excessive emissions.

  • Introduced an emit_diagnostic_limit field on Translator and initialized it to 100.
  • Added EMIT_DIAGNOSTIC_LIMIT_PER_FILE constant and applied a check/decrement in the diagnostic emission path.
  • Kept existing behavior for Debug-level diagnostics (they are not counted against the limit).
Comments suppressed due to low confidence (2)

rust/extractor/src/translate/base.rs:126

  • Add a doc comment for the emit_diagnostic_limit field (and for EMIT_DIAGNOSTIC_LIMIT_PER_FILE) to explain its purpose and units, so future maintainers understand its role.
    emit_diagnostic_limit: usize,

rust/extractor/src/translate/base.rs:251

  • No tests were added to verify that diagnostics stop after the limit is reached; consider adding a unit or integration test that emits over 100 diagnostics and asserts that subsequent ones are suppressed.
        if severity > DiagnosticSeverity::Debug && self.emit_diagnostic_limit > 0 {

}

const UNKNOWN_LOCATION: (LineCol, LineCol) =
(LineCol { line: 0, col: 0 }, LineCol { line: 0, col: 0 });

const EMIT_DIAGNOSTIC_LIMIT_PER_FILE: usize = 100;
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider making the diagnostic limit configurable (e.g., via a constructor argument or config file) instead of a fixed constant, which would improve flexibility and allow per-project adjustments.

Copilot uses AI. Check for mistakes.

hvitved
hvitved previously approved these changes Jun 18, 2025
@aibaars aibaars force-pushed the aibaars/limit-diagnostics branch from 05ab438 to a253b31 Compare June 18, 2025 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants