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

goto-def on e.index() wrongly goes to the Index trait #5353

Closed
cuviper opened this issue Jul 13, 2020 · 7 comments
Closed

goto-def on e.index() wrongly goes to the Index trait #5353

cuviper opened this issue Jul 13, 2020 · 7 comments
Labels
A-ty type system / type inference / traits / method resolution

Comments

@cuviper
Copy link
Member

cuviper commented Jul 13, 2020

In indexmap's map::tests::entry, two e.index() calls are being associated with the wrong method, here and here. They go to core::ops::Index::index, when they ought to go to Entry::index. While this code does have ops::Index in scope, inherent methods should always take precedence.

For posterity, note those links are specifically using commit indexmap-rs/indexmap@299d950.

I only noticed this after updating to release 2020-07-13, because it started warning "Expected 1 argument, found 0" per #5270. However, I did confirm that goto-def was wrong on those calls with 2020-07-06 too. I guess with the new warning, it increases the expectation that method resolution must be correct to avoid false errors.

(I'm using Vim+ALE, but I doubt that specifically makes any difference.)

@flodiebold
Copy link
Member

Hm. Inherent methods should resolve without problems usually. Is the type of e inferred correctly? Is the entry method call resolved?

@flodiebold flodiebold added the A-ty type system / type inference / traits / method resolution label Jul 14, 2020
@cuviper
Copy link
Member Author

cuviper commented Jul 14, 2020

Is the type of e inferred correctly?

It seems not, and it doesn't even figure out the type of the map. If I turbofish that call, IndexMap::<i32, &str>::new(), it still doesn't work. If I annotate the map itself, even just the key type like let mut map: IndexMap<i32, _> = ..., then everything else falls into place, including the right goto-def on e.index().

Is the entry method call resolved?

Sort of -- in Vim it jumps to the right function, but in VSCode it pops up a menu to choose IndexMap::entry or that test fn entry() itself. I guess that's not truly resolved, just guessing candidates.

@bjorn3
Copy link
Member

bjorn3 commented Jul 14, 2020

If you remove the #[cfg(test)] from the test module does it work?

@cuviper
Copy link
Member Author

cuviper commented Jul 14, 2020

If you remove the #[cfg(test)] from the test module does it work?

No effect.

@flodiebold
Copy link
Member

flodiebold commented Jul 14, 2020

IndexMap::new is behind #[cfg(has_std)]. I'm not sure where that has_std comes from, I'm guessing the build.rs. Do you have loadOutDirsFromCheck enabled?

(You could also try removing that #[cfg(has_std)] to confirm my suspicion that that's causing the problem.)

@cuviper
Copy link
Member Author

cuviper commented Jul 14, 2020

Ah, yes, that's detected in build.rs -- when I enable that setting in VSCode, it works!

Now if I can figure out the right way to set that for ALE...

@cuviper
Copy link
Member Author

cuviper commented Jul 14, 2020

Regarding ALE, it seems there was a PR for "workspace/configuration" that was dropped:
dense-analysis/ale#3130

But we can consider this immediate issue resolved, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution
Projects
None yet
Development

No branches or pull requests

3 participants