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

EarlyBinder prevent misuse #101901

Merged
merged 3 commits into from
Sep 19, 2022
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 16, 2022

folding a type before substituting is pretty much always wrong and could happen by accident, e.g. see #99798 (comment)

this PR removes the TypeFoldable and TypeVisitable impl from EarlyBinder.

r? types cc @jackh726

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 16, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2022
Comment on lines 2771 to 2774
ty::FnDef(def_id, substs) => tcx.normalize_erasing_regions(
tcx.param_env(def_id),
tcx.bound_fn_sig(def_id).subst(tcx, substs),
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g. this was wrong in that it could result in unnormalized types, we should be able to write a testcase for this 🤷 but that seems like effort, so i am not going to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and woops, wrong param env xd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it feels wrong but polymorphization breaks with this change, ah whatever.

Copy link
Member

Choose a reason for hiding this comment

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

Any idea of a "proper" solution for polymorphism?

Copy link
Contributor Author

@lcnr lcnr Sep 19, 2022

Choose a reason for hiding this comment

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

well, two ideas:

  1. don't normalize while computing whether we may polymorphize
  2. only use the normalized types after polymorphization

thinking about this, it would be correct to normalize twice here: once to get to the state assumed by polymorphization, and then once after the subst call to actually have a fully normalized type.

Actually, the result of fn_sig_for_fn_abi is only used by fn_abi_new_uncached which does the following 😁

        let sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, sig);

so we don't have to normalize here at all, except to deal with 2. added a comment mentioning this.

@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the early-binder-type-foldable branch from b2cf8e7 to 872aa52 Compare September 16, 2022 14:42
@bors
Copy link
Contributor

bors commented Sep 17, 2022

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

@compiler-errors
Copy link
Member

I think this is fine to land now, since the polymorphism thing is already labeled with a // HACK comment.

r=me once rebased

@jackh726
Copy link
Member

I would like at least some idea of what the plan to fix polymorphism might look like though.

(Though, I really do like the goal of this PR)

@lcnr lcnr force-pushed the early-binder-type-foldable branch from 872aa52 to 647052f Compare September 19, 2022 09:37
@lcnr
Copy link
Contributor Author

lcnr commented Sep 19, 2022

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Sep 19, 2022

📌 Commit 0c3e01d has been approved by compiler-errors

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 Sep 19, 2022
@bors
Copy link
Contributor

bors commented Sep 19, 2022

⌛ Testing commit 0c3e01d with merge 11bb80a...

@bors
Copy link
Contributor

bors commented Sep 19, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 11bb80a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #101901!

Tested on commit 11bb80a.
Direct link to PR: #101901

💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung).

@bors bors merged commit 11bb80a into rust-lang:master Sep 19, 2022
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 19, 2022
Tested on commit rust-lang/rust@11bb80a.
Direct link to PR: <rust-lang/rust#101901>

💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung).
@rustbot rustbot added this to the 1.66.0 milestone Sep 19, 2022
@lcnr lcnr deleted the early-binder-type-foldable branch September 19, 2022 14:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (11bb80a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.1% [-4.1%, -4.1%] 1
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)
2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) 2.1% [2.1%, 2.1%] 1

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants