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

Support multi-line inline tables #11500

Open
poscat0x04 opened this issue Dec 19, 2022 · 8 comments
Open

Support multi-line inline tables #11500

poscat0x04 opened this issue Dec 19, 2022 · 8 comments
Labels
A-toml Area: TOML parsing and handling C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix

Comments

@poscat0x04
Copy link

Problem

Currently cargo won't parse a cargo.toml file that contains multi-line inline tables.

Proposed Solution

Change the toml parser to allow multi-line inline tables

Notes

No response

@poscat0x04 poscat0x04 added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Dec 19, 2022
@weihanglo weihanglo added S-blocked A-toml Area: TOML parsing and handling S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix and removed S-blocked labels Dec 19, 2022
@weihanglo
Copy link
Member

Note that “newlines and trailing commas in inline tables” is still unreleased.

https://github.com/toml-lang/toml/blob/913ff560d935e7fb2788b402abba567680f6e03c/CHANGELOG.md

@epage
Copy link
Contributor

epage commented Dec 19, 2022

Yes, we currently support TOML 1.0.

@epage
Copy link
Contributor

epage commented Dec 19, 2022

Upstream issue toml-rs/toml#397

A downside to the new TOML version is it could cause a 10% slow down in TOML parsing without a lot of optimization work as it adds support for unicode bare keys. Unless we implement a UTF-8 state machine in our parser (and deal with the risks associated with it), it'll require us to move from &[u8] parsing to &str parsing.

@poscat0x04
Copy link
Author

Ah ok. Didn't know that toml has a release cycle.

A downside to the new TOML version is it could cause a 10% slow down in TOML parsing without a lot of optimization work as it adds support for unicode bare keys. Unless we implement a UTF-8 state machine in our parser (and deal with the risks associated with it), it'll require us to move from &[u8] parsing to &str parsing.

Parsing toml shouldn't be a performance bottleneck, right? IMO a 10% slowdown is acceptable.

@weihanglo
Copy link
Member

Parsing toml shouldn't be a performance bottleneck, right? IMO a 10% slowdown is acceptable.

Parse does contribute to total build time to some extent. Cargo parses Cargo.toml for each dependency, as well as configuration files and Cargo.lock. Back in the day we perceived a performance loss when switching from toml-rs to toml_edit. Anyhow, let's keep an eye on it :)

@epage
Copy link
Contributor

epage commented Dec 20, 2022

The biggest concern for performance is parsing someone's dependency tree. We do it on every run and, for large programs, can involve parsing hundreds of toml files. Toml is designed for humans and isn't the most optimal format to parse already.

One potential area of improvement is if we track immutable manifests and cache those in a fast to parse format. The biggest gains for immutable manifests would be from the registry. A manifest for a given git dependency's SHA would also be immutable but I wouldn't expect there to be too many of those to be worth it.

@pronebird
Copy link

Can we please fix this, it's really surprising that this is even an issue for a text parser.

@epage
Copy link
Contributor

epage commented Apr 19, 2024

TOML 1.1 spec is not released yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-toml Area: TOML parsing and handling C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix
Projects
None yet
Development

No branches or pull requests

4 participants