FileLines::all().to_json_spans() returns [] which is parsed as empty map

[Xanewok]

This surfaced at rust-lang/rls#1463 (fixed in rust-lang/rls#1497).

The underlying data structure discerns a missing/"all" state in addition to actual set (possibly empty) of lines:

rustfmt/src/config/file_lines.rs

Lines 151 to 157 in 26d370e

	/// A set of lines in files.
	///
	/// It is represented as a multimap keyed on file names, with values a collection of
	/// non-overlapping ranges sorted by their start point. An inner `None` is interpreted to mean all
	/// lines in all files.
	#[derive(Clone, Debug, Default, PartialEq)]
	pub struct FileLines(Option<HashMap<FileName, Vec<Range>>>);

whereas to_json_span only encodes a specified set of those changes (can't represent FileLines(None)):

rustfmt/src/config/file_lines.rs

Lines 218 to 221 in 26d370e

	/// Returns JSON representation as accepted by the `--file-lines JSON` arg.
	pub fn to_json_spans(&self) -> Vec<JsonSpan> {
	match &self.0 {
	None => vec![],

I think we should either:

1. panic/return err on None match in to_json_spans (since vec![] returned for FileLines(None) won't be parsed back into the FileLines(None) but rather FileLines(Some(HashMap::new))
2. Somehow discern or accept null lines in --file-lines JSON and make to_json_spans() return Option<Vec<JsonSpan>>

Thoughts?

cc @jsgf @ljw1004

[topecongiro]

since vec![] returned for FileLines(None) won't be parsed back into the FileLines(None) but rather FileLines(Some(HashMap::new)

I think that it would be simpler to fix this part instead, vec![] should be parsed to FileLines(None)?

[Xanewok]

This would mean that --file-lines [] would correspond to FileLines::all() - wouldn't that also be surprising?

[topecongiro]

Yeah, you are right. Actually rustfmt --help=file-lines explicitly states that Specifying an empty array will result in no files being formatted.

In that case, I would prefer to go with your second option.

[topecongiro]

If we go with @Xanewok's second option, then it will introduce an update to the public API (FileLines:: to_json_spans), which in theory requires a major version update of rustfmt. However I am not planning to release 2.0 anytime soon, so the fix will be postponed for a few months. This does not sound great.

Rather, we could implement Serialize and Deserialize on FileLines to fix the issue, and deprecate FileLines::to_json_spans. This does not introduce any breaking changes.

A different approach would be to relax the stability guarantee of rustfmt so that 'small' breaking changes can be introduced with a minor version update. This sounds fine, as the main (or the only?) consumer of rustfmt-as-a-library is rls.

[Xanewok]

I'd probably lean towards the last option since it's more of a bugfix (comment mentions serializing into format accepted by --file-lines JSON but in this case the None match is wrongly deserialized to []).

However, having a (De)Serialize impl sounds like a useful thing on its own.

What's puzzling is the difference between representation - file-lines accepts Vec<(File, Range)> while internally (which makes more sense) it's stored as HashMap<File, Vec<Range>>. Is it for backwards-compatibility and the internals were switched at some point?

I'd probably find it puzzling to see a slightly different Serialized data layout to the one used internally (when the original could be trivially serialized as-is), so I'm -0.5 on the fix using these impls.

[calebcartwright]

@topecongiro I can take a look at this one

[calebcartwright]

Opened #3954 that implements the second option detailed by @Xanewok

I didn't include the Serialize and Deserialize implementations for FileLines as it wasn't clear to me from this thread if we wanted to do so, and it also seems like we're already using those to bubble up some errors:

rustfmt/src/config/file_lines.rs

Lines 333 to 354 in 7713d05

	impl<'de> ::serde::de::Deserialize<'de> for FileLines {
	fn deserialize<D>(_: D) -> Result<Self, D::Error>
	where
	D: ::serde::de::Deserializer<'de>,
	{
	panic!(
	"FileLines cannot be deserialized from a project rustfmt.toml file: please \
	specify it via the `--file-lines` option instead"
	);
	}
	}
	// We also want to avoid attempting to serialize a FileLines to toml. The
	// `Config` struct should ensure this impl is never reached.
	impl ::serde::ser::Serialize for FileLines {
	fn serialize<S>(&self, _: S) -> Result<S::Ok, S::Error>
	where
	S: ::serde::ser::Serializer,
	{
	unreachable!("FileLines cannot be serialized. This is a rustfmt bug.");
	}
	}

However, let me know you'd prefer utilizing those impls and removing the relevant functions/etc. that are currently in use.