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

Work around rust-analyzer false-positive type errors #111121

Merged
merged 1 commit into from
May 25, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented May 3, 2023

rust-analyzer incorrectly reports two type errors in debug.rs:

expected &dyn Display, found &i32
expected &dyn Display, found &i32

This is due to a known bug in r-a: (rust-lang/rust-analyzer#11847).

In these particular cases, changing &0 to &0i32 seems to be enough to avoid the bug.

@rustbot
Copy link
Collaborator

rustbot commented May 3, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented May 3, 2023

I'm not sure it's a good idea to work around bugs in our tools, but I'm not against it on principle.

@Zalathar
Copy link
Contributor Author

Zalathar commented May 3, 2023

Looking again at these call sites, I don’t think there’s any particular reason why the argument has to be the number zero, so it may be preferable to replace it with some opaque string instead.

@apiraino
Copy link
Contributor

How do people feel about this patch? Trying to figure out if we want to land this or rather should be tackled by r-a. thanks!

@Zalathar
Copy link
Contributor Author

A few other pieces of context:

  • The affected method has a few other call sites that also trigger the same issue (with a dummy 0 argument). This PR only touches the coverage-related ones because that’s what affects me personally, but in principle the other sites could get the same treatment.
  • The argument in question is only used as part of the filename of a dump file, so the affected code is pretty low-stakes overall.

@jackh726
Copy link
Member

Honestly, this is such a small, semantically equivalent change that ultimately doesn't hurt readability, so I'm going to r+ it.

In general though, I agree with oli that we shouldn't try to work around tooling issues, particularly if they impact readability or maintainability.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 24, 2023

📌 Commit 5494eec has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#111121 (Work around `rust-analyzer` false-positive type errors)
 - rust-lang#111759 (Leverage the interval property to precompute borrow kill points.)
 - rust-lang#111841 (Run AST validation on match guards correctly)
 - rust-lang#111862 (Split out opaque collection from from `type_of`)
 - rust-lang#111863 (Don't skip mir typeck if body has errors)
 - rust-lang#111903 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 09489b9 into rust-lang:master May 25, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 25, 2023
@Zalathar Zalathar deleted the ra-false-positive branch May 31, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants