-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: add quickfix for redundant_assoc_item diagnostic #16223
Conversation
.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 }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
crates/ide-diagnostics/src/handlers/trait_impl_redundant_assoc_item.rs
Outdated
Show resolved
Hide resolved
Thanks! |
@Veykril I guess you forgot the |
@bors r=Veykril |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
Happy New Year 😊
follow up #15990, now it's time to close #15958, closes #16269
EDIT: add a demo.gif would be more illustrated when making release change log.