Skip to content

Commit

Permalink
chore: clean up some lints
Browse files Browse the repository at this point in the history
  • Loading branch information
@WillLillis
WillLillis committed Oct 14, 2024
1 parent 228f054 commit b477aa6
Show file tree
Hide file tree
Showing 7 changed files with 308 additions and 238 deletions.
56 changes: 53 additions & 3 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ publish = true
exclude = ["samples/*", "demo/*"]
license = "BSD-2-Clause"

[lints.clippy]
# Using the URI type to identify open files is kind of unavoidable, tell clippy everything's gonna be ok
mutable_key_type = "allow"

[lib]
name = "asm_lsp"
Expand All @@ -29,6 +26,59 @@ path = "src/lib.rs"
name = "asm-lsp"
path = "src/bin/main.rs"

[lints.clippy]
dbg_macro = "deny"
todo = "deny"
pedantic = { level = "warn", priority = -1 }
nursery = { level = "warn", priority = -1 }
cargo = { level = "warn", priority = -1 }

# Taken from tree-sitter's `Cargo.toml`
# The lints below are a specific subset of the pedantic+nursery lints
# that we explicitly allow in the tree-sitter codebase because they either:
#
# 1. Contain false positives,
# 2. Are unnecessary, or
# 3. Worsen the code

# Using the URI type to identify open files is kind of unavoidable, tell clippy everything's gonna be ok
mutable_key_type = "allow"

branches_sharing_code = "allow"
cast_lossless = "allow"
cast_possible_truncation = "allow"
cast_possible_wrap = "allow"
cast_precision_loss = "allow"
cast_sign_loss = "allow"
checked_conversions = "allow"
cognitive_complexity = "allow"
collection_is_never_read = "allow"
fallible_impl_from = "allow"
fn_params_excessive_bools = "allow"
inline_always = "allow"
if_not_else = "allow"
items_after_statements = "allow"
match_wildcard_for_single_variants = "allow"
missing_errors_doc = "allow"
missing_panics_doc = "allow"
module_name_repetitions = "allow"
multiple_crate_versions = "allow"
option_if_let_else = "allow"
or_fun_call = "allow"
range_plus_one = "allow"
redundant_clone = "allow"
redundant_closure_for_method_calls = "allow"
ref_option = "allow"
similar_names = "allow"
string_lit_as_bytes = "allow"
struct_excessive_bools = "allow"
struct_field_names = "allow"
transmute_undefined_repr = "allow"
too_many_lines = "allow"
unnecessary_wraps = "allow"
unused_self = "allow"
used_underscore_items = "allow"

[dependencies]
anyhow = "1.0.70"
# write to stderr instead of stdout
Expand Down
14 changes: 12 additions & 2 deletions src/bin/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,17 @@ use log::{error, info};
use lsp_server::{Connection, Message, Notification, Request, RequestId};
use lsp_textdocument::TextDocuments;

// main -------------------------------------------------------------------------------------------
/// Entry point of the server. Connects to the client, loads documentation resources,
/// and then enters the main loop
///
/// # Errors
///
/// Returns `Err` if the server fails to connect to the lsp client
///
/// # Panics
///
/// Panics if JSON serialization of the server capabilities fails
#[allow(clippy::too_many_lines)]
pub fn main() -> Result<()> {
// initialisation -----------------------------------------------------------------------------
// Set up logging. Because `stdio_transport` gets a lock on stdout and stdin, we must have our
Expand Down Expand Up @@ -393,7 +403,7 @@ pub fn main() -> Result<()> {
Ok(())
}

#[allow(clippy::too_many_arguments)]
#[allow(clippy::too_many_arguments, clippy::too_many_lines)]
fn main_loop(
connection: &Connection,
config: &Config,
Expand Down
1 change: 0 additions & 1 deletion src/handle.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ pub fn handle_references_request(

/// Produces diagnostics and sends a `PublishDiagnostics` notification to the client
/// Diagnostics are only produced for the file specified by `uri`
/// Returns 'Err' if the response fails to send via `connection`
///
/// # Errors
///
Expand Down
12 changes: 8 additions & 4 deletions src/lsp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,12 @@ fn get_compilation_db_files(path: &Path) -> Option<CompilationDatabase> {
None
}

/// Returns a default `CompileCommand` for the provided `uri`. If the user specified
/// a compiler in their config, it will be used. Otherwise, the command will be constructed
/// with a single flag consisting of the provided `uri`
/// Returns a default `CompileCommand` for the provided `uri`.
///
/// - If the user specified a compiler in their config, it will be used.
/// - Otherwise, the command will be constructed with a single flag consisting of
/// the provided `uri`
///
/// NOTE: Several fields within the returned `CompileCommand` are intentionally left
/// uninitialized to avoid unnecessary allocations. If you're using this function
/// in a new place, please reconsider this assumption
Expand Down Expand Up @@ -915,6 +918,7 @@ macro_rules! cursor_matches {
}};
}

#[allow(clippy::too_many_lines)]
pub fn get_comp_resp(
curr_doc: &str,
tree_entry: &mut TreeEntry,
Expand Down Expand Up @@ -1108,7 +1112,7 @@ pub fn get_comp_resp(
None
}

fn lsp_pos_of_point(pos: tree_sitter::Point) -> lsp_types::Position {
const fn lsp_pos_of_point(pos: tree_sitter::Point) -> lsp_types::Position {
Position {
line: pos.row as u32,
character: pos.column as u32,
Expand Down
140 changes: 71 additions & 69 deletions src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ fn parse_arm_alias(xml_contents: &str) -> Result<Option<(InstructionAlias, Strin
QName(b"instructionsection") => {
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if let Ok("title") = str::from_utf8(key.into_inner()) {
if Ok("title") == str::from_utf8(key.into_inner()) {
alias.title = (unsafe { str::from_utf8_unchecked(&value) }).to_string();
}
}
Expand All @@ -501,20 +501,18 @@ fn parse_arm_alias(xml_contents: &str) -> Result<Option<(InstructionAlias, Strin
}
}
Ok(Event::Empty(ref e)) => {
if let QName(b"docvar") = e.name() {
if QName(b"docvar") == e.name() {
let mut alias_next = false;
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if alias_next {
if let Ok("value") = str::from_utf8(key.into_inner()) {
aliased_instr = Some(str::from_utf8(&value)?.to_owned());
break;
}
if alias_next && Ok("value") == str::from_utf8(key.into_inner()) {
aliased_instr = Some(str::from_utf8(&value)?.to_owned());
break;
}
if let Ok("key") = str::from_utf8(key.into_inner()) {
if let Ok("alias_mnemonic") = str::from_utf8(&value) {
alias_next = true;
}
if Ok("key") == str::from_utf8(key.into_inner())
&& Ok("alias_mnemonic") == str::from_utf8(&value)
{
alias_next = true;
}
}
}
Expand Down Expand Up @@ -584,15 +582,15 @@ fn parse_arm_instruction(xml_contents: &str) -> Result<Option<Instruction>> {
},
Ok(Event::Empty(ref e)) => {
// e.g. <docvar key="mnemonic" value="ABS"/>
if let QName(b"docvar") = e.name() {
if QName(b"docvar") == e.name() {
// There are multiple entries like this in each opcode file, but
// *all* of them are the same within each file, so it doesn't matter which
// one we use
if instruction.name.is_empty() {
let mut mnemonic_next = false;
for attr in e.attributes() {
let Attribute { key: _, value } = attr.unwrap();
if let Ok("mnemonic") = str::from_utf8(&value) {
if Ok("mnemonic") == str::from_utf8(&value) {
mnemonic_next = true;
} else if mnemonic_next {
let name =
Expand Down Expand Up @@ -662,6 +660,7 @@ fn parse_arm_instruction(xml_contents: &str) -> Result<Option<Instruction>> {
///
/// This function is highly specialized to parse a handful of files and will panic or return
/// `Err` for most mal-formed/unexpected inputs
#[allow(clippy::too_many_lines)]
pub fn populate_instructions(xml_contents: &str) -> Result<Vec<Instruction>> {
// initialise the instruction set
let mut instructions_map = HashMap::<String, Instruction>::new();
Expand All @@ -683,7 +682,7 @@ pub fn populate_instructions(xml_contents: &str) -> Result<Vec<Instruction>> {
QName(b"InstructionSet") => {
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if let Ok("name") = str::from_utf8(key.into_inner()) {
if Ok("name") == str::from_utf8(key.into_inner()) {
arch = Arch::from_str(unsafe { str::from_utf8_unchecked(&value) })
.ok();
} else {
Expand Down Expand Up @@ -1053,50 +1052,6 @@ pub fn populate_name_to_instruction_map<'instruction>(
}
}

#[cfg(test)]
mod tests {
use mockito::ServerOpts;

use crate::parser::{get_cache_dir, populate_instructions};
#[test]
fn test_populate_instructions() {
let mut server = mockito::Server::new_with_opts(ServerOpts {
port: 8080,
..Default::default()
});

let _ = server
.mock("GET", "/x86/")
.with_status(200)
.with_header("content-type", "text/html")
.with_body(include_str!(
"../docs_store/instr_info_cache/x86_instr_docs.html"
))
.create();

// Need to clear the cache file (if there is one)
// to ensure a request is made for each test call
let mut x86_cache_path = get_cache_dir().unwrap();
x86_cache_path.push("x86_instr_docs.html");
if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
let xml_conts_x86 = include_str!("../docs_store/opcodes/raw/x86.xml");
assert!(populate_instructions(xml_conts_x86).is_ok());

if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
let xml_conts_x86_64 = include_str!("../docs_store/opcodes/raw/x86_64.xml");
assert!(populate_instructions(xml_conts_x86_64).is_ok());

// Clean things up so we don't have an empty cache file
if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
}
}

/// Parse the provided XML contents and return a vector of all the registers based on that.
/// If parsing fails, the appropriate error will be returned instead.
///
Expand All @@ -1112,6 +1067,7 @@ mod tests {
///
/// This function is highly specialized to parse a handful of files and will panic or return
/// `Err` for most mal-formed/unexpected inputs
#[allow(clippy::too_many_lines)]
pub fn populate_registers(xml_contents: &str) -> Result<Vec<Register>> {
let mut registers_map = HashMap::<String, Register>::new();

Expand All @@ -1132,7 +1088,7 @@ pub fn populate_registers(xml_contents: &str) -> Result<Vec<Register>> {
QName(b"InstructionSet") => {
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if let Ok("name") = str::from_utf8(key.into_inner()) {
if Ok("name") == str::from_utf8(key.into_inner()) {
arch = Arch::from_str(unsafe { str::from_utf8_unchecked(&value) })
.ok();
}
Expand Down Expand Up @@ -1184,7 +1140,7 @@ pub fn populate_registers(xml_contents: &str) -> Result<Vec<Register>> {
}
}
}
QName(b"Flags") => {} // it's just a wrapper...
//QName(b"Flags") => {} // it's just a wrapper...
// Actual flag bit info
QName(b"Flag") => {
curr_bit_flag = RegisterBitInfo::default();
Expand Down Expand Up @@ -1327,9 +1283,9 @@ pub fn populate_masm_nasm_directives(xml_contents: &str) -> Result<Vec<Directive
}
// end event --------------------------------------------------------------------------
Ok(Event::End(ref e)) => {
if let QName(b"directive") = e.name() {
if QName(b"directive") == e.name() {
directives_map.insert(curr_directive.name.clone(), curr_directive.clone());
} else if let QName(b"description") = e.name() {
} else if QName(b"description") == e.name() {
in_desc = false;
}
}
Expand Down Expand Up @@ -1381,7 +1337,7 @@ pub fn populate_gas_directives(xml_contents: &str) -> Result<Vec<Directive>> {
QName(b"Assembler") => {
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if let Ok("name") = str::from_utf8(key.into_inner()) {
if Ok("name") == str::from_utf8(key.into_inner()) {
assembler = Assembler::from_str(unsafe {
str::from_utf8_unchecked(&value)
})
Expand Down Expand Up @@ -1426,11 +1382,11 @@ pub fn populate_gas_directives(xml_contents: &str) -> Result<Vec<Directive>> {
}
}
}
QName(b"Signatures") => {} // it's just a wrapper...
//QName(b"Signatures") => {} // it's just a wrapper...
QName(b"Signature") => {
for attr in e.attributes() {
let Attribute { key, value } = attr.unwrap();
if let Ok("sig") = str::from_utf8(key.into_inner()) {
if Ok("sig") == str::from_utf8(key.into_inner()) {
let sig = String::from(unsafe { str::from_utf8_unchecked(&value) });
curr_directive
.signatures
Expand All @@ -1443,7 +1399,7 @@ pub fn populate_gas_directives(xml_contents: &str) -> Result<Vec<Directive>> {
}
// end event --------------------------------------------------------------------------
Ok(Event::End(ref e)) => {
if let QName(b"Directive") = e.name() {
if QName(b"Directive") == e.name() {
// finish directive
directives_map.insert(curr_directive.name.clone(), curr_directive.clone());
}
Expand Down Expand Up @@ -1546,9 +1502,11 @@ fn get_docs_body(x86_online_docs: &str) -> Option<String> {
Some(body)
}

/// Searches for the asm-lsp cache directory. First checks for the `ASM_LSP_CACHE_DIR`
/// environment variable. If this variable is present and points to a valid directory,
/// this path is returned. Otherwise, the function returns `~/.config/asm-lsp/`
/// Searches for the asm-lsp cache directory.
///
/// - First checks for the `ASM_LSP_CACHE_DIR` environment variable. If this variable
/// is present and points to a valid directory, this path is returned.
/// - Otherwise, the function returns `~/.config/asm-lsp/`
///
/// # Errors
///
Expand Down Expand Up @@ -1626,3 +1584,47 @@ fn set_x86_docs_cache(contents: &str, x86_cache_path: &PathBuf) {
}
}
}

#[cfg(test)]
mod tests {
use mockito::ServerOpts;

use crate::parser::{get_cache_dir, populate_instructions};
#[test]
fn test_populate_instructions() {
let mut server = mockito::Server::new_with_opts(ServerOpts {
port: 8080,
..Default::default()
});

let _ = server
.mock("GET", "/x86/")
.with_status(200)
.with_header("content-type", "text/html")
.with_body(include_str!(
"../docs_store/instr_info_cache/x86_instr_docs.html"
))
.create();

// Need to clear the cache file (if there is one)
// to ensure a request is made for each test call
let mut x86_cache_path = get_cache_dir().unwrap();
x86_cache_path.push("x86_instr_docs.html");
if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
let xml_conts_x86 = include_str!("../docs_store/opcodes/raw/x86.xml");
assert!(populate_instructions(xml_conts_x86).is_ok());

if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
let xml_conts_x86_64 = include_str!("../docs_store/opcodes/raw/x86_64.xml");
assert!(populate_instructions(xml_conts_x86_64).is_ok());

// Clean things up so we don't have an empty cache file
if x86_cache_path.is_file() {
std::fs::remove_file(&x86_cache_path).unwrap();
}
}
}
Loading

0 comments on commit b477aa6

Please sign in to comment.