Make privacy visitor use types more (instead of HIR) by oli-obk · Pull Request #113671 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation68 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

r? @petrochenkov

This is a prerequisite to normalizing projections, as otherwise we have too many invalid bound vars (hir_ty_to_ty is creating types that have bound vars, but no binder).

The commits are still chaotic, I'm gonna clean them up, but I just wanted to let you know about the general direction and wondering if we could land this before adding normalization, as normalization is where behavioral changes happen, and I'd like to keep that part as minimal as possible.

context can be found on zulip

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

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

Jul 13, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

(I also don't understand all the new binder play here, but I assume it's not something complex, and you know what you are doing.)

wondering if we could land this before adding normalization, as normalization is where behavioral changes happen, and I'd like to keep that part as minimal as possible.

Yes, that's quite reasonable.

(I also don't understand all the new binder play here, but I assume it's not something complex, and you know what you are doing.)

I'll pull it into a separate commit. It's not necessary until we do normalization, and it's indeed just a mechanical refactoring to not lose the binders

This comment was marked as resolved.

oli-obk 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

Jul 18, 2023

rustbot added S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jul 18, 2023

This comment was marked as resolved.

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-blocked

Status: Blocked on something else such as an RFC or other implementation work.

labels

Sep 1, 2023

@rustbot ready

not sure about the last commit, but I added it to show why I'm special casing projections

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

Sep 6, 2023

📌 Commit d80d7ea has been approved by petrochenkov

It is now in the queue for this repository.

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

Feb 8, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 9, 2024

…ochenkov

Make privacy visitor use types more (instead of HIR)

r? @petrochenkov

This is a prerequisite to normalizing projections, as otherwise we have too many invalid bound vars (hir_ty_to_ty is creating types that have bound vars, but no binder).

The commits are still chaotic, I'm gonna clean them up, but I just wanted to let you know about the general direction and wondering if we could land this before adding normalization, as normalization is where behavioral changes happen, and I'd like to keep that part as minimal as possible.

context can be found on zulip

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

…iaskrgr

Rollup of 9 pull requests

Successful merges:

• rust-lang#113026 (Introduce run-make V2 infrastructure, a run_make_support library and port over 2 tests as example)
• rust-lang#113671 (Make privacy visitor use types more (instead of HIR))
• rust-lang#120308 (core/time: avoid divisions in Duration::new)
• rust-lang#120693 (Invert diagnostic lints.)
• rust-lang#120704 (A drive-by rewrite of give_region_a_name())
• rust-lang#120809 (Use transmute_unchecked in NonZero::new.)
• rust-lang#120817 (Fix more ty::Error ICEs in MIR passes)
• rust-lang#120828 (Fix ErrorGuaranteed unsoundness with stash/steal.)
• rust-lang#120831 (Startup objects disappearing from sysroot)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

Rollup merge of rust-lang#113671 - oli-obk:normalize_weak_tys, r=petrochenkov

Make privacy visitor use types more (instead of HIR)

r? @petrochenkov

This is a prerequisite to normalizing projections, as otherwise we have too many invalid bound vars (hir_ty_to_ty is creating types that have bound vars, but no binder).

The commits are still chaotic, I'm gonna clean them up, but I just wanted to let you know about the general direction and wondering if we could land this before adding normalization, as normalization is where behavioral changes happen, and I'd like to keep that part as minimal as possible.

context can be found on zulip

flip1995 pushed a commit to flip1995/rust that referenced this pull request

Feb 26, 2024

This was referenced

Feb 27, 2024

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request

Mar 8, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 15, 2024

…=compiler-errors

Clean up AstConv

Split off from rust-lang#120926 to make it only contain the renaming & (doc) comment updates. Any changes other than that which have accumulated over time are now part of this PR. Let's be disciplined ;) Inspired by rust-lang#120926 (comment).

• Remove hir_trait_to_predicates
  • Unused since rust-lang#113671
• Inline create_args_for_ast_trait_ref
  • Only had a single call site
  • Having it as a separate method didn't gain us anything
• Use an if-let guard somewhere to avoid unwrapping
• Avoid explicit trait object lifetimes
  • More legible, stylistic-only (the updated code is 100% semantically identical)
  • Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
  • Make use of object lifetime defaulting for a trait object type inside of a reference type somewhere
• Use preexisting dedicated method ItemCtxt::to_ty over <dyn AstConv<'_>>::ast_ty_to_ty
• Use preexisting dedicated method AstConv::astconv over explicit coercions
• Simplify the function signature of create_args_for_ast_path and of check_generic_arg_count
  • In both cases redundant information was passed rendering the call sites verbose and confusing
  • No perf impact (tested in rust-lang#120926)
• Move diagnostic method report_ambiguous_associated_type from astconv to astconv::errors
  • The submodule errors exists specifically for that purpose
  • Use it to keep the main module clean & short

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 15, 2024

…=compiler-errors

Clean up AstConv

Split off from rust-lang#120926 to make it only contain the renaming & (doc) comment updates. Any changes other than that which have accumulated over time are now part of this PR. Let's be disciplined ;) Inspired by rust-lang#120926 (comment).

• Remove hir_trait_to_predicates
  • Unused since rust-lang#113671
• Inline create_args_for_ast_trait_ref
  • Only had a single call site
  • Having it as a separate method didn't gain us anything
• Use an if-let guard somewhere to avoid unwrapping
• Avoid explicit trait object lifetimes
  • More legible, stylistic-only (the updated code is 100% semantically identical)
  • Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
  • Make use of object lifetime defaulting for a trait object type inside of a reference type somewhere
• Use preexisting dedicated method ItemCtxt::to_ty over <dyn AstConv<'_>>::ast_ty_to_ty
• Use preexisting dedicated method AstConv::astconv over explicit coercions
• Simplify the function signature of create_args_for_ast_path and of check_generic_arg_count
  • In both cases redundant information was passed rendering the call sites verbose and confusing
  • No perf impact (tested in rust-lang#120926)
• Move diagnostic method report_ambiguous_associated_type from astconv to astconv::errors
  • The submodule errors exists specifically for that purpose
  • Use it to keep the main module clean & short

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 15, 2024

Rollup merge of rust-lang#122527 - fmease:clean-up-hir-ty-lowering, r=compiler-errors

Clean up AstConv

Split off from rust-lang#120926 to make it only contain the renaming & (doc) comment updates. Any changes other than that which have accumulated over time are now part of this PR. Let's be disciplined ;) Inspired by rust-lang#120926 (comment).

• Remove hir_trait_to_predicates
  • Unused since rust-lang#113671
• Inline create_args_for_ast_trait_ref
  • Only had a single call site
  • Having it as a separate method didn't gain us anything
• Use an if-let guard somewhere to avoid unwrapping
• Avoid explicit trait object lifetimes
  • More legible, stylistic-only (the updated code is 100% semantically identical)
  • Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
  • Make use of object lifetime defaulting for a trait object type inside of a reference type somewhere
• Use preexisting dedicated method ItemCtxt::to_ty over <dyn AstConv<'_>>::ast_ty_to_ty
• Use preexisting dedicated method AstConv::astconv over explicit coercions
• Simplify the function signature of create_args_for_ast_path and of check_generic_arg_count
  • In both cases redundant information was passed rendering the call sites verbose and confusing
  • No perf impact (tested in rust-lang#120926)
• Move diagnostic method report_ambiguous_associated_type from astconv to astconv::errors
  • The submodule errors exists specifically for that purpose
  • Use it to keep the main module clean & short

fmease added a commit to fmease/rust that referenced this pull request

Mar 16, 2024

…piler-errors

Remove obsolete parameter speculative from instantiate_poly_trait_ref

In rust-lang#122527 I totally missed that speculative has become obsolete with the removal of hir_trait_to_predicates / due to rust-lang#113671.

Fixes rust-lang#114635.

r? @compiler-errors

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 17, 2024

Rollup merge of rust-lang#122577 - fmease:speculative-say-what, r=compiler-errors

Remove obsolete parameter speculative from instantiate_poly_trait_ref

In rust-lang#122527 I totally missed that speculative has become obsolete with the removal of hir_trait_to_predicates / due to rust-lang#113671.

Fixes rust-lang#114635.

r? @compiler-errors

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request

Apr 7, 2024

Refactor type visitor walking

r? @petrochenkov

pulling out the uncontroversial parts of rust-lang/rust#113671

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request

Apr 27, 2024

Refactor type visitor walking

r? @petrochenkov

pulling out the uncontroversial parts of rust-lang/rust#113671

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 31, 2024

Fix validation when lowering ? trait bounds

Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.

This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in rust-lang#113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Nov 1, 2024

Fix validation when lowering ? trait bounds

Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.

This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in rust-lang#113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Nov 1, 2024

Rollup merge of rust-lang#132209 - compiler-errors:modifiers, r=fmease

Fix validation when lowering ? trait bounds

Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.

This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in rust-lang#113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.

oli-obk deleted the normalize_weak_tys branch

January 14, 2025 10:32

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.