Handle multiple dependency tables and dependency delimiters

[mkatychev]

Handle multiple dependency tables:

cargo-machete/src/main.rs

Line 268 in 4fb101d

let valid_tables = HashSet::from(["dependencies", "build-dependencies", "dev-dependencies"]);

Handle superset of dependency names:

cargo-machete/src/main.rs

Lines 255 to 263 in 4fb101d

	// handle a superset of all dependency name dashed/underscored variants: re'\w[-_]'
	fn dep_name_superset(dep_names: &[String]) -> HashSet<String> {
	let mut unused: HashSet<String> = dep_names.iter().cloned().collect();
	for dep in unused.clone() {
	unused.insert(dep.replace('-', "_"));
	unused.insert(dep.replace('_', "-"));
	}
	unused
	}

[mkatychev]

I'm not sure how much the scanner recuses so cases like this may not be handled:

[target.'cfg(not(some_flag))'.dev-dependencies]
prost.workspace = true

[mkatychev]

dependency_list seems to consistently normalize for underscores but the toml_edit::Document preserves multi word delimiters

[mkatychev]

crate/build.rs probably also needs to be scanned, it doesn't seem to be the case with this PR

[bnjbvr]

Thanks! So if I understand correctly, it will find unused dependencies as specified from the main dependencies, and remove them when they're unused in any of dependencies, build-dependencies, and dev-dependencies.

Later, we could also handle a dependency that was analyzed in other dependencies arrays, like build-dependencies, and remove it from the list if it's unused there too. For build-dependencies it would require analyzing the build.rs file; for dev-dependencies it might also imply looking only at tests/benchmarks, which I'm not sure we can figure by just scanning text (i.e. there can be tests in any Rust source non-test file). Well, these are things to do later :)

Thanks for the PR! Can you add a test showing the new behavior, please?

[bnjbvr]

I think we should keep the previous: errors do implement Display. If we need the full context of an anyhow error, we can use {err:#}.

[bnjbvr]

I don't think we need this: the dependency name should be exact, or using cargo-machete --with-metadata will find the proper name. If you're fixing something here, can you add a test that would fail on main, and show what this is fixing, please?

[bnjbvr]

Suggested change

	.filter(|(k, v)| v.is_table_like() && valid_tables.contains(k.display_repr().as_ref()))
	.map(|(k, v)| {
	let table = v.as_table_like_mut().context(missing_table_msg)?;
	Ok((k, table))
	})
	.filter(|v| match v {
	Ok((_, table)) if dependency_superset.iter().any(|k| table.contains_key(k)) => true,
	Err(_) => true,
	Ok(_) => false,
	})
	Maybe `valid_tables` has some kind of `.get()` that would make it so we can avoid the filter, then the map, then the error handling step?
	```suggestion
	.filter_map(|(k, v)| if v.is_table_like() {
	if let Some(table) = valid_tables.get(k.display_repr().as_ref())) {
	if dependency_superset.iter().any(|k| table.contains_key(k)) {
	Some((k, table))
	} else {
	None
	}
	} else {
	None
	}
	})

Or, even better, let's get rid of all the iterators transformers and use plain imperative code instead, as it might be more readable in this case.

[bnjbvr]

Unfinished comment here? Maybe we could comment something like "We should find the unused dependency in at least one of the tables; return an error if that's not the case."

[bnjbvr]

Again, I'm not sure why we would need this; we'd never have to do it, so I would need a proof before being convinced we need to replace dashes ourselves :)

[bnjbvr]

bail! with a context doesn't seem very idiomatic to me: maybe we could instead plain use bail! here, and join the two error messages with a \n or :?

[bnjbvr]

hi @mkatychev, happy new year! Do you plan to work on this anytime soon, or should I close this PR for the time being?

[mkatychev]

@bnjbvr I can take a look later this week.

[mkatychev]

Just an update, haven't gotten around to this yet but still on my radar