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

Comments between lines in a table are duplicated #673

Closed
kpreid opened this issue Feb 4, 2024 · 4 comments · Fixed by #675
Closed

Comments between lines in a table are duplicated #673

kpreid opened this issue Feb 4, 2024 · 4 comments · Fixed by #675
Labels
A-output Area: Outputting TOML C-bug Category: Things not working as expected

Comments

@kpreid
Copy link

kpreid commented Feb 4, 2024

Repro that just round-trips some toml:

[dependencies]
pretty_assertions = "=1.3.0"
toml_edit = "=0.21.1"
#[test]
fn toml_edit_bug() {
    use pretty_assertions::assert_eq;

    let text = r###"
[workspace.lints]
rust.unsafe_op_in_unsafe_fn = "deny"

rust.explicit_outlives_requirements = "warn"
# rust.unused_crate_dependencies = "warn"

clippy.cast_lossless = "warn"
clippy.doc_markdown = "warn"
clippy.exhaustive_enums = "warn"
"###;

    let manifest: toml_edit::Document = text.parse().unwrap();

    assert_eq!(text, manifest.to_string());
}

The text should be identical, but it is not. Specifically, the whitespace between the first two items is deleted, and the comment and whitespace are duplicated between all following pairs of items.

 [workspace.lints]
 rust.unsafe_op_in_unsafe_fn = "deny"
-
 rust.explicit_outlives_requirements = "warn"
 # rust.unused_crate_dependencies = "warn"
 
 clippy.cast_lossless = "warn"
+# rust.unused_crate_dependencies = "warn"
+
 clippy.doc_markdown = "warn"
+# rust.unused_crate_dependencies = "warn"
+
 clippy.exhaustive_enums = "warn"

The same behavior happens with toml_edit 0.19.15, so this is not a new bug.

@epage epage added C-bug Category: Things not working as expected A-output Area: Outputting TOML labels Feb 5, 2024
@epage
Copy link
Member

epage commented Feb 5, 2024

The core of this problem is that we don't track these dotted keys independently but treat them as one table. Another manifestation of this is #163.

Looking at a dump of the document, the comment is part of the "prefix" of the clippy table, so every time that table shows up, we show the "prefix".

let manifest = Document {
    root: Table(
        Table {
            decor: Decor {
                prefix: "default",
                suffix: "default",
            },
            implicit: false,
            dotted: false,
            doc_position: None,
            span: None,
            items: {
                "rust": TableKeyValue {
                    key: Key {
                        key: "rust",
                        repr: Some(
                            "rust",
                        ),
                        decor: Decor {
                            prefix: "\n",
                            suffix: empty,
                        },
                    },
                    value: Table(
                        Table {
                            decor: Decor {
                                prefix: "default",
                                suffix: "default",
                            },
                            implicit: true,
                            dotted: true,
                            doc_position: None,
                            span: None,
                            items: {
                                "unsafe_op_in_unsafe_fn": TableKeyValue {
                                    key: Key {
                                        key: "unsafe_op_in_unsafe_fn",
                                        repr: Some(
                                            "unsafe_op_in_unsafe_fn",
                                        ),
                                        decor: Decor {
                                            prefix: empty,
                                            suffix: " ",
                                        },
                                    },
                                    value: Value(
                                        String(
                                            Formatted {
                                                value: "deny",
                                                repr: "\"deny\"",
                                                decor: Decor {
                                                    prefix: " ",
                                                    suffix: empty,
                                                },
                                            },
                                        ),
                                    ),
                                },
                                "explicit_outlives_requirements": TableKeyValue {
                                    key: Key {
                                        key: "explicit_outlives_requirements",
                                        repr: Some(
                                            "explicit_outlives_requirements",
                                        ),
                                        decor: Decor {
                                            prefix: empty,
                                            suffix: " ",
                                        },
                                    },
                                    value: Value(
                                        String(
                                            Formatted {
                                                value: "warn",
                                                repr: "\"warn\"",
                                                decor: Decor {
                                                    prefix: " ",
                                                    suffix: empty,
                                                },
                                            },
                                        ),
                                    ),
                                },
                            },
                        },
                    ),
                },
                "clippy": TableKeyValue {
                    key: Key {
                        key: "clippy",
                        repr: Some(
                            "clippy",
                        ),
                        decor: Decor {
                            prefix: "# rust.unused_crate_dependencies = \"warn\"\n\n",
                            suffix: empty,
                        },
                    },
                    value: Table(
                        Table {
                            decor: Decor {
                                prefix: "default",
                                suffix: "default",
                            },
                            implicit: true,
                            dotted: true,
                            doc_position: None,
                            span: None,
                            items: {
                                "cast_lossless": TableKeyValue {
                                    key: Key {
                                        key: "cast_lossless",
                                        repr: Some(
                                            "cast_lossless",
                                        ),
                                        decor: Decor {
                                            prefix: empty,
                                            suffix: " ",
                                        },
                                    },
                                    value: Value(
                                        String(
                                            Formatted {
                                                value: "warn",
                                                repr: "\"warn\"",
                                                decor: Decor {
                                                    prefix: " ",
                                                    suffix: empty,
                                                },
                                            },
                                        ),
                                    ),
                                },
                                "doc_markdown": TableKeyValue {
                                    key: Key {
                                        key: "doc_markdown",
                                        repr: Some(
                                            "doc_markdown",
                                        ),
                                        decor: Decor {
                                            prefix: empty,
                                            suffix: " ",
                                        },
                                    },
                                    value: Value(
                                        String(
                                            Formatted {
                                                value: "warn",
                                                repr: "\"warn\"",
                                                decor: Decor {
                                                    prefix: " ",
                                                    suffix: empty,
                                                },
                                            },
                                        ),
                                    ),
                                },
                                "exhaustive_enums": TableKeyValue {
                                    key: Key {
                                        key: "exhaustive_enums",
                                        repr: Some(
                                            "exhaustive_enums",
                                        ),
                                        decor: Decor {
                                            prefix: empty,
                                            suffix: " ",
                                        },
                                    },
                                    value: Value(
                                        String(
                                            Formatted {
                                                value: "warn",
                                                repr: "\"warn\"",
                                                decor: Decor {
                                                    prefix: " ",
                                                    suffix: empty,
                                                },
                                            },
                                        ),
                                    ),
                                },
                            },
                        },
                    ),
                },
            },
        },
    ),
    trailing: empty,
    original: Some(
        "\nrust.unsafe_op_in_unsafe_fn = \"deny\"\n\nrust.explicit_outlives_requirements = \"warn\"\n# rust.unused_crate_dependencies = \"warn\"\n\nclippy.cast_lossless = \"warn\"\nclippy.doc_markdown = \"warn\"\nclippy.exhaustive_enums = \"warn\"\n",
    ),
    span: None,
}

epage added a commit to epage/toml_edit that referenced this issue Feb 5, 2024
We tracked one decor per key.
When dealing with dotted keys,
we used the decor prefix from the first key.
The problem is that we then re-encode that key for each dotted key,
duplicating any comments or whitespace associated with it.

We solved this by tracking decor between dotted keys separate from the
decor used per final, or leaf, entry.

Fixes toml-rs#673
@epage epage closed this as completed in #675 Feb 5, 2024
@epage
Copy link
Member

epage commented Feb 5, 2024

Opened rust-lang/cargo#13402 to make sure cargo gets the fixed behavior

@kpreid
Copy link
Author

kpreid commented Feb 5, 2024

we don't track these dotted keys independently

Aha, and now I understand why the bug wasn't already noticed — dotted keys are less common.

@epage
Copy link
Member

epage commented Feb 5, 2024

Hadn't considered using dotted keys for lints but I like it!

bors added a commit to rust-lang/cargo that referenced this issue Feb 7, 2024
fix: Don't duplicate comments when editing TOML

### What does this PR try to resolve?

`toml_edit` <0.22 has a bug that will cause
```toml
[lints]
rust.unsafe_op_in_unsafe_fn = "deny"

rust.explicit_outlives_requirements = "warn"
# rust.unused_crate_dependencies = "warn"

clippy.cast_lossless = "warn"
clippy.doc_markdown = "warn"
clippy.exhaustive_enums = "warn"
```
to be written out as
```toml
[lints]
rust.unsafe_op_in_unsafe_fn = "deny"
rust.explicit_outlives_requirements = "warn"
# rust.unused_crate_dependencies = "warn"

clippy.cast_lossless = "warn"
# rust.unused_crate_dependencies = "warn"

clippy.doc_markdown = "warn"
# rust.unused_crate_dependencies = "warn"

clippy.exhaustive_enums = "warn"
```
when it is parsed and then saved.

See toml-rs/toml#673

This affects any format-preserving edits we do, including:
- `cargo add`
- `cargo remove`
- `cargo init` / `cargo new` editing the workspace

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

I didn't add any tests as this is covered by `toml_edit`s tests (we already don't cover a fraction of the edit preserving tests it has)

### Additional information
kpreid added a commit to kpreid/all-is-cubes that referenced this issue Aug 16, 2024
Dependabot’s parser currently doesn't like this for some reason
(<dependabot/dependabot-core#10453>),
and while that’s certainly a bug, I’d like to get dependency updates
working again, and this is in fact the second time I’ve found a bug
in tools’ handling of dotted keys (the first time was
<toml-rs/toml#673>), so it is pragmatic to
not do the thing that often doesn't work. For now, at least.

(The reasons I prefer the dotted keys are because one doesn’t have to
scan upward to the most recent table header to see whether the lint is
`rust` or `clippy`, and it’s more like `#[warn()]` syntax.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-output Area: Outputting TOML C-bug Category: Things not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants