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

draft: fix: unlinked-file diagnostic handling #19398

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljahier
Copy link

@ljahier ljahier commented Mar 20, 2025

Fixing the FIXME related to autofix detection in unlinked_file

This PR attempts to address the following FIXME:

    // FIXME: This is a hack for the vscode extension to notice whether there is an autofix or not before having to resolve diagnostics.
    // This is to prevent project linking popups from appearing when there is an autofix. https://github.com/rust-lang/rust-analyzer/issues/14523

(See: [unlinked_file.rs#L32](https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-diagnostics/src/handlers/unlinked_file.rs#L32))

Hypothesis: ghost crate detection

During this fix, I noticed that certain files appear to be detected as unlinked due to a potential ghost crate issue. This could happen when a file is technically part of a project but isn't properly linked to a valid crate root (main.rs or lib.rs).

Previously, the check was relying on a workaround, likely to ensure that VS Code’s extension can determine the presence of autofix suggestions early. This PR refines that behavior by properly verifying:

  • Whether the file belongs to a crate (is_in_crate)
  • Whether it is the crate root (is_crate_root)
  • If it should be considered unlinked (is_unlinked = !is_in_crate || is_crate_root)

This approach helps avoid false positives where a file may be mistakenly marked as unlinked while still being part of a phantom crate.

Current Status :
🚧 This PR is currently in draft mode while I investigate and resolve the issue related to ghost crates. Once the behavior is fully understood and properly handled, I will mark it as ready for review.

Further testing is required to confirm whether this resolves all related issues without introducing regressions. Feedback and additional test cases are welcome !

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2025
@ljahier ljahier marked this pull request as draft March 20, 2025 00:02
@ljahier ljahier changed the title draft: fix: unlinked-file diagnostic handling but maybe a case of ghost crate draft: fix: unlinked-file diagnostic handling Mar 20, 2025
Copy link
Author

@ljahier ljahier left a comment

Choose a reason for hiding this comment

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

When running cargo test -p ide-diagnostics I get the following output :

---- handlers::unlinked_file::tests::unlinked_file_respects_unused_flag stdout ----

[DEBUG] file_id: FileId(1), is_in_crate: true, is_crate_root: false, is_unlinked: false
[DEBUG] file_id: FileId(1), fixes: Some([Assist { id: AssistId("add_mod_declaration", QuickFix), label: "Insert `mod foo;`", group: None, target: 0..161, source_change: Some(SourceChange { source_file_edits: {FileId(0): (TextEdit { indels: [Indel { insert: "mod foo;\n\n", delete: 0..0 }], annotation: None }, None)}, file_system_edits: [], is_snippet: false, annotations: {}, next_annotation_id: 0 }), command: None }, Assist { id: AssistId("add_pub_mod_declaration", QuickFix), label: "Insert `pub mod foo;`", group: None, target: 0..161, source_change: Some(SourceChange { source_file_edits: {FileId(0): (TextEdit { indels: [Indel { insert: "pub mod foo;\n\n", delete: 0..0 }], annotation: None }, None)}, file_system_edits: [], is_snippet: false, annotations: {}, next_annotation_id: 0 }), command: None }]), has_fixes: true

This test output suggests that the file is detected as being in a crate (is_in_crate = true) but not a crate root (is_crate_root = false), and yet it is not considered unlinked (is_unlinked = false). However, fixes are still being suggested, which raises the question of why these autofixes are appearing.

This behavior aligns with my hypothesis of a ghost crate detection issue:

Some files may be falsely classified as part of a crate even though they aren't properly linked to a valid root (main.rs or lib.rs).
The presence of autofix suggestions despite is_unlinked = false suggests that the system may be detecting an implicit "ghost" state.

.unwrap_or(false);

let is_unlinked = !is_in_crate || is_crate_root;

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
eprintln!(
"[DEBUG] file_id: {:?}, is_in_crate: {}, is_crate_root: {}, is_unlinked: {}",
file_id, is_in_crate, is_crate_root, is_unlinked
);

let fixes = if is_unlinked { None } else { fixes(ctx, file_id, range) };

let has_fixes = fixes.is_some();

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
eprintln!("[DEBUG] file_id: {:?}, fixes: {:?}, has_fixes: {}", file_id, fixes, has_fixes);

@ljahier
Copy link
Author

ljahier commented Mar 20, 2025

I attempted to fix the issue by modifying the crate detection logic with the following snippet:

let is_in_crate = check_crate.as_ref().map_or(false, |krate| {
    let root_file_id = krate.root_module().definition_source(ctx.sema.db).file_id;
    if root_file_id == file_id {
        false
    } else {
        let def_map = ctx.sema.db.crate_def_map((*krate).into());
        def_map.modules.iter().any(|(_, module)| {
            let mod_file_id = module.definition_source(ctx.sema.db).file_id;
            mod_file_id == file_id
        })
    }
});

However, this change breaks several other tests. It seems that the "ghost crate" behavior, where a crate is detected even if it isn’t fully linked (e.g., due to missing Cargo.toml or main.rs/lib.rs), is expected by the current test setup. This leads me to believe that the current behavior might be intentional to support certain VSCode extension hacks, as mentioned in the issue.

My primary goal is to address the issues I experience when rust-analyzer’s indexing is off, but it seems that altering this detection logic may have wider consequences. I’d appreciate feedback on whether we should adjust this behavior or if there might be an alternative approach to resolve the indexing issues without breaking the expected behavior in other tests.

Thanks in advance for your insights !

@Veykril
Copy link
Member

Veykril commented Apr 6, 2025

During this fix, I noticed that certain files appear to be detected as unlinked due to a potential ghost crate issue. This could happen when a file is technically part of a project but isn't properly linked to a valid crate root (main.rs or lib.rs).

I don't quite understand the issue. For one, what is a "ghost crate"? Do you have an example of the issue you are encountering that you are trying to fix with this PR here?

.map(|krate| krate.root_module().definition_source(ctx.sema.db).file_id == file_id)
.unwrap_or(false);

let is_unlinked = !is_in_crate || is_crate_root;
Copy link
Member

Choose a reason for hiding this comment

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

I am confused by what you are doing here. If we end up with this diagnostic we have already figured out that the file is in fact unlinked so I am not sure what all of this is actually trying to compute here

@@ -28,39 +28,40 @@ pub(crate) fn unlinked_file(
file_id: FileId,
) {
let mut range = TextRange::up_to(ctx.sema.db.line_index(file_id).len());
let fixes = fixes(ctx, file_id, range);
// FIXME: This is a hack for the vscode extension to notice whether there is an autofix or not before having to resolve diagnostics.
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw this fixme isn't quite applicable anymore currently. The unlinked file handling the vscode client is completely disabled at the moment. (the vscode extension used to scan the content of the message here to do conditional work which is what this FIXME is referring to)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants