-
Notifications
You must be signed in to change notification settings - Fork 13k
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
resolve: Resolve visibilities on fields with non-builtin attributes #67106
Conversation
Beta nominated for this ^. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone wondering what the fix is here: placeholders have inherited visibility for named fields, so the visibility of the actual field isn't being resolved for them, and so no error is reported.
r=me with the comment below addressed
@petrochenkov to be clear, the beta-nomination is solely on commit 1a89f4d4d218398c0c8f99c5dabfdb8f4895fedf, right? |
@pnkfelix |
To avoid potential duplicate diagnostics and separate the error reporting logic
@bors r=matthewjasper The exact commit hashes have changed due to rebase and added tests, but the commit to backport is still the first one. |
📌 Commit 5f6267c has been approved by |
resolve: Resolve visibilities on fields with non-builtin attributes Follow-up to rust-lang#66669. The first commit is primary (and also a backport candidate), the other ones are further cleanups. In this case it's not strictly necessary to avoid reporting errors during speculative resolution because 1) all visibilities are resolved non-speculatively sooner or later and 2) error reporting infrastructure merges identical errors with identical spans anyway. Fixes rust-lang#67006 r? @matthewjasper
Rollup of 11 pull requests Successful merges: - #66892 (Format libcore with rustfmt (including tests and benches)) - #67106 (resolve: Resolve visibilities on fields with non-builtin attributes) - #67113 (Print the visibility in `print_variant`.) - #67115 (Simplify `check_decl_no_pat`.) - #67119 (libstd miri tests: avoid warnings) - #67125 (Added ExactSizeIterator bound to return types) - #67138 (Simplify `Layout::extend_packed`) - #67145 (fix miri step debug printing) - #67149 (Do not ICE #67123) - #67155 (Move `Layout`s instead of binding by reference) - #67169 (inline some common methods on OsStr) Failed merges: r? @ghost
resolve: Always resolve visibilities on impl items Fixes rust-lang#64705. Similarly to rust-lang#67106 this was an issue with visitor discipline. Impl items were visited as a part of visiting `ast::ItemKind::Impl`, but they should be visit-able in isolation from their parents as well, because that's how they are visited when they are expanded from macros. I've checked that all the remaining `resolve_visibility` calls are used correctly. r? @matthewjasper
discussed at T-compiler meeting. accepted for beta backport. |
[beta] Beta backports Backporting the following pull requests: * resolve: Always resolve visibilities on impl items #67236 * resolve: Resolve visibilities on fields with non-builtin attributes #67106 * E0023: handle expected != tuple pattern type #67044 * Fix `unused_parens` triggers on macro by example code #66983 * Fix some issues with attributes on unnamed fields #66669 * Ensure that we get a hard error on generic ZST constants if their bodies #67134 (via #67297) Some of these conflicted on merge, I resolved where possible, sometimes by cherry-picking a commit or two more from the relevant PRs. Since those changes are necessary though for backport to proceed (otherwise not even std/core compile), seems fine -- they're fairly minor cleanups anyway.
Follow-up to #66669.
The first commit is primary (and also a backport candidate), the other ones are further cleanups.
In this case it's not strictly necessary to avoid reporting errors during speculative resolution because 1) all visibilities are resolved non-speculatively sooner or later and 2) error reporting infrastructure merges identical errors with identical spans anyway.
Fixes #67006
r? @matthewjasper