toml-rs / toml

Rust TOML Parser
https://docs.rs/toml
Apache License 2.0
730 stars 107 forks source link

Whitespace change after editing when updating from `0.22.20` to `0.22.21` #787

Closed torokati44 closed 1 month ago

torokati44 commented 1 month ago

In our regular weekly dependency bump (https://github.com/ruffle-rs/ruffle/pull/18048), we have a failing test, here: https://github.com/ruffle-rs/ruffle/blob/0a9d2bae1e9abab279fe8d20e13b2ee8e19a5bc2/frontend-utils/src/bookmarks/write.rs#L118-L142

The failure is:

thread 'bookmarks::write::tests::overwrite_invalid_bookmark_type' panicked at frontend-utils/src/bookmarks/write.rs:67:5:
assertion `left == right` failed
  left: "[[bookmark]]\nurl = \"file:///test.swf\"\nname = \"test.swf\"\n"
 right: "[[bookmark ]]\nurl = \"file:///test.swf\"\nname = \"test.swf\"\n"

Could you please advise on whether this is on us to fix, it's inconsequential because it doesn't matter for the toml syntax, or perhaps a bug in toml_edit?

epage commented 1 month ago

Nothing in the changelog indicates any intentional whitespace changes.

Can you create an independent reproduction case?

torokati44 commented 1 month ago

Sure, here you go:

use toml_edit::{DocumentMut, Table};

fn main() {
    let mut document = "bookmark = 1010".parse::<DocumentMut>().unwrap();

    let key: &str = "bookmark";

    document.insert(key, toml_edit::array());
    let table = document[key].as_array_of_tables_mut().unwrap();

    let mut bookmark_table = Table::new();
    bookmark_table["name"] = toml_edit::value("test.swf".to_string());
    table.push(bookmark_table);

    let expected: &str = "[[bookmark]]\nname = \"test.swf\"\n";
    assert_eq!(expected, document.to_string());
}

This passes with 0.22.20, but fails with 0.22.21:

thread 'main' panicked at src/main.rs:16:5:
assertion `left == right` failed
  left: "[[bookmark]]\nname = \"test.swf\"\n"
 right: "[[bookmark ]]\nname = \"test.swf\"\n"
epage commented 1 month ago

It appears the root cause was 189697d16c0c4cf047e35ce047bbce923844e133.

epage commented 1 month ago

The exact change that caused this is at https://github.com/toml-rs/toml/commit/189697d16c0c4cf047e35ce047bbce923844e133#r147105791

This is an interesting case. It looks like this might actually be a bug fix as we are preserving the user's original formatting in more cases. In this case its a problem for the user because they are changing from a key-value pair to a table where the decor no longer applies.

epage commented 1 month ago

insert_formatted is not overriding formatting, like it is supposed to.

In light of insert_formatted, I'm tempted to keep insert not preserving the existing formatting.

torokati44 commented 1 month ago

Just so I'm not mistaken: Does this mean that plain insert will resume working like it used to, in a patch release; or that we're supposed to switch to insert_formatted, or do something else...?

epage commented 1 month ago

For now, I'm restoring 0.22.20's behavior, see f74124d

torokati44 commented 1 month ago

Yeah, just saw that, thank you!

"For now" - and how about later?

epage commented 1 month ago

"For now" - and how about later?

Likely based on feedback and, at this point, through a breaking change

torokati44 commented 1 month ago

Thank you for the quick fix! ^^