Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add quickfix for redundant_assoc_item diagnostic #16223

Merged
merged 3 commits into from
Jan 5, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 184 additions & 28 deletions crates/ide-diagnostics/src/handlers/trait_impl_redundant_assoc_item.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
use hir::{Const, Function, HasSource, TypeAlias};
use ide_db::base_db::FileRange;
use hir::{db::ExpandDatabase, Const, Function, HasSource, HirDisplay, TypeAlias};
use ide_db::{
assists::{Assist, AssistId, AssistKind},
base_db::FileRange,
label::Label,
source_change::SourceChangeBuilder,
};
use syntax::{AstNode, SyntaxKind};
use text_edit::TextRange;

use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};

Expand All @@ -10,47 +17,196 @@ pub(crate) fn trait_impl_redundant_assoc_item(
ctx: &DiagnosticsContext<'_>,
d: &hir::TraitImplRedundantAssocItems,
) -> Diagnostic {
let db = ctx.sema.db;
let name = d.assoc_item.0.clone();
let redundant_assoc_item_name = name.display(db);
let assoc_item = d.assoc_item.1;
let db = ctx.sema.db;

let default_range = d.impl_.syntax_node_ptr().text_range();
let trait_name = d.trait_.name(db).to_smol_str();

let (redundant_item_name, diagnostic_range) = match assoc_item {
hir::AssocItem::Function(id) => (
format!("`fn {}`", name.display(db)),
Function::from(id)
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
),
hir::AssocItem::Const(id) => (
format!("`const {}`", name.display(db)),
Const::from(id)
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
),
hir::AssocItem::TypeAlias(id) => (
format!("`type {}`", name.display(db)),
TypeAlias::from(id)
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
),
let (redundant_item_name, diagnostic_range, redundant_item_def) = match assoc_item {
hir::AssocItem::Function(id) => {
let function = Function::from(id);
(
format!("`fn {}`", redundant_assoc_item_name),
function
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
format!("\n {};", function.display(db).to_string()),
)
}
hir::AssocItem::Const(id) => {
let constant = Const::from(id);
(
format!("`const {}`", redundant_assoc_item_name),
constant
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
format!("\n {};", constant.display(db).to_string()),
)
}
hir::AssocItem::TypeAlias(id) => {
let type_alias = TypeAlias::from(id);
(
format!("`type {}`", redundant_assoc_item_name),
type_alias
.source(db)
.map(|it| it.syntax().value.text_range())
.unwrap_or(default_range),
format!("\n type {};", type_alias.name(ctx.sema.db).to_smol_str()),
)
}
};

Diagnostic::new(
DiagnosticCode::RustcHardError("E0407"),
format!("{redundant_item_name} is not a member of trait `{trait_name}`"),
FileRange { file_id: d.file_id.file_id().unwrap(), range: diagnostic_range },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just realized I missed this on the review that added this diagnostic, but this unwrap is very bad 😅 We should be using adjusted_display_range_new to get the new range for this here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted_display_range_new require a diag_ptr: InFile<AstPtr<N>>, regarding get diag_ptr from the current hir::AssocItem, we could use hir::AssocItem.source(db), which return an Option<InFile<Self::Ast>>, could we unwarp or expect directly? since hir::AssocItem.source(db) actually return Some(...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also do something similar to adjusted_display_range_new since we are skipping the diag_ptr. And no unwrapping isn't an option here, the source fetching is technically fully infallible right now, but we might change things at some point where we allow loading libraries with missing sources at which point the unwrap would fail.

Copy link
Contributor

@saiintbrisson saiintbrisson Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't aware this was already discussed here. Opened an issue #16269.

)
.with_fixes(quickfix_for_redundant_assoc_item(
ctx,
d,
redundant_item_def,
diagnostic_range,
))
}

/// add assoc item into the trait def body
fn quickfix_for_redundant_assoc_item(
ctx: &DiagnosticsContext<'_>,
d: &hir::TraitImplRedundantAssocItems,
redundant_item_def: String,
range: TextRange,
) -> Option<Vec<Assist>> {
let add_assoc_item_def = |builder: &mut SourceChangeBuilder| -> Option<()> {
let db = ctx.sema.db;
let root = db.parse_or_expand(d.file_id);
// don't modify trait def in outer crate
let current_crate = ctx.sema.scope(&d.impl_.syntax_node_ptr().to_node(&root))?.krate();
let trait_def_crate = d.trait_.module(db).krate();
if trait_def_crate != current_crate {
return None;
}
let trait_def = d.trait_.source(db)?.value;
let where_to_insert = trait_def
.syntax()
.descendants_with_tokens()
.find(|it| it.kind() == SyntaxKind::L_CURLY)
.map(|it| it.text_range())?;
Young-Flash marked this conversation as resolved.
Show resolved Hide resolved

Some(builder.insert(where_to_insert.end(), redundant_item_def))
};
let file_id = d.file_id.file_id()?;
let mut source_change_builder = SourceChangeBuilder::new(file_id);
add_assoc_item_def(&mut source_change_builder)?;

Some(vec![Assist {
id: AssistId("add assoc item def into trait def", AssistKind::QuickFix),
label: Label::new("Add assoc item def into trait def".to_string()),
group: None,
target: range,
source_change: Some(source_change_builder.finish()),
trigger_signature_help: false,
}])
}

#[cfg(test)]
mod tests {
use crate::tests::check_diagnostics;
use crate::tests::{check_diagnostics, check_fix, check_no_fix};

#[test]
fn quickfix_for_assoc_func() {
check_fix(
r#"
trait Marker {
fn boo();
}
struct Foo;
impl Marker for Foo {
fn$0 bar(_a: i32, _b: String) -> String {}
fn boo() {}
}
"#,
r#"
trait Marker {
fn bar(_a: i32, _b: String) -> String;
fn boo();
}
struct Foo;
impl Marker for Foo {
fn bar(_a: i32, _b: String) -> String {}
fn boo() {}
}
"#,
)
}

#[test]
fn quickfix_for_assoc_const() {
check_fix(
r#"
trait Marker {
fn foo () {}
}
struct Foo;
impl Marker for Foo {
const FLAG: bool$0 = false;
}
"#,
r#"
trait Marker {
const FLAG: bool;
fn foo () {}
}
struct Foo;
impl Marker for Foo {
const FLAG: bool = false;
}
"#,
)
}

#[test]
fn quickfix_for_assoc_type() {
check_fix(
r#"
trait Marker {
}
struct Foo;
impl Marker for Foo {
type T = i32;$0
}
"#,
r#"
trait Marker {
type T;
}
struct Foo;
impl Marker for Foo {
type T = i32;
}
"#,
)
}

#[test]
fn quickfix_dont_work() {
check_no_fix(
r#"
//- /dep.rs crate:dep
trait Marker {
}
//- /main.rs crate:main deps:dep
struct Foo;
impl dep::Marker for Foo {
type T = i32;$0
}
"#,
)
}

#[test]
fn trait_with_default_value() {
Expand All @@ -64,12 +220,12 @@ trait Marker {
struct Foo;
impl Marker for Foo {
type T = i32;
//^^^^^^^^^^^^^ error: `type T` is not a member of trait `Marker`
//^^^^^^^^^^^^^ 💡 error: `type T` is not a member of trait `Marker`

const FLAG: bool = true;

fn bar() {}
//^^^^^^^^^^^ error: `fn bar` is not a member of trait `Marker`
//^^^^^^^^^^^ 💡 error: `fn bar` is not a member of trait `Marker`

fn boo() {}
}
Expand Down