Shorten type hints for std::iter Iterators #6154

Veykril · 2020-10-06T17:10:39Z

This re-exports the hir_expand::name::known module to be able to fetch the Iterator and iter names.
I'm not sure if there is anything to do with Solution::Ambig in normalize_trait_assoc_type or whether discarding those results is always wanted.

matklad · 2020-10-06T17:48:13Z

crates/ide/src/inlay_hints.rs

+    let iter_trait = module.scope(db, None).into_iter().find_map(|(name, def)| match def {
+        hir::ScopeDef::ModuleDef(ModuleDef::Trait(r#trait)) if name == known::Iterator => {
+            Some(r#trait)
+        }
+        _ => None,
+    })?;


Rather then iterating through the scope, we should ask for Iterator directly. Currently, this handled by the FamousDefs type in assists. It is somewhat ad-hoc, long term, I think we need to make use of rustc_diagnostic_item attribute, but, for now, using FamousDefs as an API is the way to go!

FamousDefs is crate private, I assume making it pub for this should be alright?

I had to change a thing in FamousDefs aside from the visibility. It now also checks the crate you directly pass to it whether that is std/core, due to the crate the hint has already being std/core.

And the FIXTURE const can't be cfg(test) gated anymore for some reason. The cfg just doesn't seem to work properly across crate boundaries?

crates/ide/src/inlay_hints.rs

matklad · 2020-10-06T17:49:38Z

cc @SomeoneToIgnore

matklad · 2020-10-06T18:26:01Z

yup!

…

On Tue, 6 Oct 2020 at 20:25, Lukas Wirth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ide/src/inlay_hints.rs <#6154 (comment)> : > + let iter_trait = module.scope(db, None).into_iter().find_map(|(name, def)| match def { + hir::ScopeDef::ModuleDef(ModuleDef::Trait(r#trait)) if name == known::Iterator => { + Some(r#trait) + } + _ => None, + })?; FamousDefs is crate private, I assume making it pub for this should be alright? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MYLSKY4ELYY4SQN2Y3SJNOHZANCNFSM4SGJX2CQ> .

Veykril · 2020-10-06T19:13:53Z

Is there a better way to check this? https://github.com/rust-analyzer/rust-analyzer/blob/821a55ff2f0c215e78dfbb29838bc0572f72e321/crates/ide/src/inlay_hints.rs#L224-L230

Veykril · 2020-10-06T19:26:50Z

So sorry for the billion force-pushes I keep forgetting to run cargo test -p xtask to check for whitespaces 😅

matklad · 2020-10-06T19:33:25Z

no worries, force-pushes are OK!

…

On Tue, 6 Oct 2020 at 21:27, Lukas Wirth ***@***.***> wrote: So sorry for the billion force-pushes I keep forgetting to run cargo test -p xtask to check for whitespaces 😅 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M6U7BS44JU5IJMTS6TSJNVQXANCNFSM4SGJX2CQ> .

SomeoneToIgnore

Looks like a great step forward!

One interesting finding:

I wonder, if this is caused by some internal limit being too low?

crates/ide/src/inlay_hints.rs

SomeoneToIgnore · 2020-10-06T23:02:31Z

crates/ide/src/inlay_hints.rs

+        .path_to_root(db)
+        .into_iter()
+        .rev()
+        .find(|module| module.name(db) == Some(known::iter))?;


The way we use known::item here and core_iter_Iterator() is interesting.
We seem to use more or less the same semantics here (some predefined knowledge about sdlib), both using hir inside, yet in totally different parts of the code.

Any thoughts? (maybe @matklad ?)

I suppose we could add module lookup to FamousDefs and then check if the module exports the type or something along those lines? Then we wouldn't have to use known here I think

Yeah, I am not sure what this should look like, but funneling everything via FamousDefs seems prudent for future refactorability.

I think the right worklist here:

make sure that IDE bits only use FamousDefs

replace FamousDefs::FIXTURE with more fine-grained opt-ins at the fixture level (we want to have syntax like //- core with:Iterator,IntoIterator)

try to replace as much of path lookup as possible with lang-items and diagnostics items, but keep an eye on ide/compiler split

For this PR, only keeping everything scoped to FamousDefs should be sufficient.

SomeoneToIgnore · 2020-10-06T23:32:12Z

Is there a better way to check this?

(I cannot reply to this question directly due to force pushes).
I believe the majority of this method will mutate into a more high-level function, returning the list of all impl Trait possible for the type given.
Yet this might not be feasible to fix now, before we decide what to do with the fact that we need 2 totally different places in the code to define Iter trait related data.

crates/ide/src/inlay_hints.rs

Veykril · 2020-10-07T09:50:24Z

The iter structs comes from two module definitions, core::iter::sources and core::iter::adapters, so one possibility would be to check the types declaration module for these two, the other option would be to check if core::iter re-exports the type(which I am kinda doing there right now, just not in a nice way in my eyes). Hence my question if there was a better way to do this.

Veykril · 2020-10-07T10:02:46Z

I wonder, if this is caused by some internal limit being too low?

This seems to be chalk failing at that depth, it returns Solution::Ambig with Unknown at that point.

Veykril · 2020-10-07T11:15:46Z

Okay I think everything so far got addressed now 👍

SomeoneToIgnore · 2020-10-07T11:51:49Z

Thanks for the fixes, the same place in RA now looks way nicer:

Interesting that there are hints with the bloated Filter and FilterMap in the associated types, yet the Chalk limit is clearly not reached by this time.
Maybe we need to apply the same hint_iterator logic once more to the associated type.
But that can land in a different PR, I think now we have a good set up to iterate from already, thank you.

Chalk limits is also an interesting thing to tackle, the chaining hints loose their new appearance too fast IMO.
But I think we'd better to wait for issues on the topic with good test examples :)

bors r+

bors · 2020-10-07T11:57:29Z

Build succeeded:

Shorten type hints for std::iter Iterators

aaa3905

Veykril force-pushed the iter-hints branch from ffe66cf to aaa3905 Compare October 6, 2020 17:20

matklad reviewed Oct 6, 2020

View reviewed changes

crates/ide/src/inlay_hints.rs Outdated Show resolved Hide resolved

Veykril force-pushed the iter-hints branch 3 times, most recently from 821a55f to 4a39de1 Compare October 6, 2020 19:12

Veykril force-pushed the iter-hints branch from 4a39de1 to a3ca226 Compare October 6, 2020 19:18

Use FamousDefs for shorten_iterator hint

c6f1de6

Veykril force-pushed the iter-hints branch from a3ca226 to c6f1de6 Compare October 6, 2020 19:25

SomeoneToIgnore reviewed Oct 6, 2020

View reviewed changes

Veykril added 2 commits October 7, 2020 10:14

Move IntoIterator into FamousDefs

c133651

Shorten iterator chain hints

209e9b9

Veykril force-pushed the iter-hints branch from 20bb9eb to 209e9b9 Compare October 7, 2020 09:30

Veykril commented Oct 7, 2020

View reviewed changes

crates/ide/src/inlay_hints.rs Outdated Show resolved Hide resolved

Shorten iterator hints for std::iter iterators behind references

e106857

Veykril force-pushed the iter-hints branch from a4b1cab to e106857 Compare October 7, 2020 10:14

Clean up inlay_hints

783af17

Veykril force-pushed the iter-hints branch from d87a01b to 783af17 Compare October 7, 2020 11:18

bors bot merged commit 83a651b into rust-lang:master Oct 7, 2020

SomeoneToIgnore added the hacktoberfest-accepted label Oct 7, 2020

Veykril mentioned this pull request Feb 12, 2021

Show common iterators as impl Iterator within nested types #7647

Open

rami3l mentioned this pull request Dec 8, 2022

Improve type hints for long/deeply nested types (parser combinators etc.) #11240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shorten type hints for std::iter Iterators #6154

Shorten type hints for std::iter Iterators #6154

Veykril commented Oct 6, 2020

matklad Oct 6, 2020

Veykril Oct 6, 2020

Veykril Oct 6, 2020

Veykril Oct 6, 2020

matklad commented Oct 6, 2020

matklad commented Oct 6, 2020 via email

Veykril commented Oct 6, 2020

Veykril commented Oct 6, 2020

matklad commented Oct 6, 2020 via email

SomeoneToIgnore left a comment

SomeoneToIgnore Oct 6, 2020

Veykril Oct 7, 2020

matklad Oct 7, 2020

SomeoneToIgnore commented Oct 6, 2020 •

edited

Loading

Veykril commented Oct 7, 2020

Veykril commented Oct 7, 2020 •

edited

Loading

Veykril commented Oct 7, 2020

SomeoneToIgnore commented Oct 7, 2020 •

edited

Loading

bors bot commented Oct 7, 2020

Shorten type hints for std::iter Iterators #6154

Shorten type hints for std::iter Iterators #6154

Conversation

Veykril commented Oct 6, 2020

matklad Oct 6, 2020

Choose a reason for hiding this comment

Veykril Oct 6, 2020

Choose a reason for hiding this comment

Veykril Oct 6, 2020

Choose a reason for hiding this comment

Veykril Oct 6, 2020

Choose a reason for hiding this comment

matklad commented Oct 6, 2020

matklad commented Oct 6, 2020 via email

Veykril commented Oct 6, 2020

Veykril commented Oct 6, 2020

matklad commented Oct 6, 2020 via email

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

SomeoneToIgnore Oct 6, 2020

Choose a reason for hiding this comment

Veykril Oct 7, 2020

Choose a reason for hiding this comment

matklad Oct 7, 2020

Choose a reason for hiding this comment

SomeoneToIgnore commented Oct 6, 2020 • edited Loading

Veykril commented Oct 7, 2020

Veykril commented Oct 7, 2020 • edited Loading

Veykril commented Oct 7, 2020

SomeoneToIgnore commented Oct 7, 2020 • edited Loading

bors bot commented Oct 7, 2020

SomeoneToIgnore commented Oct 6, 2020 •

edited

Loading

Veykril commented Oct 7, 2020 •

edited

Loading

SomeoneToIgnore commented Oct 7, 2020 •

edited

Loading