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

Rust: More information about extractor errors and warnings #17647

Merged
merged 16 commits into from
Oct 10, 2024

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Oct 2, 2024

Output more information about extractor errors and warnings:

  • add rust/diagnostics/extraction-warnings listing extractor warnings.
  • update rust/diagnostics/extraction-errors to output only errors, not errors and warnings.
    • note that at the moment the extractor doesn't currently have any path to output a Diagnostic that is not an warning. However that may change, and doing all this right ensures that code and DCA output are not confusing.
  • add extraction errors, warnings, and successfully / unsuccessfully extracted files to rust/summary/summary-statistics. This is helpful for examining databases manually.
  • define a class SuccessfullyExtractedFile to improve code re-use.

@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Oct 2, 2024
@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 3, 2024

@aibaars sync-identical-files is failing, it wants me to update the Ruby version of Diagnostics.qll. I think if I do that I may need to change some uses of ExtractionError as well, perhaps even give Ruby separate extraction errors / warnings queries. Or I can just turn the sync check off. How far do you (assuming you can speak for the Ruby team) think I should take this?

@aibaars
Copy link
Contributor

aibaars commented Oct 3, 2024

@aibaars sync-identical-files is failing, it wants me to update the Ruby version of Diagnostics.qll. I think if I do that I may need to change some uses of ExtractionError as well, perhaps even give Ruby separate extraction errors / warnings queries. Or I can just turn the sync check off. How far do you (assuming you can speak for the Ruby team) think I should take this?

The purpose of the sync check is to avoid having diverging implementations between languages. I think we should strive for consistency across languages. It's really annoying if all the diagnostics libraries have very similar but slightly differing behaviour.

I think your changes to Diagnostics.qll are fine, and could be applied to Ruby as well. Just make sure that behaviour doesn't change and add a change note if needed.

@geoffw0 geoffw0 added the Ruby label Oct 3, 2024
@geoffw0 geoffw0 requested a review from a team as a code owner October 3, 2024 16:40
@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 3, 2024

I've done the same changes for Ruby. Definitely deserves a review from the Ruby perspective (and I wouldn't be surprised if some consistency tests need updating when CI has run them all).

Comment on lines +14 to +19
from ExtractionWarning warning, File f
where
f = warning.getLocation().getFile() and
exists(f.getRelativePath())
select warning, "Extraction warning in " + f + " with message " + warning.getMessage(),
getSeverity()

Check warning

Code scanning / CodeQL

Consistent alert message Warning

The rb/diagnostics/extraction-warnings query does not have the same alert message as cpp, py.
@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 8, 2024

@github/codeql-ruby please take a look at the Ruby changes here (that mirror changes made in Rust). Thanks.

aibaars
aibaars previously approved these changes Oct 10, 2024
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me.

ruby/ql/lib/codeql/files/FileSystem.qll Outdated Show resolved Hide resolved
@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 10, 2024

Both Rust and Ruby changes have been approved (admittedly by the same person), CI is happy. Merging.

@geoffw0 geoffw0 merged commit 04c7319 into github:main Oct 10, 2024
29 checks passed
@geoffw0 geoffw0 deleted the warnings branch October 29, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Ruby Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants