Allow the specification of lint crates in a file (local path and git)

[xFrednet]

Currently, the lint crates have to be specified via console arguments. In a real world applications users most likely have a set of crates they want to run every time. Therefore, I suggest specifying lint crates in a file like Cargo does for dependencies.

There are several design decisions as part of this issue:

1. Which sources should be allowed?
   • I suggest mimicking the cargo format, as it has proven to work and users are used to it by now. This would mean supporting: git, path and version dependencies
2. Where should this information be specified, in Cargo.toml or a new marker.toml file?

---

Generally see: https://doc.rust-lang.org/cargo/commands/index.html for cargo comments and documentation

[xFrednet]

@Niki4tap I created an issue for this feature and for discussions

Some information:

• cargo install sadly only works with binaries, which is sad since that could have handled all the source resolution magic for us
• Cargo will warn about weird headers in the Cargo.toml file, but metadata headers are allowed like: [workspace.metadata.marker.lints]

This information was gathered by @Niki4tap, thank you!

[Niki4tap]

Tasks:

• Initial implementation (done in cargo-marker: add initial configuration support #84)
• Support git source fetching (done in cargo-marker: add git support #126)
• Support features
• (Needs discussion) Use cargo library to discover nested configurations in workspaces.

Moved tasks:

• (Hard) Support registries Moved to Allow fetching lints from registries #87
• (Optional/Hard) Support lint configuration Moved to Add option to add lint configurations #88

[Niki4tap]

Last point needs a bit of explanation, imagine:
Cargo.toml:

[workspace]
members = [
    "member_a",
    "member_b"
]

[workspace.metadata.marker.lints]
lint_a = { path = "lint_a" }

Do we want to give the ability for member_a to override the lints supplied in the virtual manifest (add new ones)?
member_a/Cargo.toml:

[package]
# your usual keys...

[package.metadata.marker.lints]
# Should we allow adding more lints here?
lint_b = { path = "../lint_b" }
# Should `lint_a` still apply for `member_a`?

[Niki4tap]

Additionally, using the cargo library would simplify source fetching and crate building a lot. The drawback for this, is that cargo library is unstable (and will never stabilize), just like rustc internals.

[xFrednet]

Thank you @Niki4tap!

(Hard) Support registries

I think this item will also be blocked, until v0.0.1 can be published to crates.io, since we can't upload a lint crate, without having marker_api on crates.io as well

(Optional/Hard) Support lint configuration

That would be something cool to see. Though, I'd move it to a new issue.

Cargo library

Development on cargo has slowed down AFAIK. So while this'll add to the maintenance cost, I'd be fine with it if it simplifies the code significantly. I fear that we'll have a weird mix of calling cargo commands where possible and link to it directly where not.

Do we want to give the ability for member_a to override the lints supplied in the virtual manifest (add new ones)?

This is an important question, maybe we could have the restriction that only the main Cargo.toml can specify lints. Crates can just enable lints they don't want. This limitation can be relaxed later on. That would be my take, for a simpler solution. Clippy only reads the main clippy.toml file and I don't know of anyone requesting this to be more specific. If someone really needs that, they could first navigate into the crate directory and then run cargo marker

Maybe dylint could be an interesting example. It's a tool for custom rust lints, though the user has to directly interact with rustc. From the README, it looks like they only look in the main Cargo.toml.

What are your thoughts on these topics?

[Niki4tap]

I think this item will also be blocked, until v0.0.1 can be published to crates.io, since we can't upload a lint crate, without having marker_api on crates.io as well

Oh, yeah, good point, feel free to extract this into a separate issue.

That would be something cool to see. Though, I'd move it to a new issue.

Yep, I was thinking adding a new field, something like:

[workspace.metadata.marker.lints.lint_a]
path = "lint_a"
config = { ... }

And passing raw toml Value to the lint somehow (maybe adding a function to LintPass), but again, that comes with its own issues, and a dependency on toml_edit for lints 😅, so you're right about moving this.

Maybe dylint could be an interesting example. It's a tool for custom rust lints, though the user has to directly interact with rustc. From the README, it looks like they only look in the main Cargo.toml.

I haven't looked that much into it so far, but I'm aware of it, and yes, afaik you're correct that looks only in the root Cargo.toml.

What are your thoughts on these topics?

For me it would ideally look like this:
Cargo.toml:

[workspace]
members = [
    "member_a",
    "member_b"
]

[workspace.metadata.marker.lints]
lint_a = { ... } # this isn't possible to override by workspace members

member_a/Cargo.toml:

[package]
# ...

[package.metadata.marker.lints]
lint_a = { ... } # <- this is an error
lint_b = { ... } # but adding new lints is ok (`lint_b` won't apply to `member_b`)

So the structure would be:

root: lint_a
|
|--- member_a: lint_b
|
|--- member_b: -

and then after cascading:

member_a: lint_a, lint_b
member_b: lint_a

But that requires a lot of work, and I'm probably just overthinking it 😅, so I'm fine with root config being the only one for now :)

[xFrednet]

I think this item will also be blocked, until v0.0.1 can be published to crates.io, since we can't upload a lint crate, without having marker_api on crates.io as well

Oh, yeah, good point, feel free to extract this into a separate issue.

I would keep it here for now, as this feels like a tracking issue. Or do you prefer it to be an extra issue?

That would be something cool to see. Though, I'd move it to a new issue.

Yep, I was thinking adding a new field, something like:

Related to lint configs, it might make sense to move them into a separate file. I've heard now several times, that larger multilingual projects don't use cargo but integrate rustc into their toolchain directly. For Clippy they also use the driver instead of cargo-clippy AFAIK. Having cargo-marker handle compilation etc is fine IMO, in the worst case they have to do the setup manually. Though, that might be one reason to move the lint configuration into a separate file. I'm mentioning it here, since that might affect where we want to define lints later one. For now, I prefer Cargo.toml though it could result in a weird split in the future.

But that requires a lot of work, and I'm probably just overthinking it 😅, so I'm fine with root config being the only one for now :)

I love to make things overly complicated as well, that's one reason for the Driver/API split. With some things, like this, I'm definitely open for the option, but would like to see a use-case first, since it might require a lot of work as you said. :)

[xFrednet]

Support git source fetching

Btw, with CVE-2022-46176 recently being posted for Cargo, we should think about how we interact with git for fetching resources. In the report, it says that using git via the console command is fine, as git then handles everything. If we bind to cargo the same should apply. However, if we bind to a crate like gitlib2, we might have to deal with something similar.

[Niki4tap]

I would keep it here for now, as this feels like a tracking issue. Or do you prefer it to be an extra issue?

I think it might be better to move out:

• Support registries task, since it's blocked on marker_api being released.
• Support lint configuration task, since it probably needs some design decisions, like you've mentioned with custom toolchains, I think we're fine with making basic lint dependencies in Cargo.toml, as we still leave an option to supply lint crates by cli arguments, but lint configuration might be better in a different file.

[xFrednet]

Btw, I just found this fetching code Clippy uses in the lintcheck tool. It might not be the most robust, but maybe a good reference.

I'm going to ask cargo to maybe add a download command, but I think it's unlikely to be added as there is a crate called cargo-download and the demand is quite low. The use case is also relatively specific. rust-lang/cargo#11563 rust-lang/cargo#1861

We could consider moving the crates.io and git fetching code into a different crate and then share it with Clippy. This would also be useful, if we want a tool like lintcheck for Marker. But this is probably a bigger task for the future :)

[xFrednet]

I'm guessing this issue can be closed, now that #126 has been merged. Ideally, we'll migrate to a cargo command version when the feature gets added to cargo. But until then this works well :)