fix: improve message for inactive weak optional feature with edition2024 through unused dep collection

[linyihai]

What does this PR try to resolve?

Collect the unused dependencies to check whether a weak optional dependency had set. Then we can improve the message when weak optional dependency inactive.

Fixes #14015

How should we test and review this PR?

One commit test added, one commit fixed and updated

Additional information

Part of #14039

• migrate tests/testsuite/lints/unused_optional_dependencies.rs to snapshot

And rename MissingField to MissingFieldError

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[linyihai]

r? @epage

I'm sorry split the fix into two. If #14019 more reasonable and finally merge, this PR can be closed.

[epage]

@Muscraft would be interested in your thoughts on this approach to solving #14015.

#14019 is an attempt but it lacks the information for a clear message, so this tries to track the additional information to make that happen.

At least one potential ramification of this is we could simplified unused optional dependency lint to just checking the unused_dependencies tables

I do think #14028 should be merged first so we make sure we don't have regressions.

[epage]

(also I should note that I've only looked at the high level details and not all of the nitty gritty of the implementation yet)

[bors]

☔ The latest upstream changes (presumably #14028) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #14106) made this pull request unmergeable. Please resolve the merge conflicts.

[linyihai]

Try to minimize code changes and performance impact, which will return a custom Error MissingDependency when Summary::new? found a dependency missing. And then handle the MissingDependency in to_real_manifest.

[linyihai]

The suggestions above have been considered.

Then in the subsequent commit, add these alternation by the way.

• Rename MissingField to MissingFieldError
• Migrate unused_optional_dependencies to snapshot
  The changes are small and easy to review :)

[weihanglo]

Have talked to @Muscraft and we think the current mechanism of this is fine!

In the future, we might want to make a dedicated Error type (probably an enum) for Summary::new() through thiserror crate, so we don't need to downcast them anymore. The Error type could additionally be handled in cargo::util::toml with original manifest TOML to get an enhanced error with richer information.

The future version would be something like this:

fn error_with_original_toml(err: &SummaryError, original_toml: &TomlManifest) -> CargoResult<()> {
    match err {
        SmmmaryError::Foo => return Err(...),
        SmmmaryError::BarError => return Err(...),
        SummaryError::MissingDependency(missing_dep) => {
		    let message = "...";
            if let Err(err) = gctx.shell().print_message(message) {
            	return Err(err.into());
            }
            return Err(AlreadyPrintedError::new(anyhow!("").into()).into());
        }
    }
}

which might be rendered to something like:

or

Credit to @Muscraft doing this experiment. See Muscraft@c0b44e0 for the code that generate the snapshots above.

[weihanglo]

To me, it is a bit odd to mutate an error. Could we extract this login to a function, which accepts the original_toml and the err as arguments and returns a refined error?

By doing that, MissingDependencyError doesn't need the field unused_dependency. We could do the logic complete outside core::summary.Summary is used by both index file parsing and manifest parsing, but the missing dep feature error doesn't make sense for index parsing at this moment. Therefore, MissingDependencyError will emit only one kind of error message.

Also, we could make all fields pub because the error is not intended to be modified.

[linyihai]

Thanks for your suggestions.

All have been rebased into fix: improve message for inactive weak optional feature with edition2024

[linyihai]

And the last commit has added the missing_dep_diagnostic, but I shamless copy the code, may be it can be improved in latter PR.

[linyihai]

Credit to @Muscraft doing this experiment. See Muscraft@c0b44e0 for the code that generate the snapshots above.

I have added the missing_dep_diagnostic into the last commit with tiny twist, thanks @Muscraft.

These tips are more user-friendly.

[weihanglo]

Thanks! That looks awesome. Will go ahead and merge this.

@bors r+

[bors]

📌 Commit b28eef9 has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit b28eef9 with merge 838d81d...

[bors]

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