Merge #203

203: Rustc: Create `Adapter` early to register lints with rustc r=NathanFrasier a=xFrednet

Registering the lints early, makes rustc track the lint level correctly (Assuming that marker is registered as a tool)

Not much more to say, I wrote this thing last night and I was super tired at the end. But it works, that's the important part ^^

---

Closes: #200


Co-authored-by: xFrednet <[email protected]>

Author: bors[bot]

marker_adapter/src/lib.rs

@@ -7,7 +7,7 @@
 pub mod context;
 mod loader;
 
-use std::ops::ControlFlow;
+use std::{cell::RefCell, ops::ControlFlow};
 
 use loader::LintCrateRegistry;
 use marker_api::{
@@ -22,34 +22,48 @@ use marker_api::{
 };
 use marker_utils::visitor::{self, Visitor};
 
-/// This struct is the interface used by lint drivers to pass transformed objects to
-/// external lint passes.
+/// This struct is the interface used by lint drivers to load lint crates, pass
+/// `marker_api` objects to external lint passes and all other magic you can think of.
 pub struct Adapter {
+    /// [`LintPass`] functions are called with a mutable `self` parameter as the
+    /// first argument. This `RefCell` acts as a wrapper to hide the internal
+    /// mutability from drivers.
+    ///
+    /// The effects of the mutability should never reach the driver anyways and
+    /// this just makes it way easier to handle the adapter in drivers.
+    inner: RefCell<AdapterInner>,
+}
+
+struct AdapterInner {
     external_lint_crates: LintCrateRegistry,
 }
 
 impl Adapter {
     #[must_use]
     pub fn new_from_env() -> Self {
         let external_lint_crates = LintCrateRegistry::new_from_env();
-        Self { external_lint_crates }
+        Self {
+            inner: RefCell::new(AdapterInner { external_lint_crates }),
+        }
     }
 
     #[must_use]
-    pub fn registered_lints(&self) -> Vec<LintPassInfo> {
-        self.external_lint_crates.collect_lint_pass_info()
+    pub fn lint_pass_infos(&self) -> Vec<LintPassInfo> {
+        self.inner.borrow().external_lint_crates.collect_lint_pass_info()
     }
 
-    pub fn process_krate<'ast>(&mut self, cx: &'ast AstContext<'ast>, krate: &Crate<'ast>) {
-        self.external_lint_crates.set_ast_context(cx);
+    pub fn process_krate<'ast>(&self, cx: &'ast AstContext<'ast>, krate: &Crate<'ast>) {
+        let inner = &mut *self.inner.borrow_mut();
+
+        inner.external_lint_crates.set_ast_context(cx);
 
         for item in krate.items() {
-            visitor::traverse_item::<()>(cx, self, *item);
+            visitor::traverse_item::<()>(cx, inner, *item);
         }
     }
 }
 
-impl Visitor<()> for Adapter {
+impl Visitor<()> for AdapterInner {
     fn visit_item<'ast>(&mut self, cx: &'ast AstContext<'ast>, item: ItemKind<'ast>) -> ControlFlow<()> {
         self.external_lint_crates.check_item(cx, item);
         ControlFlow::Continue(())

marker_api/src/interface.rs

@@ -169,3 +169,10 @@ impl LintPassInfoBuilder {
 pub struct LintPassInfo {
     lints: FfiSlice<'static, &'static Lint>,
 }
+
+#[cfg(feature = "driver-api")]
+impl LintPassInfo {
+    pub fn lints(&self) -> &[&'static Lint] {
+        self.lints.get()
+    }
+}

marker_lints/src/lib.rs

@@ -18,11 +18,11 @@ marker_api::declare_lint!(
 );
 
 #[derive(Debug, Default)]
-struct MarkerLintPass;
+struct MarkerLintsLintPass;
 
-marker_api::export_lint_pass!(MarkerLintPass);
+marker_api::export_lint_pass!(MarkerLintsLintPass);
 
-impl LintPass for MarkerLintPass {
+impl LintPass for MarkerLintsLintPass {
     fn info(&self) -> LintPassInfo {
         LintPassInfoBuilder::new(Box::new([DIAG_MSG_UPPERCASE_START])).build()
     }

marker_rustc_driver/src/conversion/rustc.rs

@@ -8,18 +8,27 @@ use rustc_hash::FxHashMap;
 
 use crate::context::storage::Storage;
 
+thread_local! {
+    /// This maps marker lints to lint instances used by rustc.
+    ///
+    /// Rustc requires lints to be registered, before the lint pass is run. This
+    /// is a problem for this conversion setup, as the used `*Converter` structs
+    /// require the `'ast` lifetime. Storing this specific map outside the struct
+    /// and providing a static conversion method, is simply a hack to allow the
+    /// early conversion of lints, so they can be registered.
+    ///
+    /// If we run into more problems like this, we might have to rethink the structure
+    /// again... let's just hope this doesn't happen!
+    static LINTS_MAP: RefCell<FxHashMap<&'static Lint, &'static rustc_lint::Lint>> = RefCell::default();
+}
+
 pub struct RustcConverter<'ast, 'tcx> {
     rustc_cx: rustc_middle::ty::TyCtxt<'tcx>,
     storage: &'ast Storage<'ast>,
-    lints: RefCell<FxHashMap<&'static Lint, &'static rustc_lint::Lint>>,
 }
 
 impl<'ast, 'tcx> RustcConverter<'ast, 'tcx> {
     pub fn new(rustc_cx: rustc_middle::ty::TyCtxt<'tcx>, storage: &'ast Storage<'ast>) -> Self {
-        Self {
-            rustc_cx,
-            storage,
-            lints: RefCell::default(),
-        }
+        Self { rustc_cx, storage }
     }
 }

marker_rustc_driver/src/conversion/rustc/common.rs

@@ -164,6 +164,16 @@ impl<'ast, 'tcx> RustcConverter<'ast, 'tcx> {
 
     #[must_use]
     pub fn to_lint_level(&self, api_level: Level) -> rustc_lint::Level {
+        Self::static_to_lint_level(api_level)
+    }
+
+    /// This not being a method taking `&self` is a small hack, to allow the
+    /// creation of `&'static Lint` instances before the start of the `'ast`
+    /// lifetime, required by the [`RustcConverter`].
+    ///
+    /// When possible, please use [`RustcConverter::to_lint_level`] instead.
+    #[must_use]
+    pub fn static_to_lint_level(api_level: Level) -> rustc_lint::Level {
         match api_level {
             Level::Allow => rustc_lint::Level::Allow,
             Level::Warn => rustc_lint::Level::Warn,
@@ -173,6 +183,7 @@ impl<'ast, 'tcx> RustcConverter<'ast, 'tcx> {
         }
     }
 
+    #[must_use]
     pub(crate) fn to_applicability(&self, app: Applicability) -> rustc_errors::Applicability {
         match app {
             Applicability::MachineApplicable => rustc_errors::Applicability::MachineApplicable,
@@ -183,6 +194,7 @@ impl<'ast, 'tcx> RustcConverter<'ast, 'tcx> {
         }
     }
 
+    #[must_use]
     pub fn to_span(&self, api_span: &Span<'ast>) -> rustc_span::Span {
         let (_, src_info) = self
             .storage

marker_rustc_driver/src/conversion/rustc/unstable.rs

@@ -3,26 +3,42 @@ use marker_api::lint::{Lint, MacroReport};
 use super::RustcConverter;
 
 impl<'ast, 'tcx> RustcConverter<'ast, 'tcx> {
+    #[must_use]
     pub fn to_lint(&self, api_lint: &'static Lint) -> &'static rustc_lint::Lint {
-        self.lints.borrow_mut().entry(api_lint).or_insert_with(|| {
-            // Not extracted to an extra function, as it's very specific
-            let report_in_external_macro = match api_lint.report_in_macro {
-                MacroReport::No => false,
-                MacroReport::All => true,
-                _ => unreachable!(),
-            };
+        Self::static_to_lint(api_lint)
+    }
+
+    /// This not being a method taking `&self` is a small hack, to allow the
+    /// creation of `&'static Lint` instances before the start of the `'ast`
+    /// lifetime, required by the [`RustcConverter`].
+    ///
+    /// When possible, please use [`RustcConverter::to_lint`] instead.
+    #[must_use]
+    pub fn static_to_lint(api_lint: &'static Lint) -> &'static rustc_lint::Lint {
+        super::LINTS_MAP.with(|lints| {
+            // This extra value, with the explicit lifetime is needed to make rustc
+            // see that it actually has the `'static` lifetime
+            let lint: &'static rustc_lint::Lint = lints.borrow_mut().entry(api_lint).or_insert_with(move || {
+                // Not extracted to an extra function, as it's very specific
+                let report_in_external_macro = match api_lint.report_in_macro {
+                    MacroReport::No => false,
+                    MacroReport::All => true,
+                    _ => unreachable!(),
+                };
 
-            Box::leak(Box::new(rustc_lint::Lint {
-                name: api_lint.name,
-                default_level: self.to_lint_level(api_lint.default_level),
-                desc: api_lint.explanation,
-                edition_lint_opts: None,
-                report_in_external_macro,
-                future_incompatible: None,
-                is_plugin: true,
-                feature_gate: None,
-                crate_level_only: false,
-            }))
+                Box::leak(Box::new(rustc_lint::Lint {
+                    name: api_lint.name,
+                    default_level: Self::static_to_lint_level(api_lint.default_level),
+                    desc: api_lint.explanation,
+                    edition_lint_opts: None,
+                    report_in_external_macro,
+                    future_incompatible: None,
+                    is_plugin: true,
+                    feature_gate: None,
+                    crate_level_only: false,
+                }))
+            });
+            lint
         })
     }
 }

marker_rustc_driver/src/lint_pass.rs

@@ -1,25 +1,65 @@
+use std::cell::LazyCell;
+
 use marker_adapter::Adapter;
+use marker_api::lint::Lint;
 
 use crate::context::{storage::Storage, RustcContext};
 
-pub struct MarkerLintPass;
+thread_local! {
+    /// The [`Adapter`] loads the lint crates and is the general interface used
+    /// by drivers to communicate with lint crates.
+    ///
+    /// The lint crates have to be loaded before the instantiation of [`RustcLintPass`]
+    /// to allow this driver to register the lints before the lint pass starts.
+    /// (See [`super::MarkerCallback::config`]). Storing the `Adapter` in a `thread_local`
+    /// cell is the easiest solution I could come up with. It should be fine performance
+    /// wise.
+    ///
+    /// Storing the [`Adapter`] in a `thread_local` is safe, since rustc is currently
+    /// only single threaded. This cell will therefore only be constructed once, and
+    /// this driver will always use the same adapter.
+    static ADAPTER: LazyCell<Adapter> = LazyCell::new(|| {
+        Adapter::new_from_env()
+    });
+}
+
+pub struct RustcLintPass;
+
+impl RustcLintPass {
+    pub fn marker_lints() -> Vec<&'static Lint> {
+        ADAPTER.with(|adapter| {
+            adapter
+                .lint_pass_infos()
+                .iter()
+                .flat_map(marker_api::LintPassInfo::lints)
+                .copied()
+                .collect()
+        })
+    }
+}
 
-rustc_lint_defs::impl_lint_pass!(MarkerLintPass => []);
+rustc_lint_defs::impl_lint_pass!(RustcLintPass => []);
 
-impl<'tcx> rustc_lint::LateLintPass<'tcx> for MarkerLintPass {
+impl<'tcx> rustc_lint::LateLintPass<'tcx> for RustcLintPass {
     fn check_crate(&mut self, rustc_cx: &rustc_lint::LateContext<'tcx>) {
-        process_crate(rustc_cx);
+        ADAPTER.with(|adapter| {
+            process_crate(rustc_cx, adapter);
+        });
     }
 }
 
-fn process_crate(rustc_cx: &rustc_lint::LateContext<'_>) {
+fn process_crate(rustc_cx: &rustc_lint::LateContext<'_>, adapter: &Adapter) {
     let storage = Storage::default();
-    process_crate_lifetime(rustc_cx, &storage);
+    process_crate_lifetime(rustc_cx, &storage, adapter);
 }
 
 /// This function marks the start of the `'ast` lifetime. The lifetime is defined
 /// by the [`Storage`] object.
-fn process_crate_lifetime<'ast, 'tcx: 'ast>(rustc_cx: &rustc_lint::LateContext<'tcx>, storage: &'ast Storage<'ast>) {
+fn process_crate_lifetime<'ast, 'tcx: 'ast>(
+    rustc_cx: &rustc_lint::LateContext<'tcx>,
+    storage: &'ast Storage<'ast>,
+    adapter: &Adapter,
+) {
     let driver_cx = RustcContext::new(rustc_cx.tcx, rustc_cx.lint_store, storage);
 
     // To support debug printing of AST nodes, as these might sometimes require the
@@ -31,6 +71,5 @@ fn process_crate_lifetime<'ast, 'tcx: 'ast>(rustc_cx: &rustc_lint::LateContext<'
         .marker_converter
         .to_crate(rustc_hir::def_id::LOCAL_CRATE, driver_cx.rustc_cx.hir().root_module());
 
-    let mut adapter = Adapter::new_from_env();
     adapter.process_krate(driver_cx.ast_cx(), krate);
 }

marker_rustc_driver/src/main.rs

@@ -3,6 +3,7 @@
 #![feature(let_chains)]
 #![feature(lint_reasons)]
 #![feature(iter_collect_into)]
+#![feature(lazy_cell)]
 #![feature(non_exhaustive_omitted_patterns_lint)]
 #![warn(rustc::internal)]
 #![warn(clippy::pedantic)]
@@ -40,6 +41,8 @@ use std::process::{exit, Command};
 use rustc_session::config::ErrorOutputType;
 use rustc_session::EarlyErrorHandler;
 
+use crate::conversion::rustc::RustcConverter;
+
 const RUSTC_TOOLCHAIN_VERSION: &str = "nightly-2023-07-13";
 
 struct DefaultCallbacks;
@@ -49,13 +52,21 @@ struct MarkerCallback;
 
 impl rustc_driver::Callbacks for MarkerCallback {
     fn config(&mut self, config: &mut rustc_interface::Config) {
-        // Clippy explicitly calls any previous functions. This will not be
-        // done here to keep it simple and to ensure that only known code is
-        // executed.
+        // Clippy explicitly calls any previous `register_lints` functions. This
+        // will not be done here to keep it simple and to ensure that only known
+        // code is executed.
         assert!(config.register_lints.is_none());
 
         config.register_lints = Some(Box::new(|_sess, lint_store| {
-            lint_store.register_late_pass(|_| Box::new(lint_pass::MarkerLintPass));
+            // Register lints from lint crates. This is required to have rustc track
+            // the lint level correctly.
+            let lints: Vec<_> = lint_pass::RustcLintPass::marker_lints()
+                .into_iter()
+                .map(RustcConverter::static_to_lint)
+                .collect();
+            lint_store.register_lints(&lints);
+
+            lint_store.register_late_pass(|_| Box::new(lint_pass::RustcLintPass));
         }));
     }
 }

marker_uilints/tests/ui/lint_level_attributes.rs

@@ -0,0 +1,10 @@
+#![feature(register_tool)]
+#![register_tool(marker)]
+
+const FIND_ME_DEFAULT: i32 = 0;
+
+#[allow(marker::item_with_test_name)]
+const FIND_ME_ALLOW: i32 = 0;
+
+#[deny(marker::item_with_test_name)]
+const FIND_ME_DENY: i32 = 0;

marker_uilints/tests/ui/lint_level_attributes.stderr

@@ -0,0 +1,22 @@
+warning: found a `const` item with a test name
+ --> $DIR/lint_level_attributes.rs:4:1
+  |
+4 | const FIND_ME_DEFAULT: i32 = 0;
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |
+  = note: `#[warn(marker::item_with_test_name)]` on by default
+
+error: found a `const` item with a test name
+  --> $DIR/lint_level_attributes.rs:10:1
+   |
+10 | const FIND_ME_DENY: i32 = 0;
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/lint_level_attributes.rs:9:8
+   |
+9  | #[deny(marker::item_with_test_name)]
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error; 1 warning emitted
+