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

false positives when detecting errors in editor #9432

Closed
blueglyph opened this issue Sep 30, 2022 · 27 comments · Fixed by #9461
Closed

false positives when detecting errors in editor #9432

blueglyph opened this issue Sep 30, 2022 · 27 comments · Fixed by #9461
Assignees
Labels
bug subsystem::type inference & name resolution Issues related to name resolution and/or type inference

Comments

@blueglyph
Copy link

blueglyph commented Sep 30, 2022

Environment

  • IntelliJ Rust plugin version: 0.4.179.4903-222
  • Rust toolchain version: 1.64.0 (a55dd71d5 2022-09-19) x86_64-pc-windows-msvc
  • IDE name and version: IntelliJ IDEA 2022.2.2 Ultimate Edition (IU-222.4167.29)
  • Operating system: Windows 10 10.0
  • Macro expansion: enabled

Problem description

I get a lot of underwaved errors when editing Rust code, for

  • formatted strings, e.g. println! and similar macros, with the following explanation: "'(type)' doesn't implement 'Display', for types like String, usize, ...
  • arguments of generic functions with types that should normally be accepted, but are marked as wrong.

The program compiles without any error or warning, and runs without any problem.

A few examples:

  • 'usize' doesn't implement 'Display':
    image

  • 'String' doesn't implement 'Display': (the tooltip only shows the function signature, not the error)
    image

  • here, a function that takes any type from which f64 can be taken:
    image

  • (function signature)
    image

This makes IntelliJ's error detection unusable and is very annoying.

Misc info

Some of the errors disappear if I invalidate the cache and restart the IDE (File, Invalidate Caches...), but not all of them. Then they come back after a while.

I never saw that in 2022.2.1. I think I briefly used it with 1.64 and never saw those problems either. I have just updated to 2022.2.2 and now I'm seeing them all the time.

EDIT: work-around by reverting to previous version, see post below.

@helios2k6
Copy link

Yep, this is occurring for me as well. Seems like the new Rust plugin update borked much of the Rust plugin's ability to distinguish between genuine errors and false positives.

@bruno-ortiz
Copy link

[+1]

@bruno-ortiz
Copy link

Just to ask a question: Isn't this the case to rollback this version? Right now it's in a pretty unusable state.

@blueglyph
Copy link
Author

blueglyph commented Oct 1, 2022

The question is, revert the plugin, IntelliJ, or both?
Here's the plugin page URL if that helps, maybe I'll try later.

image

@blueglyph
Copy link
Author

Change log excerpt of version 0.4.179:

@blueglyph
Copy link
Author

I also noticed that in some conditions, after replacing strings in a selection, the selection would disappear (no text is selected after the replacement), making it tedious to do successive replacements. But I couldn't reproduce it. It does feel quite unstable.

I reverted to 178. For now I don't see those type problems anymore but there are still strange bugs when I copy / paste code, sometimes it escapes it when there is no reason to do so.

I was copying &[&[(Key, State, &str, &str, &[Value])]] as a base to define a type alias.

Maybe it's the usual IntelliJ instability. I have funny things regularly happening in Kotlin too, sometimes I have to reinstall everything after a thorough clean-up.

@blueglyph
Copy link
Author

blueglyph commented Oct 1, 2022

So far I haven't seen the problem since I reverted to Rust 0.4.178.4873-222, so I would say it's a good work around.

I think those type-related optimizations above may have broken something in the trait system in version 0.4.179. It seems unable to see what traits are supported by a type.

Procedure:

  • Visit this page and download the appropriate 0.4.178 version depending on your IntelliJ version: https://plugins.jetbrains.com/plugin/8182-rust/versions/stable
  • Open the settings and the plugin section.
  • Click on the setting cog wheel at the top, NOT the one in the plugin information area (see below).
  • Select Install Plugin from disk.

No need to uninstall it first. Maybe a Restart & Invalidate Caches may be safer, though I didn't need to do it.

image

@vlad20012
Copy link
Member

@blueglyph, @helios2k6, @bruno-ortiz, could you please attach your IDE logs? You can collect them using Help | Collect Logs and Diagnostic Data

@blueglyph
Copy link
Author

Hmm, I can't set the permissions so they are public.

I'll make a bug report on Jetbrain's Youtrack instead.

@Undin
Copy link
Member

Undin commented Oct 1, 2022

@blueglyph you can do it via https://uploads.jetbrains.com not to make them publicly available

@blueglyph
Copy link
Author

@blueglyph, @helios2k6, @bruno-ortiz, could you please attach your IDE logs? You can collect them using Help | Collect Logs and Diagnostic Data

Here's the reference:
Upload id: 2022_10_01_duRUfB3ggZ2ZazvPkm2ice (file: idea-logs-20221001-1436241054201450842933051.zip)

You can look at September 30th, the problem came back a few times. If that's easy for you to find, it happened before I did the first restart & invalidate, so I see one at 15h27. It came back several times later too, and it was pretty much permanent until the end of the day.

If it's too hard to spot I can clean the logs and try again but I'll have to update the plugin first. Or if I have to do something specific to help (custom properties, ...), just let me know.

@bruno-ortiz
Copy link

@vlad20012 here's the upload id:
Upload id: 2022_10_01_21Z4Htt3NBCiU1rY61qG5F (file: clion-logs-20221001-1123437637570799884844738.zip)

@helios2k6
Copy link

helios2k6 commented Oct 2, 2022

@blueglyph, @helios2k6, @bruno-ortiz, could you please attach your IDE logs? You can collect them using Help | Collect Logs and Diagnostic Data

@vlad20012: Upload id: 2022_10_02_YciksdcsAnSa12EKtW9Uhq (file: stdout_logs.log). Just FYI, I'm on Linux and using the Help > Collect Logs and Diagnostic Data menu item does nothing so I copied the logs from stdout.

@vlad20012
Copy link
Member

vlad20012 commented Oct 2, 2022

Any chance you work on some open source project? In this case, could you please link the project and mention the file you experience such false-positives? If project is not open-source, but it's not under NDA, may I ask you to upload it using https://uploads.jetbrains.com/ ?

@blueglyph
Copy link
Author

blueglyph commented Oct 3, 2022

@vlad20012
I may have found a similar error that is reproducible on a small piece of code. I removed many project files, so don't be surprised if some Cargo items are not relevant. It shouldn't be a problem though.

Code / Inspect code... will

  • produce 4 errors in 0.4.179 because it fails to see the proper U256::from trait, you can see the difference with a Ctrl-B on it (fixedpt.rs line 135) using both versions of the Rust plugin. The code compiles without any problem and all the tests pass.
  • be fine in 0.4.178.

I hope the root cause is common with what we saw. I didn't find anything more obvious, because the problems are hard to reproduce just by working in the IDE. If I re-install the new version, I don't see the errors immediately, I need to work on the project for a little while. So that's not something I can just hand you because it wouldn't be efficient. But I'd say you have a good chance to see the problem in any of your own projects if you're working a few hours with the new plugin version.

bug_0_4_179.zip

@vlad20012 vlad20012 self-assigned this Oct 3, 2022
@vlad20012
Copy link
Member

vlad20012 commented Oct 3, 2022

I managed to reproduce it! Thank you!

@blueglyph
Copy link
Author

Thanks for looking into this! :)

@helios2k6
Copy link

I managed to reproduce it! Thank you!

Amazing! Good to hear!

vlad20012 added a commit that referenced this issue Oct 4, 2022
Actually, the bug is a wrong (missing) implementation of CargoBasedCrate.equals/hashCode
@vlad20012
Copy link
Member

vlad20012 commented Oct 4, 2022

It was difficult, but I solved this riddle!
The bug was introduced in #9229 (included into 0.4.179 release).
I'm fixing it in #9461 that will likely be included into 0.4.180 (will be released next Monday).

Technical explanation:
CargoBasedCrate had identity-based equals/hashCode (looking ahead, this led to the bug). CrateGraphServiceImpl.crateGraph is stored under a CachedValue that stores a cached value indirectly under SoftReference ("indirectly" means SoftReference -> SomeWrapper -> CrateGraph), so Java GC can collect SomeWrapper anytime, that means invalidation of the the crateGraph cached value (and its re-calculation during a next access). Hence it's possible that at some point we have multiple CargoBasedCrate instances referring to the "same" crate. Crate.hasTransitiveDependencyOrSelf(Crate) (introduced in #9229) uses CargoBasedCrate.equals/hashCode in order to check if a crate is a dependency of another crate. The combination of these nuances has led to the fact that in some circumstances Crate.hasTransitiveDependencyOrSelf(Crate) could wrongly return false, that turns the #9229 filtering mechanism against valid impls (like impl Copy for primitives), that in turn leads to observable false-positives

@helios2k6
Copy link

It was difficult, but I solved this riddle! The bug was introduced in #9229 (included into 0.4.179 release). I'm fixing it in #9461 that will likely be included into 0.4.180 (will be released next Monday).

Technical explanation: CargoBasedCrate had identity-based equals/hashCode (looking ahead, this led to the bug). CrateGraphServiceImpl.crateGraph is stored under a CachedValue that stores a cached value indirectly under SoftReference ("indirectly" means SoftReference -> SomeWrapper -> CrateGraph), so Java GC can collect SomeWrapper anytime, that means invalidation of the the crateGraph cached value (and its re-calculation during a next access). Hence it's possible that at some point we have multiple CargoBasedCrate instances referring to the "same" crate. Crate.hasTransitiveDependencyOrSelf(Crate) (introduced in #9229) uses CargoBasedCrate.equals/hashCode in order to check if a crate is a dependency of another crate. The combination of these nuances has led to the fact that in some circumstances Crate.hasTransitiveDependencyOrSelf(Crate) could wrongly return false that turns the #9229 filtering mechanism against valid impls (like impl Copy for primitives) that in turn leads to observable false-positives

Remarkable! Great work!

@Undin Undin added bug subsystem::type inference & name resolution Issues related to name resolution and/or type inference labels Oct 4, 2022
@blueglyph
Copy link
Author

Well done, thanks for the quick resolution of this annoying issue!

@bors bors bot closed this as completed in 8826911 Oct 5, 2022
Undin pushed a commit that referenced this issue Oct 5, 2022
Actually, the bug is a wrong (missing) implementation of CargoBasedCrate.equals/hashCode

(cherry picked from commit f640f7b)
@vlad20012
Copy link
Member

The issue is fixed in the nightly plugin. You can try it now (see instructions)

Thank everyone for participating!

@TobiasDeBruijn
Copy link

TobiasDeBruijn commented Oct 22, 2022

I'm encountering something similar, I think, though I'm unsure if it's related to the issue.
I've got the following:

pub struct ControllerAuth<const CHECK_LOGIN: bool = true> {
    /// The id of the controller
    pub id: u32,
}

bool here is marked with a red squiggly line: bool doesn't derive both `PartialEq` and `Eq` [E0741]. Looking on docs.rs, bool certainly impls both PartialEq and Eq

Edit: Noticed that the issue is also present when used in the generic parameter for an impl block:

impl<const CHECK_LOGIN: bool> FromRequest for ControllerAuth<CHECK_LOGIN> {

@vlad20012
Copy link
Member

@TobiasDeBruijn Hi! It seems like something went wrong with macro expansion in your case, could you please try to perform Invalidate Caches?

@TobiasDeBruijn
Copy link

That indeed fixes it. Interestingly though, neither the struct nor the impl block have macros on them.

@vlad20012
Copy link
Member

@TobiasDeBruijn, PartialEq and Eq are implemented for bool via macros in the stdlib

@TobiasDeBruijn
Copy link

Ah in that way. Alright, thanks!

yopox pushed a commit to Kobzol/intellij-rust that referenced this issue Feb 27, 2023
…vity

Actually, the bug is a wrong (missing) implementation of CargoBasedCrate.equals/hashCode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug subsystem::type inference & name resolution Issues related to name resolution and/or type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants