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

Yeet unnecessary param envs #119096

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Yeet unnecessary param envs #119096

merged 3 commits into from
Dec 20, 2023

Conversation

compiler-errors
Copy link
Member

We don't need to pass in param-envs around in the lexical region resolution code (or in MatchAgainstFreshVars in the solver), since it is only used to eval some consts in structurally_relate_tys which I removed.

This is in preparation for normalizing the outlives clauses in ParamEnv for the new trait solver.

r? lcnr

@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 Dec 18, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2023

Type relation code was changed

cc @compiler-errors, @lcnr

lcnr
lcnr reviewed Dec 19, 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.

do we want to move fn param_env into ObligationEmittingRelation to avoid needing these bug! impls?

afaict that's the only way param_env is used anymore. tbh I would maybe even rename TypeRelation to StructuralTypeRelation and ObligationEmittingRelation to simply be TypeRelation 🤔

@lcnr

This comment was marked as duplicate.

Sign in to view
@compiler-errors
Copy link
Member Author

compiler-errors commented Dec 19, 2023 •
edited
Loading

afaict that's the only way param_env is used anymore. tbh I would maybe even rename TypeRelation to StructuralTypeRelation and ObligationEmittingRelation to simply be TypeRelation 🤔

param_env is still used in structurally_relate_consts

edit: nvm

@compiler-errors
Copy link
Member Author

I would maybe even rename TypeRelation to StructuralTypeRelation and ObligationEmittingRelation to simply be TypeRelation

I don't really like the names because StructuralTypeRelation is not really the accurate name when e.g. the fn tys just delegates to super_combine_tys. I think the naming right now is fine, but I will remove the param-env fns.

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2023

Type relation code was changed

cc @compiler-errors, @lcnr

aliemjay
aliemjay approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

Nice!

@lcnr
Copy link
Contributor

lcnr commented Dec 19, 2023

I would maybe even rename TypeRelation to StructuralTypeRelation and ObligationEmittingRelation to simply be TypeRelation

I don't really like the names because StructuralTypeRelation is not really the accurate name when e.g. the fn tys just delegates to super_combine_tys. I think the naming right now is fine, but I will remove the param-env fns.

I proposed to rename TypeRelation to StructuralTypeRelation. Anything that uses super_combine_tys has to impl ObligationEmittingRelation anyways, so it is not merely implementing StructuralTypeRelation

@compiler-errors
Copy link
Member Author

compiler-errors commented Dec 19, 2023 •
edited
Loading

Yes, I just think it's weird for a not-purely-structural-impl to implement a trait called "StructuralTypeRelation"

@compiler-errors
Copy link
Member Author

compiler-errors commented Dec 19, 2023 •
edited
Loading

I think the base trait in a lattice of capabilities (e.g. Iterator) should have the shortest name because it's not promising any capabilities. Additional traits qualify that (FusedIterator, DoubleEndedIterator) with additional capabilities in a way that's additive.

In this case Structural is not additive, but claiming something about types that only implement that trait.

@lcnr
Copy link
Contributor

lcnr commented Dec 19, 2023

hmm, I guess for now lets just merge this. Will open a PR myself if I end up having a strong opinion here

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2023

📌 Commit a75d002 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 19, 2023
@compiler-errors
Copy link
Member Author

like impl StructuralTypeRelation<'tcx> for Equate<'tcx> is misleading imo, because equate does not implement a structural type relation -- it implements a type relation, but that relation isn't structural 😅

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 19, 2023
@compiler-errors
Rollup merge of rust-lang#119096 - compiler-errors:yeet-unnecessary-p…
53880ef 
…aram-envs, r=lcnr

Yeet unnecessary param envs

We don't need to pass in param-envs around in the lexical region resolution code (or in `MatchAgainstFreshVars` in the solver), since it is only used to eval some consts in `structurally_relate_tys` which I removed.

This is in preparation for normalizing the outlives clauses in `ParamEnv` for the new trait solver.

r? lcnr
@compiler-errors compiler-errors mentioned this pull request Dec 19, 2023
Closed
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 19, 2023
@compiler-errors
Rollup merge of rust-lang#119096 - compiler-errors:yeet-unnecessary-p…
f5a1261 
…aram-envs, r=lcnr

Yeet unnecessary param envs

We don't need to pass in param-envs around in the lexical region resolution code (or in `MatchAgainstFreshVars` in the solver), since it is only used to eval some consts in `structurally_relate_tys` which I removed.

This is in preparation for normalizing the outlives clauses in `ParamEnv` for the new trait solver.

r? lcnr
@compiler-errors compiler-errors mentioned this pull request Dec 19, 2023
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2023
@bors
Auto merge of rust-lang#119143 - compiler-errors:rollup-vps74yb, r=co…
29fdb02 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118691 (Add check for possible CStr literals in pre-2021)
 - rust-lang#118973 (rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work)
 - rust-lang#119071 (-Znext-solver: adapt overflow rules to avoid breakage)
 - rust-lang#119089 (effects: fix a comment)
 - rust-lang#119096 (Yeet unnecessary param envs)
 - rust-lang#119118 (Fix arm64e-apple-ios target)
 - rust-lang#119134 (resolve: Feed visibilities for unresolved trait impl items)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2023
@bors
Auto merge of rust-lang#119143 - compiler-errors:rollup-vps74yb, r=co…
9d12984 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118691 (Add check for possible CStr literals in pre-2021)
 - rust-lang#118973 (rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work)
 - rust-lang#119071 (-Znext-solver: adapt overflow rules to avoid breakage)
 - rust-lang#119089 (effects: fix a comment)
 - rust-lang#119096 (Yeet unnecessary param envs)
 - rust-lang#119118 (Fix arm64e-apple-ios target)
 - rust-lang#119134 (resolve: Feed visibilities for unresolved trait impl items)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Dec 20, 2023

⌛ Testing commit a75d002 with merge 5810dee...

@bors
Copy link
Contributor

bors commented Dec 20, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 5810dee to master...

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

Finished benchmarking commit (5810dee): comparison URL.

Overall result: ✅ improvements - 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
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.8% [-1.0%, -0.7%] 6
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.9% [0.5%, 1.5%] 3
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.1% [-1.1%, -1.1%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.4% [-1.1%, 1.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
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)		 -2.5% [-3.0%, -1.9%] 6
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

Binary size

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

Bootstrap: 675.418s -> 670.994s (-0.66%)
Artifact size: 312.80 MiB -> 312.83 MiB (0.01%)

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

@aliemjay aliemjay aliemjay approved these changes

Assignees

@lcnr lcnr

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@compiler-errors @rustbot @lcnr @bors @rust-timer @aliemjay