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.

Sign up for GitHub

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

Jump to bottom

Make IMPLIED_BOUNDS_ENTAILMENT into a hard error from a lint #117984

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Make IMPLIED_BOUNDS_ENTAILMENT into a hard error from a lint #117984

merged 2 commits into from
Dec 16, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 16, 2023 •
edited by lcnr
Loading

closes #105572

Removes the IMPLIED_BOUNDS_ENTAILMENT and makes the compare_method_predicate_entailment logic just run once.

r? lcnr

@compiler-errors compiler-errors added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Nov 16, 2023
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2023
@lqd
Copy link
Member

lqd commented Nov 16, 2023 •
edited
Loading

I also looked into this and did it the same way you did, but then I also wondered whether we actually wanted to keep the same diagnostics, and emit a "impl method assumes more implied bounds than the corresponding trait method" error instead of a region error (i.e. make a new error code and all that...)?

@Urgau
Copy link
Member

Urgau commented Nov 16, 2023

I actually have no idea what to do with the existing lint. We don't want to remove it, since it'll cause code that has allow(implied_bounds_entailment) to fail, making it annoying to compile between versions or whatever.

In think you can just remove it. unknown_lints is just warn-by-default and lints can be added, renamed and removed without prior notice anyway.

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 16, 2023 •
edited
Loading

@lqd: I have no idea how to make that possible, unless we literally just keep the same "try running compare_method_predicate_entailment twice" strategy, which is what this PR aims to remove. The problem here is that we don't really have a good way of emitting special error messages for region obligations that come from different sources, unlike ObligationCause.

@Urgau: Thanks for the clarification. Makes sense. Totally missed that unknown_lints was just a warning.

@rust-log-analyzer

This comment was marked as outdated.

Sign in to view
@lqd
Copy link
Member

lqd commented Nov 16, 2023

@compiler-errors I came to the exact same conclusion, then closed my editor and deleted my branch 😂

lcnr
lcnr reviewed Nov 17, 2023
View reviewed changes
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
}
}
return Err(infcx
.tainted_by_errors()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't tainted_by_errors always be None, given that we return in case select_all_or_error failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk, astconv might taint the infcx or something. I didn't want to unwrap here, avoids an unnecessary ICE for no cost.

@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 17, 2023
@bors
Copy link
Contributor

bors commented Nov 17, 2023

⌛ Trying commit 16f7de6 with merge aa7c6ba...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2023
@bors
Auto merge of rust-lang#117984 - compiler-errors:implied-bounds-entai…
aa7c6ba 
…lment, r=<try>

Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint

closes rust-lang#105572

Removes the `IMPLIED_BOUNDS_ENTAILMENT` and makes the `compare_method_predicate_entailment` logic just run once.

r? lcnr
@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2023

want to run crater, looking at discussions referencing #105572 there are still some projects depending on rustc-serialize, would start the FCP afterwards 👍

@bors
Copy link
Contributor

bors commented Nov 17, 2023

☀️ Try build successful - checks-actions
Build commit: aa7c6ba (aa7c6ba69acce8b10a275eabfe6157ea72c8b599)

1 similar comment
@bors
Copy link
Contributor

bors commented Nov 17, 2023

☀️ Try build successful - checks-actions
Build commit: aa7c6ba (aa7c6ba69acce8b10a275eabfe6157ea72c8b599)

@rust-timer

This comment has been minimized.

Sign in to view
@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-117984 created and queued.
🤖 Automatically detected try build aa7c6ba
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 17, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (aa7c6ba): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.6% [0.4%, 0.9%] 2
Regressions ❌
(secondary)		 2.1% [1.4%, 2.7%] 2
Improvements ✅
(primary)		 -1.8% [-1.8%, -1.8%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.2% [-1.8%, 0.9%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.6%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.7% [-0.7%, -0.6%] 2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.609s -> 676.495s (0.13%)
Artifact size: 313.59 MiB -> 313.62 MiB (0.01%)

lcnr
lcnr reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

the lint should be registered as a removed lint in

rust/compiler/rustc_lint/src/lib.rs

Line 509 in e24e5af

store.register_removed(

@compiler-errors compiler-errors force-pushed the implied-bounds-entailment branch 2 times, most recently from 890caa1 to 5b088a4 Compare November 21, 2023 20:21
@craterbot
Copy link
Collaborator

🚧 Experiment pr-117984 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors
Copy link
Contributor

bors commented Nov 26, 2023

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

@lcnr
Copy link
Contributor

lcnr commented Dec 15, 2023

@bors r rollup=never

@bors
Copy link
Contributor

bors commented Dec 15, 2023

📌 Commit ea0e059 has been approved by lcnr

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 Dec 15, 2023
@bors
Copy link
Contributor

bors commented Dec 16, 2023

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 16, 2023
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Dec 16, 2023

📌 Commit 32907c7 has been approved by lcnr

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2023
@bors
Copy link
Contributor

bors commented Dec 16, 2023

⌛ Testing commit 32907c7 with merge 5c927ab...

@bors
Copy link
Contributor

bors commented Dec 16, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 5c927ab to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2023
@bors bors merged commit 5c927ab into rust-lang:master Dec 16, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 16, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5c927ab): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.8% [-3.8%, -0.5%] 3
Improvements ✅
(secondary)		 -1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) -1.2% [-3.8%, 0.5%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.5% [0.4%, 0.7%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.5% [0.4%, 0.7%] 2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.592s -> 672.112s (0.08%)
Artifact size: 312.57 MiB -> 312.58 MiB (0.00%)

heinrich5991 added a commit to heinrich5991/libtw2 that referenced this pull request Jan 4, 2024
@heinrich5991
Fix compilation with newest Rust
a445cf2 
rust-lang-deprecated/rustc-serialize#200
rust-lang-deprecated/rustc-serialize@75ce924
rust-lang/rust#117984
heinrich5991 added a commit to heinrich5991/libtw2 that referenced this pull request Jan 4, 2024
@heinrich5991
Fix compilation with Rust 1.76.0
8246fba 
rust-lang-deprecated/rustc-serialize#200
rust-lang-deprecated/rustc-serialize@75ce924
rust-lang/rust#117984
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jan 12, 2024
Closed
This was referenced Jan 12, 2024
Closed
Closed
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 12, 2024
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Feb 18, 2024
@he32
Add a package for rust version 1.76.0.
0e9a2a1 
Pkgsrc changes:
 * Adapt checksums and patches.

Upstream chnages:

Version 1.76.0 (2024-02-08)
==========================

Language
--------
- [Document Rust ABI compatibility between various types]
  (rust-lang/rust#115476)
- [Also: guarantee that char and u32 are ABI-compatible]
  (rust-lang/rust#118032)
- [Warn against ambiguous wide pointer comparisons]
  (rust-lang/rust#117758)

Compiler
--------
- [Lint pinned `#[must_use]` pointers (in particular, `Box<T>`
  where `T` is `#[must_use]`) in `unused_must_use`.]
  (rust-lang/rust#118054)
- [Soundness fix: fix computing the offset of an unsized field in
  a packed struct]
  (rust-lang/rust#118540)
- [Soundness fix: fix dynamic size/align computation logic for
  packed types with dyn Trait tail]
  (rust-lang/rust#118538)
- [Add `$message_type` field to distinguish json diagnostic outputs]
  (rust-lang/rust#115691)
- [Enable Rust to use the EHCont security feature of Windows]
  (rust-lang/rust#118013)
- [Add tier 3 {x86_64,i686}-win7-windows-msvc targets]
  (rust-lang/rust#118150)
- [Add tier 3 aarch64-apple-watchos target]
  (rust-lang/rust#119074)
- [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets]
  (rust-lang/rust#115526)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------
- [Add a column number to `dbg!()`]
  (rust-lang/rust#114962)
- [Add `std::hash::{DefaultHasher, RandomState}` exports]
  (rust-lang/rust#115694)
- [Fix rounding issue with exponents in fmt]
  (rust-lang/rust#116301)
- [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.]
  (rust-lang/rust#117138)
- [Windows: Allow `File::create` to work on hidden files]
  (rust-lang/rust#116438)

Stabilized APIs
---------------
- [`Arc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone)
- [`Rc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone)
- [`Result::inspect`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect)
- [`Result::inspect_err`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err)
- [`Option::inspect`]
  (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect)
- [`type_name_of_val`]
  (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html)
- [`std::hash::{DefaultHasher, RandomState}`]
  (https://doc.rust-lang.org/stable/std/hash/index.html#structs)
  These were previously available only through `std::collections::hash_map`.
- [`ptr::{from_ref, from_mut}`]
  (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html)
- [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html)

Cargo
-----

See [Cargo release notes]
(https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08).

Rustdoc
-------
- [Don't merge cfg and doc(cfg) attributes for re-exports]
  (rust-lang/rust#113091)
- [rustdoc: allow resizing the sidebar / hiding the top bar]
  (rust-lang/rust#115660)
- [rustdoc-search: add support for traits and associated types]
  (rust-lang/rust#116085)
- [rustdoc: Add highlighting for comments in items declaration]
  (rust-lang/rust#117869)

Compatibility Notes
-------------------
- [Add allow-by-default lint for unit bindings]
  (rust-lang/rust#112380)
  This is expected to be upgraded to a warning by default in a future Rust
  release. Some macros emit bindings with type `()` with user-provided spans,
  which means that this lint will warn for user code.
- [Remove x86_64-sun-solaris target.]
  (rust-lang/rust#118091)
- [Remove asmjs-unknown-emscripten target]
  (rust-lang/rust#117338)
- [Report errors in jobserver inherited through environment variables]
  (rust-lang/rust#113730)
  This [may warn](rust-lang/rust#120515)
  on benign problems too.
- [Update the minimum external LLVM to 16.]
  (rust-lang/rust#117947)
- [Improve `print_tts`](rust-lang/rust#114571)
  This change can break some naive manual parsing of token trees
  in proc macro code which expect a particular structure after
  `.to_string()`, rather than just arbitrary Rust code.
- [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint]
  (rust-lang/rust#117984)
- [Vec's allocation behavior was changed when collecting some iterators]
  (rust-lang/rust#110353)
  Allocation behavior is currently not specified, nevertheless
  changes can be surprising.
  See [`impl FromIterator for Vec`]
  (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator-for-Vec)
  for more details.
- [Properly reject `default` on free const items]
  (rust-lang/rust#117818)
@lcnr lcnr mentioned this pull request Feb 29, 2024
Closed
@github-actions github-actions bot mentioned this pull request Apr 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

Tracking issue for IMPLIED_BOUNDS_ENTAILMENT lint