Merge #46

46: Rustc syntactic types, global context and some docs r=xFrednet a=xFrednet

Follow-up of #45. This adds the rustc backend. I've also adapted the type representation a bit.

As always, happy about reviews, but I'll merge it without, if there has been no interaction by the next step 🙃 

closes #44
closes #40

CC: #35

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

Author: bors[bot]

.github/workflows/rust.yml

@@ -38,7 +38,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-          toolchain: nightly-2022-09-22
+          toolchain: nightly-2022-11-03
           components: cargo, clippy, rustfmt
     - run: rustc -vV
     - run: cargo build

.github/workflows/rust_bors.yml

@@ -33,7 +33,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: actions-rs/toolchain@v1
       with:
-          toolchain: nightly-2022-09-22
+          toolchain: nightly-2022-11-03
           components: cargo, clippy, rustfmt
     - run: rustc -vV
     - run: cargo build

Cargo.lock

@@ -43,7 +43,7 @@ fn main() {
     let matches = get_clap_config().get_matches_from(std::env::args().take_while(|s| s != CARGO_ARGS_SEPARATOR));
     if matches.is_present("version") {
         let version_info = env!("CARGO_PKG_VERSION");
-        println!("{}", version_info);
+        println!("{version_info}");
         exit(0);
     }
 

cargo-linter/src/main.rs

@@ -0,0 +1,39 @@
+# Driver information
+
+This document contains information required to implement linter drivers.
+
+## Lifetimes
+
+### Models
+
+Lint-crates officially only know the `'ast` lifetime. All non-copy items given to lint-crates have this lifetime. For a lint crate, it feels like the `'ast` lifetime start with the call of a `check_` function and also ends with it. Drivers layer their implementation on top of this view.
+
+Rustc's linter driver uses roughly the following lifetime model:
+
+```
+'tcx by Rustc:       |------------------------------------------|
+'ast by Driver:         |------------------------------------|
+Lint-crates:               |------------------------------|
+LintPass functions:           |--------|  |----|  |----|
+```
+
+The lint-crates are loaded after the entire AST has been mapped and will only be dropped after they're unloaded. This makes `'ast` basically equivalent to `'static` in this model. However, this API is not only intended for compilers like rustc, but also potentially language servers. These have different requirements as the AST has to change. This means nodes have to be dropped and rechecked again. The following showcases a model that could be used by rust-analyzer:
+
+```
+rust-analyzer:        |------------------------------------|
+Lint-crates:             |------------------------------|
+'ast for AST nodes:         |----------|  |----------|
+LintPass functions:            |-----|      |-----|
+```
+
+Notice that here the lint-crates life longer than the AST they're analyzing.
+
+### `'static AstContext` hackery
+
+AST nodes can provide a lot of information about themselves and their context. Most of this information is not stored in the node itself, but dynamically loaded using an `AstContext` instance. In the initial prototype, the `AstContext` was stored in each node. However, this added an unnecessary reference to every node, increasing their size and made the driver code more convoluted. For these reason, the implementation was changed to use a global instance that is used by all nodes.
+
+This global instance is set by the adapter before any `check_*` function is called inside the lint crate. The context will be updated once for every tree given to the adapter. The current implementation imposes the following requirements:
+
+1. The `AstContext` instance has to be valid for the entire time, that it takes to process the AST passed to the adapter. This means that the instance as well as the AST has to remain in memory. This behavior can implement all models as described above.
+2. Lint crates should always be called by the same thread and not be access concurrently. (The implementation might be able to handle it, but this is not actively tested.)
+3. The driver should never call functions on AST nodes that depend on the current `AstContext` instance. (In general, there should be no reason to do so.)

docs/internal/driver-info.md

@@ -0,0 +1,10 @@
+# Type guidelines
+
+This file contains guidelines for API types and the reasoning behind these:
+
+* API types should all have `#[repr(C)]`
+* API enums should all have a `#[non_exhaustive]` attribute
+* Slices provided and stored by the API should contain objects and not references to objects:
+    * Prefer `&[Parameter]` over `&[&Parameter]`
+    * This reduces the number of lifetimes and helps the driver creation a bit. The borrow checker will enforce, that the items in the slice are never moved unless they're `Copy`. This also means that we should be careful which items we give `Copy`
+

docs/internal/type-guidelines.md

@@ -29,6 +29,8 @@ impl<'ast> Adapter<'ast> {
     }
 
     pub fn process_krate(&mut self, cx: &'ast AstContext<'ast>, krate: &Crate<'ast>) {
+        self.external_lint_crates.set_ast_context(cx);
+
         for item in krate.items() {
             self.external_lint_crates.check_item(cx, *item);
         }

linter_adapter/src/lib.rs

@@ -1,5 +1,6 @@
 use libloading::Library;
 
+use linter_api::context::AstContext;
 use linter_api::lint::Lint;
 use linter_api::LintPass;
 
@@ -10,7 +11,7 @@ pub struct LintCrateRegistry<'ast> {
     passes: Vec<LoadedLintCrate<'ast>>,
 }
 
-impl<'a> LintCrateRegistry<'a> {
+impl<'ast> LintCrateRegistry<'ast> {
     /// # Errors
     /// This can return errors if the library couldn't be found or if the
     /// required symbols weren't provided.
@@ -43,6 +44,12 @@ impl<'a> LintCrateRegistry<'a> {
 
         new_self
     }
+
+    pub(super) fn set_ast_context(&self, cx: &'ast AstContext<'ast>) {
+        for lint_pass in &self.passes {
+            lint_pass.set_ast_context(cx);
+        }
+    }
 }
 
 impl<'ast> LintPass<'ast> for LintCrateRegistry<'ast> {
@@ -82,6 +89,7 @@ macro_rules! gen_LoadedLintCrate {
         /// reference due to lifetime restrictions.
         struct LoadedLintCrate<'ast> {
             _lib: &'static Library,
+            set_ast_context: libloading::Symbol<'ast, unsafe extern "C" fn(&'ast AstContext<'ast>) -> ()>,
             $(
                 $fn_name: libloading::Symbol<'ast, unsafe extern "C" fn($($arg_ty,)*) -> $ret_ty>,
             )*
@@ -101,6 +109,11 @@ macro_rules! gen_LoadedLintCrate {
                     return Err(LoadingError::IncompatibleVersion);
                 }
 
+                let set_ast_context = unsafe {
+                    lib.get::<unsafe extern "C" fn(&'ast AstContext<'ast>) -> ()>(b"set_ast_context\0")
+                        .map_err(|_| LoadingError::MissingLintDeclaration)?
+                };
+
                 $(
                     let $fn_name = {
                         let name: Vec<u8> = stringify!($fn_name).bytes().chain(std::iter::once(b'\0')).collect();
@@ -113,12 +126,19 @@ macro_rules! gen_LoadedLintCrate {
                 // create Self
                 Ok(Self {
                     _lib: lib,
+                    set_ast_context,
                     $(
                         $fn_name,
                     )*
                 })
             }
 
+            fn set_ast_context(&self, cx: &'ast AstContext<'ast>) -> () {
+                unsafe {
+                    (self.set_ast_context)(cx)
+                }
+            }
+
             // safe wrapper to external functions
             $(
                 fn $fn_name(& $($mut_)* self $(, $arg_name: $arg_ty)*) -> $ret_ty {

linter_adapter/src/loader.rs

@@ -9,6 +9,8 @@ pub use ast_path::*;
 
 use std::fmt::Debug;
 
+use super::generic::GenericArgs;
+
 #[non_exhaustive]
 #[derive(Copy, Clone, Eq, PartialEq, Debug)]
 pub enum Edition {
@@ -70,6 +72,7 @@ pub enum Abi {
     /// implementations. In general this means that the user has not selected a
     /// specific ABI.
     Default,
+    C,
     /// FIXME: Remove this variant. See
     /// <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/spec/abi/enum.Abi.html>
     Other,
@@ -117,3 +120,27 @@ pub trait Attribute<'ast>: Debug {
 }
 
 pub trait Pattern<'ast> {}
+
+#[repr(C)]
+#[derive(Debug, PartialEq, Eq, Hash)]
+pub struct TraitRef<'ast> {
+    item_id: ItemId,
+    generics: GenericArgs<'ast>,
+}
+
+#[cfg(feature = "driver-api")]
+impl<'ast> TraitRef<'ast> {
+    pub fn new(item_id: ItemId, generics: GenericArgs<'ast>) -> Self {
+        Self { item_id, generics }
+    }
+}
+
+impl<'ast> TraitRef<'ast> {
+    pub fn trait_id(&self) -> ItemId {
+        self.item_id
+    }
+
+    pub fn generics(&self) -> &GenericArgs<'ast> {
+        &self.generics
+    }
+}

linter_api/src/ast/common.rs

@@ -8,48 +8,55 @@
 use super::SymbolId;
 use crate::{
     ast::generic::GenericArgs,
-    context::AstContext,
+    context::with_cx,
     ffi::{FfiOption, FfiSlice},
 };
 
 #[repr(C)]
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct AstPath<'ast> {
-    _cx: &'ast AstContext<'ast>,
-    segments: FfiSlice<'ast, &'ast AstPathSegment<'ast>>,
+    // FIXME: Add optional target ID for values lifetimes etc that is faster to compare
+    //
+    // You were last trying to fix the compiler error related to lifetime identification and paths
+    segments: FfiSlice<'ast, AstPathSegment<'ast>>,
+}
+
+#[cfg(feature = "driver-api")]
+impl<'ast> AstPath<'ast> {
+    pub fn new(segments: FfiSlice<'ast, AstPathSegment<'ast>>) -> Self {
+        Self { segments }
+    }
 }
 
 impl<'ast> AstPath<'ast> {
-    pub fn segments(&self) -> &[&AstPathSegment<'ast>] {
+    pub fn segments(&self) -> &[AstPathSegment<'ast>] {
         self.segments.get()
     }
 }
 
 #[repr(C)]
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct AstPathSegment<'ast> {
-    cx: &'ast AstContext<'ast>,
     ident: SymbolId,
-    generic_args: FfiOption<&'ast GenericArgs<'ast>>,
+    generics: FfiOption<GenericArgs<'ast>>,
 }
 
 #[cfg(feature = "driver-api")]
 impl<'ast> AstPathSegment<'ast> {
-    pub fn new(cx: &'ast AstContext<'ast>, ident: SymbolId, generic_args: Option<&'ast GenericArgs<'ast>>) -> Self {
+    pub fn new(ident: SymbolId, generics: Option<GenericArgs<'ast>>) -> Self {
         Self {
-            cx,
             ident,
-            generic_args: generic_args.into(),
+            generics: generics.into(),
         }
     }
 }
 
 impl<'ast> AstPathSegment<'ast> {
     pub fn ident(&self) -> String {
-        self.cx.symbol_str(self.ident)
+        with_cx(self, |cx| cx.symbol_str(self.ident))
     }
 
-    pub fn generic_args(&self) -> Option<&GenericArgs<'ast>> {
-        self.generic_args.get().copied()
+    pub fn generics(&self) -> Option<&GenericArgs<'ast>> {
+        self.generics.get()
     }
 }