fix: correct error count for cargo check --message-format json

[yichi170]

Hi! This is my first time contributing to Cargo. If there is anything that I need to do, please let me know!
(I'm not sure whether the commit message is aligned with the Cargo's convention. If it doesn't, I'm willing to modify it!)

This PR resolves the issue with incorrect error count and ensures warnings are correctly displayed when using cargo check --message-format json.

Fixes #14472

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[epage]

Hi! This is my first time contributing to Cargo. If there is anything that I need to do, please let me know!

Welcome!

If you ever want some face-time with a Cargo team member, we hold office hours on jitsi, see https://github.com/rust-lang/cargo/wiki/Office-Hours

It would be great if this change could have a test! Looking around, my guess is one of the following files will have relevant tests to put this next to: tests/testsuite/check.rs, tests/testsuite/message_format.rs, or tests/testsuite/messages.rs.

Something we've found very helpful when writing tests is to add them in a commit before a fix, with every commit passing the test. See https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request for more details.

(I'm not sure whether the commit message is aligned with the Cargo's convention. If it doesn't, I'm willing to modify it!)

We don't have an official style though some of us use Conventional Commits

[yichi170]

Thanks for providing the information! I would add the test for this modification.

[yichi170]

I think the original test already cover this PR, instead of creating one, I modify the test to make it produce the correct result.
If there is anything that I can do, please let me know!

[weihanglo]

Yeah existing tests seem to cover the case.

[weihanglo]

Instead of reusing a larger struct definition, should we just add an extra field message, and change count_diagnostic to accept three args (level, message, options) ?

Although having no data, my intuition tells me that skipping unused fields is generally more performant.

{
  "rendered": "error: expected one of `!` or `::`, found `<eof>`\n --> src/main.rs:3:1\n  |\n3 | xxx\n  | ^^^ expected one of `!` or `::`\n\n",
  "children": [], // may contain nested diagnostics
  "level": "error",
  "message": "expected one of `!` or `::`, found `<eof>`",
}

compares to

{
  "level": "error",
  "message": "expected one of `!` or `::`, found `<eof>`",
}

[yichi170]

should we just add an extra field message, and change count_diagnostic to accept three args (level, message, options) ?

Do you mean we should check the message in count_diagnostic instead of checking before passing the (level, options) to count_diagnostic?

[weihanglo]

Ignore the count_diagnostic part. I have no idea what I was talking about.

I meant that we may want to keep a separate CompilerMessage struct definition, since the current patch deserializes more fields than needed.

[yichi170]

fixed!

[weihanglo]

Awesome! Thanks for helping.

@bors r+

[bors]

📌 Commit 71c830c has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 71c830c with merge a3b35a0...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a3b35a0 to master...