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

Add internal lint against Ty == Ty #107393

Conversation

Noratrieb
Copy link
Member

== is often the first and most obvious operation people use to check whether types are equal, but it's usually not what they want, as it doesn't consider inference variables, bound vars and normalization.

We still need Ty: Eq for various things, for example for the query system.
There are still places where using it is "good enough" if the types are known to not have the caveats above (it should NOT be used in diagnostics though as the caveats will lead to worse diagnostics).

The lint points people to a yet-to-be-written section of the rustc dev guide about how to compare types properly.

I have not yet activated it by default because all the cfg_attr(bootstrap, allow) would be really annoying, so we should only activate and fix it after the bootstrap compiler is bumped to contain the lint.

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

r? @eholk

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

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 27, 2023
@Noratrieb Noratrieb 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 27, 2023
@jyn514
Copy link
Member

jyn514 commented Jan 27, 2023

I have not yet activated it by default because all the cfg_attr(bootstrap, allow) would be really annoying, so we should only activate and fix it after the bootstrap compiler is bumped to contain the lint.

Can you open an issue to enable it by default in 6 weeks?

@JakobDegen
Copy link
Contributor

I have not yet activated it by default because all the cfg_attr(bootstrap, allow) would be really annoying

How many times does this fire on the repo? How many of those are things that should be fixed?

@rust-log-analyzer

This comment has been minimized.

@Noratrieb Noratrieb force-pushed the Are the types equalॽ Who even knows at this point branch 3 times, most recently from fc1687d to 0edbc4b Compare January 28, 2023 16:51
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member Author

I've done a little more thinking about this and looked at some code and I think there are now a few things to do:

  • First, land this PR with the lint being disabled so I don't have to think about cfg(bootstrap)
  • Then after the bootstrap bump, turn it on
  • With the lint enabled, make rustc warning-free again. This is done through three ways.
    • In some crates like rustc_codegen_llvm, #![allow] it in the whole crate
    • For comparisons that are fine, add a new method intern_eq (bikeshed name later) that serves as "I know what I'm doing and want this"
    • For cases that seems sketchy, investigate them and fix them. This may take some time and can be nicely split up. Just #[allow] theses cases at first and add FIXMEs, these can be checked and then either replaced with intern_eq or some better comparison later

@rust-cloud-vms rust-cloud-vms bot force-pushed the Are the types equalॽ Who even knows at this point branch 2 times, most recently from 3af7802 to db25c54 Compare February 22, 2023 19:26
@Noratrieb Noratrieb marked this pull request as ready for review February 22, 2023 19:26
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, one nit

compiler/rustc_lint/src/internal.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 11, 2023

☔ The latest upstream changes (presumably #109001) made this pull request unmergeable. Please resolve the merge conflicts.

@anden3
Copy link
Contributor

anden3 commented Apr 13, 2023

Hello @Nilstrieb! I noticed that this PR has some merge conflicts, what's the status of it?

@Noratrieb
Copy link
Member Author

basically the steps described above. I need to fix the (absolutely trivial, thanks git) conflicts and then merge it and go through the plan later. I'll try doing that later this week if I don't forget about it.

@Noratrieb Noratrieb force-pushed the Are the types equalॽ Who even knows at this point branch from db25c54 to ce705db Compare April 22, 2023 21:27
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the Are the types equalॽ Who even knows at this point branch from 768a7ca to bff2feb Compare June 24, 2023 20:11
@Noratrieb
Copy link
Member Author

great question, i had to figure that out myself
I rebased and updated it to properly implement the plan described in #107393 (comment)
@rustbot ready (i think?)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 24, 2023
@Noratrieb
Copy link
Member Author

r? compiler-errors you've already reviewed it and we were talking about it

@rustbot rustbot assigned compiler-errors and unassigned eholk Jun 24, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the Are the types equalॽ Who even knows at this point branch from bff2feb to 71a030e Compare June 24, 2023 20:25
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the Are the types equalॽ Who even knows at this point branch from 71a030e to bd04c1e Compare June 24, 2023 21:04
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me if ci is happy

@rust-log-analyzer

This comment has been minimized.

@Noratrieb Noratrieb force-pushed the Are the types equalॽ Who even knows at this point branch from bd04c1e to 0fd08a6 Compare June 27, 2023 15:08
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 Jun 29, 2023
@bors
Copy link
Contributor

bors commented Aug 5, 2023

☔ The latest upstream changes (presumably #113734) made this pull request unmergeable. Please resolve the merge conflicts.

@Noratrieb Noratrieb force-pushed the Are the types equalॽ Who even knows at this point branch from 0fd08a6 to 307610c Compare October 15, 2023 14:38
@rust-log-analyzer

This comment has been minimized.

This operation does not do what people usually want. Also check against
other comparison operators since they are just as sketchy.

Allow the `ty_eq_operator` lint for now until the next bootstrap bump
@Noratrieb Noratrieb force-pushed the Are the types equalॽ Who even knows at this point branch from 307610c to 7ca7722 Compare October 15, 2023 15:03
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_335d9cec-98a8-43ea-83a4-e218837fd9d6
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=Are the types equalॽ Who even knows at this point
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_335d9cec-98a8-43ea-83a4-e218837fd9d6
GITHUB_REF=refs/pull/107393/merge
GITHUB_REF_NAME=107393/merge
GITHUB_REF_PROTECTED=false
---
   Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
error: using the a comparison operator on `Ty`
    --> src/librustdoc/clean/mod.rs:1358:32
     |
1358 |                 if self_arg_ty == self_ty {
     |
     |
     = note: this does probably not what you want as it does not handle inference variables and more
     = note: it's also not recommended to use it for diagnostics
     = help: for more information, see https://rustc-dev-guide.rust-lang.org/ty.html#comparing-types
     = note: `-D rustc::ty-compare-operator` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(rustc::ty_compare_operator)]`
error: using the a comparison operator on `Ty`
    --> src/librustdoc/clean/mod.rs:1361:27
     |
1361 |                     if ty == self_ty {
1361 |                     if ty == self_ty {
     |                           ^^
     |
     = note: this does probably not what you want as it does not handle inference variables and more
     = note: it's also not recommended to use it for diagnostics
     = help: for more information, see https://rustc-dev-guide.rust-lang.org/ty.html#comparing-types
error: using the a comparison operator on `Ty`
   --> src/librustdoc/clean/simplify.rs:123:45
    |
    |
123 |                 if pred.trait_ref.self_ty() == self_ty { Some(pred.def_id()) } else { None }
    |
    |
    = note: this does probably not what you want as it does not handle inference variables and more
    = note: it's also not recommended to use it for diagnostics
    = help: for more information, see https://rustc-dev-guide.rust-lang.org/ty.html#comparing-types
error: using the a comparison operator on `Ty`
   --> src/librustdoc/passes/collect_intra_doc_links.rs:802:38
    |
    |
802 |             let saw_impl = impl_type == ty
    |
    |
    = note: this does probably not what you want as it does not handle inference variables and more
    = note: it's also not recommended to use it for diagnostics
    = help: for more information, see https://rustc-dev-guide.rust-lang.org/ty.html#comparing-types
error: could not compile `rustdoc` (lib) due to 4 previous errors
Build completed unsuccessfully in 0:10:09
cat: /tmp/toolstate/toolstates.json: No such file or directory
  local time: Sun Oct 15 15:17:30 UTC 2023

@bors
Copy link
Contributor

bors commented Oct 17, 2023

☔ The latest upstream changes (presumably #116196) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Ping from triage
@Nilstrieb This PR is listed as approved but has a broken build.

@Noratrieb
Copy link
Member Author

yes, i need to get back to this, there are some weird interactions with rustdoc that i haven't looked at

@JohnCSimon
Copy link
Member

@Nilstrieb

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this May 26, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

10 participants