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

refactor(toml): Flatten manifest parsing #13589

Merged
merged 19 commits into from
Mar 15, 2024
Merged

refactor(toml): Flatten manifest parsing #13589

merged 19 commits into from
Mar 15, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Mar 15, 2024

What does this PR try to resolve?

This is just a clean up but the goals are

  • Support diagnostics that show source by tracking &str, ImDocument, etc in Manifest by making each accessible in the creation of a Manifest
  • Defer warning analysis until we know what is a local vs non-local workspace by refactoring warnings out into a dedicated step
  • Centralize the logic for cargo publish stripping of dev-dependencies and their feature activations by allowing a Summary to be created from any "resolved" TomlManifest
  • Enumerate all build targets in the "resolved" TomlManifest so they get included in cargo publish, reducing the work done on registry dependencies and resolving problems like cargo publish has bad error message when explicit [[bench]] isn't in package.include #13456

Along the way, this fixed a bug where we were not reporting warnings from virtual manifests

How should we test and review this PR?

Additional information

This is prep for adding more
@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2024

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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces Command-new Command-package Command-publish Command-read-manifest Command-remove Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@rustbot rustbot added the A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. label Mar 15, 2024
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

The cleanup looks nice!

src/cargo/core/manifest.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
crates/cargo-util-schemas/src/manifest/mod.rs Show resolved Hide resolved
src/cargo/util/toml/mod.rs Show resolved Hide resolved
epage added 11 commits March 15, 2024 12:00
I plan to add a true `original`
So diagnostics can use this for reporting.
- So we can eventually track the `original`
- So we can track spans for diagnostics
So we can track spans for diagnostics
Before, we split things up.  This makes it so everything has access to
every step so we can reap the benefits
- use `&str` and `ImDocument` for diagnostics
- access `original`
Moving this out for collapsing `convert_toml`
This is prep for shifting unused keys warnings out of `convert_toml`
This makes it act more like everything else, making this easier to
evolve over time.
This is in an effort to remove `convert_toml`
This is part of an effort to remove `convert_toml`
This simplifies the interface for `toml/mod.rs` and reduces the work we
do.
This is to work towards tracking everything needed for diagnostics in
`Manifest`
@Muscraft
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2024

📌 Commit 4ed67e3 has been approved by Muscraft

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@bors
Copy link
Contributor

bors commented Mar 15, 2024

⌛ Testing commit 4ed67e3 with merge 48fb957...

@bors
Copy link
Contributor

bors commented Mar 15, 2024

☀️ Test successful - checks-actions
Approved by: Muscraft
Pushing 48fb957 to master...

@bors bors merged commit 48fb957 into rust-lang:master Mar 15, 2024
23 checks passed
@epage epage deleted the toml branch March 15, 2024 20:43
bors added a commit that referenced this pull request Mar 15, 2024
refactor: Expose source/spans to Manifest for emitting lints

### What does this PR try to resolve?

This is a follow up to #13589.

This does nothing on its own.

This is meant to short-circuit some of my refactorings so Muscraft can
start on their work on adding lints while I work to move out existing
warnings into being able to be converted to lints.

### How should we test and review this PR?

This includes documentation changes suggested in #13589

### Additional information
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2024
Update cargo

6 commits in 7065f0ef4aa267a7455e1c478b5ccacb7baea59c..2fe739fcf16c5bf8c2064ab9d357f4a0e6c8539b
2024-03-12 13:25:15 +0000 to 2024-03-15 21:39:18 +0000
- feat: Add 'open-namespaces' feature (rust-lang/cargo#13591)
- refactor: Expose source/spans to Manifest for emitting lints (rust-lang/cargo#13593)
- feat(tree): Control `--charset` via auto-detecting config value (rust-lang/cargo#13337)
- refactor(toml): Flatten manifest parsing (rust-lang/cargo#13589)
- fix: strip feature dep when dep is dev dep (rust-lang/cargo#13518)
- fix(ci): bump check error when PR is behind master (rust-lang/cargo#13581)

r? ghost
@rustbot rustbot added this to the 1.78.0 milestone Mar 16, 2024
bors added a commit that referenced this pull request Mar 28, 2024
fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces

### What does this PR try to resolve?

This splits out refactors that build on #13589 in preparation for resolving #13456.

As part of those refactors, I noticed an inconsistency on when we warn for unused keys.  We have parallel code paths between `to_virtual_manifest` and `to_real_manifest` and only one got updated on a change.  This syncs them up.  Hopefully the end state this builds to will reduce duplication.

### How should we test and review this PR?

### Additional information
bors added a commit that referenced this pull request Mar 28, 2024
fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces

### What does this PR try to resolve?

This splits out refactors that build on #13589 in preparation for resolving #13456.

As part of those refactors, I noticed an inconsistency on when we warn for unused keys.  We have parallel code paths between `to_virtual_manifest` and `to_real_manifest` and only one got updated on a change.  This syncs them up.  Hopefully the end state this builds to will reduce duplication.

### How should we test and review this PR?

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces Command-new Command-package Command-publish Command-read-manifest Command-remove Command-vendor S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants