Rustc: Librarify marker_rustc_driver

[smoelius]

Fixes #257

How do you feel about this?

I would summarize the changes as follows:

• marker_rustc_driver/src/bin/marker_rustc_driver.rs is a new file that contains a slightly modified version of the old main function.
• try_main was updated to accept an impl Iterator<Item = String>. main passes std::env::args() as this argument.¹
• The following were made pub:
  • try_main
  • lint_pass
  • MainError
  • process_crate

---

Footnotes

1. This change was not strictly necessary. But since try_main is now a public function, the idea of it reading from std::env::args() directly seemed weird to me. ↩

[smoelius]

I think this may be the happiest I can get CI?

[xFrednet]

The CI has some tests, that will fail until v0.3.0 has been released. I'll review this during the weekend :)

[xFrednet]

I think these changes are very reasonable. And I'm okay with merging it, once my comments have been addressed :)

Note, that I've just updated the used nightly version and the CI should be fully functional now. It would be good if you could rebase. And thank you for waiting on my review :D

[xFrednet]

Today I learned that you can have multiple binaries simply by adding a bin folder under source o.O

Cargo's documentation states:

• The default executable file is src/main.rs.
  • Other executables can be placed in src/bin/.

Could you move this file to src/main.rs? For me, that's the default location where I would look first, and it's also the default executable, which should be located in src/main.rs according to the docs.

I assume, this was just a stylistic choice, as moving it to src/main.rs seems to work the same way. If there is another reason, please share it :)

[smoelius]

I assume, this was just a stylistic choice

To be honest, I didn't know a package could have both a src/lib.rs and a src/main.rs. So, TIL too. :)

[smoelius]

@xFrednet I think I've addressed all of your comments. Please let me know if you see anything else.

[smoelius]

Sorry, I noticed a typo. I think the PR should be good now.

[xFrednet]

This version looks good to me. I have one question, and then we should be able to merge this. The changes make try_main public. Will dylint call this function or is this just for Markers binary?

[smoelius]

The changes make try_main public. Will dylint call this function or is this just for Markers binary?

try_main is just for Marker's binary.

Right now, the PoC uses just these exports:

use marker_adapter::{Adapter, LintCrateInfo};
use marker_rustc_driver::lint_pass;

I split-out the library at try_main because (1) it was easy to do, and (2) it was easy to see what changed.

But from my perspective, the split does not have to occur at try_main.

(No worries if you would like additional changes.)

[xFrednet]

My question about try_main was just to ensure you're not running the compiler twice :)

[xFrednet]

Rustc uses caching where possible. This function, and the function above, register relevant files and environment values which should be checked, when rustc decides if a cache can be reused or not.

This one mainly registers the Cargo.toml file, as that's where Marker's lints are configured and the lint crate files. It basically just ensures that we don't use caches if any lint crate was updated. Do you maybe want to make this one public as well? I'm not sure how dylint handles this kind of thing.

This is just a suggestion, if you handle this differently or don't need it, I'm happy to merge this as is :)

[smoelius]

Do you maybe want to make this one public as well?

It's kind of you to ask, but I don't think it's necessary.

If "marker_rustc_driver as a library" feels "incomplete" to you without this function, I can sympathize with that feeling. But I would recommend to keep the interface as small as possible until me or someone else complains. :)

[xFrednet]

I just wanted to mention it, since i[#294]t was a thing we ran into after some testing :)

[smoelius]

I'm curious now (because caching has been the source of several bugs in Dylint). Is there a direct connection between caching and #294?

[xFrednet]

The reference to #294 seems to be an accidental copy-pasta 🍝. Sorry, about that.

There is a direct connection between the function I marked here and caching. This is the PR that prevented unwanted caching for Marker: #211

Do you have some examples for caching related bugs in Dylint and how you solved them?

[smoelius]

Do you have some examples for caching related bugs in Dylint and how you solved them?

Two that come to mind are:

• Changing workspace metadata should cause the lints to be rerun trailofbits/dylint#650
  • fixed by: Track workspace metadata trailofbits/dylint#813
• Incorrect output when two different lint libs being executed consecutively trailofbits/dylint#856
  • fixed by: Fix #856 trailofbits/dylint#866