feat: resolve inherent and implemented associated items in docs #15933

71 · 2023-11-19T12:51:36Z

This partially fixes #9694.

Supported:

Trait methods and constants.
- Due to resolution differences pointed out during the review of the PR, trait associated types are not supported.
Inherent methods, constants and associated types.
- Inherent associated types are a nightly feature, and are supported with no additional work in this PR.

Screenshot of VS Code running with the change:

You can see that the items are resolved (excl. trait associated types) since they are semantically highlighted in the doc comment.

Veykril · 2023-11-27T15:48:01Z

crates/hir-ty/src/method_resolution.rs

+            // Q: should this branch be restricted to `iterate_trait_item_candidates()`?
+            //
+            // the code below does not seem to be called when adding tests to
+            // `method_resolution.rs`, so resolution of type aliases is likely performed at some
+            // other point. due to the marks below, however, the test which ensures that marks have
+            // corresponding checks will fail


inherent type aliases aren't implemented in r-a, so it would not surprise me that the checks here are unreachable in inherent impls

Veykril · 2023-11-27T15:52:17Z

crates/ide-completion/src/completions/pattern.rs

@@ -154,7 +154,6 @@ pub(crate) fn complete_pattern_path(
                    }

                    ctx.iterate_path_candidates(&ty, |item| match item {
-                        AssocItem::TypeAlias(ta) => acc.add_type_alias(ctx, ta),


Why is this gone now?

Without removing this, a couple of snapshot tests fail because completions for type aliases are emitted several times / duplicated. I figured this code was a added to work around the fact that method_resolution.rs did not consider TypeAliases, which changed in this PR, so I removed these explicit iterations over TypeAliases.

Same for all your other comments for changes in src/completions/*. This is the main thing I'm uncertain about in this PR; it looks like adding support for TypeAliases had impacts in other parts of the code (which led me to make these changes, hence the question "should be only handle TypeAliases when resolving doc comments" above).

With that said, it looks like handling TypeAliases in a generic way in method_resolution.rs allows us to explicitly handle them in other parts of the codebase, so this may be a good thing. I am not aware of why it was handled differently before, though; it's possible that there were strong reasons for handling it differently which this PR breaks (subtly enough that additional tests do not fail).

For instance if I undo this particular diff, I get the following test failure:

---- tests::pattern::enum_qualified stdout ---- error: expect test failed --> crates/ide-completion/src/tests/pattern.rs:314:9 You can update all `expect!` tests by running: env UPDATE_EXPECT=1 cargo test To update a single test, place the cursor on `expect` token and use `run` feature of rust-analyzer. Expect: ---- ct ASSOC_CONST const ASSOC_CONST: () bn RecordV {…} RecordV { field$1 }$0 bn TupleV(…) TupleV($1)$0 bn UnitV UnitV$0 ---- Actual: ---- ct ASSOC_CONST const ASSOC_CONST: () ta AssocType type AssocType = () bn RecordV {…} RecordV { field$1 }$0 bn TupleV(…) TupleV($1)$0 bn UnitV UnitV$0 ---- Diff: ---- ct ASSOC_CONST const ASSOC_CONST: () ta AssocType type AssocType = () (NOTE: THIS LINE WAS ADDED) bn RecordV {…} RecordV { field$1 }$0 bn TupleV(…) TupleV($1)$0 bn UnitV UnitV$0 ----

For this particular case it looks like the problem is that the AssocType is wrongly suggested, but I do remember that for other changes there were duplicate completions.

Veykril · 2023-11-27T15:52:48Z

crates/ide-completion/src/completions/expr.rs

@@ -124,14 +111,6 @@ pub(crate) fn complete_expr_path(
                    ctx.iterate_path_candidates(&ty, |item| {
                        add_assoc_item(acc, item);
                    });
-
-                    // Iterate assoc types separately
-                    ty.iterate_assoc_items(ctx.db, ctx.krate, |item| {


Why is this removed now?

Veykril · 2023-11-27T15:53:12Z

crates/ide-completion/src/completions/expr.rs

        }
        Qualified::With { resolution: None, .. } => {}
        Qualified::With { resolution: Some(resolution), .. } => {
-            // Add associated types on type parameters and `Self`.
-            ctx.scope.assoc_type_shorthand_candidates(resolution, |_, alias| {


same question here

Veykril · 2023-11-27T15:53:17Z

crates/ide-completion/src/completions/expr.rs

@@ -67,22 +67,9 @@ pub(crate) fn complete_expr_path(
            ctx.iterate_path_candidates(ty, |item| {
                add_assoc_item(acc, item);
            });
-
-            // Iterate assoc types separately
-            ty.iterate_assoc_items(ctx.db, ctx.krate, |item| {


Veykril · 2023-11-27T15:53:47Z

crates/ide-completion/src/completions/type.rs

-        hir::AssocItem::Function(_) | hir::AssocItem::Const(_) => (),
-        hir::AssocItem::TypeAlias(ty) => acc.add_type_alias(ctx, ty),
+        hir::AssocItem::Function(_) | hir::AssocItem::Const(_) | hir::AssocItem::TypeAlias(_) => (),


why this change?

Veykril · 2023-11-27T15:58:29Z

crates/hir-ty/src/method_resolution.rs

+                }
+            }
+
+            // Q: is it correct to use the same check as `ConstId`?


Should be alright

Veykril

So (finally) revisiting this, it seems weird that method probing / resolution now also supports type aliases, as their resolution works a bit differently. So this seems to be the wrong way to approach this. We might just need to somehow expose a similar API but for type alias resolution that can be used for the doc resolution, I am unsure in what way though.

Veykril · 2024-01-03T13:58:50Z

Can you squash the last two commits into one? (Or all three up to you), the last commit will noise up git blame unnecessarily.

71 · 2024-01-03T14:03:17Z

I removed support for type aliases and tried reusing existing features more than before, which significantly simplified the PR.

Veykril · 2024-01-03T14:25:10Z

Thanks! I've wanted this for a long time.
@bors r+

bors · 2024-01-03T14:25:13Z

📌 Commit fe6f931 has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-01-03T14:28:40Z

⌛ Testing commit fe6f931 with merge c3c07c6...

bors · 2024-01-03T14:39:07Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing c3c07c6 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2023

71 marked this pull request as ready for review November 19, 2023 13:09

Veykril reviewed Nov 27, 2023

View reviewed changes

71 requested a review from Veykril November 29, 2023 12:49

Veykril mentioned this pull request Dec 5, 2023

Items Linking rendering in doc comments is not sufficient #16021

Closed

Veykril requested changes Jan 2, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2024

71 force-pushed the inherent-items-in-docs branch from 4b6b0b7 to 13e91d5 Compare January 3, 2024 13:54

hir: resolve associated items in docs (excl. type aliases)

fe6f931

71 force-pushed the inherent-items-in-docs branch from 13e91d5 to fe6f931 Compare January 3, 2024 14:04

71 requested a review from Veykril January 3, 2024 14:05

Veykril approved these changes Jan 3, 2024

View reviewed changes

bors merged commit c3c07c6 into rust-lang:master Jan 3, 2024
10 checks passed

philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#436

Merged

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve inherent and implemented associated items in docs #15933

feat: resolve inherent and implemented associated items in docs #15933

71 commented Nov 19, 2023 •

edited

Loading

Veykril Nov 27, 2023

Veykril Nov 27, 2023

71 Nov 27, 2023 •

edited

Loading

71 Nov 27, 2023

Veykril Nov 27, 2023

Veykril Nov 27, 2023

Veykril Nov 27, 2023

Veykril Nov 27, 2023

Veykril Nov 27, 2023

Veykril left a comment

Veykril commented Jan 3, 2024

71 commented Jan 3, 2024

Veykril commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

feat: resolve inherent and implemented associated items in docs #15933

feat: resolve inherent and implemented associated items in docs #15933

Conversation

71 commented Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

71 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril left a comment

Choose a reason for hiding this comment

Veykril commented Jan 3, 2024

71 commented Jan 3, 2024

Veykril commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

71 commented Nov 19, 2023 •

edited

Loading

71 Nov 27, 2023 •

edited

Loading