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

Remove TrustedLen requirement from BuilderMethods::switch #77161

Merged
merged 1 commit into from
Sep 26, 2020

Conversation

est31
Copy link
Member

@est31 est31 commented Sep 24, 2020

The main use case of TrustedLen is allowing APIs to specialize on it,
but no use of it uses that specialization. Instead, only the .len()
function provided by ExactSizeIterator is used, which is already
required to be accurate.

Thus, the TrustedLen requirement on BuilderMethods::switch is redundant.

The main use case of TrustedLen is allowing APIs to specialize on it,
but no use of it uses that specialization. Instead, only the .len()
function provided by ExactSizeIterator is used, which is already
required to be accurate.

Thus, the TrustedLen requirement on BuilderMethods::switch is redundant.
@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2020
@ecstatic-morse
Copy link
Contributor

The other use of TrustedLen is when returning an incorrect value from ExactSizeIterator would cause UB from safe code. That seems to be the case here.

@est31
Copy link
Member Author

est31 commented Sep 24, 2020

It's not UB if the number is wrong. According to docs, it's only a hint for allocation:

Create a switch instruction with the specified value, default dest, and with a hint for the number of cases that will be added (for efficient allocation).

@ecstatic-morse
Copy link
Contributor

Good catch! However, this is part of a trait signature (though it seems that LLVM is the only backend that implements it), so I think we should maintain the status quo here. Is there a reason you want this besides cleanup?

@est31
Copy link
Member Author

est31 commented Sep 24, 2020

Admittedly I wasn't aware of this guarantee that the trait gave, but I was lucky that it's not applicable :).

Yeah, neither cranelift codegen nor gcc-rust implement the trait, only the in-tree llvm codegen. Even if they used it, the API provided is unstable anyways, and it's not really hard to update. I had to spend a lot of time reading github discussions on what TrustedLen means and I think the new API is much more easy to handle because implementors don't have this mental burden of having to understand a poorly documented and niche feature of the language (which also has still unresolved questions in its tracking issue).

Btw the commit that introduced the TrustedLen was a similar cleanup commit: 35705de

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 24, 2020

I'll defer to the original author then, although my preference is to close this. I don't think we should force implementers to handle cases where ExactSizeIterator lies. This signature allows them to assume len is accurate in unsafe code.

If that aspect of TrustedLen is unclear, perhaps the documentation could be improved?

@ecstatic-morse
Copy link
Contributor

r? @bjorn3

@est31
Copy link
Member Author

est31 commented Sep 24, 2020

This signature allows them to assume len is accurate in unsafe code.

I wouldn't agree with that. #37572 lists the question what TrustedLen means for ExactSizeIterator as open question. The only thing that implementors can assume is that size_hint is accurate. The default ExactSizeIterator impl bases on that but there is no guarantee that the implementors don't override the default :).

@bjorn3
Copy link
Member

bjorn3 commented Sep 24, 2020

It's not UB if the number is wrong. According to docs, it's only a hint for allocation:

Create a switch instruction with the specified value, default dest, and with a hint for the number of cases that will be added (for efficient allocation).

When I introduced the bound I assumed that it would be UB for the length to be wrong. Once cg_clif uses BuilderMethods it will not require TrustedLen either as cranelift_codegen::Switch doesn't need to know the size in advance. Because of this I don't have any objections to this change, but I will leave it up to someone else to decide if this should be merged.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 24, 2020

I don't think that the fact that TrustedLen is underspecified means we should start removing it from internal APIs. If you find someone who disagrees (perhaps someone who's more familiar with TrustedLen than I am?), you're free to ask for a different reviewer. I won't approve this without a better rationale.

@est31
Copy link
Member Author

est31 commented Sep 24, 2020

I don't think that the fact that TrustedLen is underspecified means we should start removing it from internal APIs.

Thankfully this PR has more justification than only that, in that TrustedLen is not required. What would you say if instead of an unused TrustedLen bound, there were an unused Clone bound?

Also note that this PR doesn't propose removal of all uses of TrustedLen in the compiler. In the other place where it's used, one I can't replace it this easily with ExactSizeIterator because latter doesn't support chaining. In this instance though, it's dead weight. Nobody needs the bound and it only confuses readers. The fact that the original reviewer would prefer review by a TrustedLen expert further proves my point :).

I'll r? @petrochenkov , maybe they are more amenable to simplification PRs :).

@petrochenkov
Copy link
Contributor

Simpler is better.
@bors r+ rollup

(Also, nobody is going to return unreasonable numbers from ExactSizeIterator::len in practice, so it can be used as a hint pretty reliably.)

@bors
Copy link
Contributor

bors commented Sep 25, 2020

📌 Commit 12ada5c has been approved by petrochenkov

@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 25, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 26, 2020
…petrochenkov

Remove TrustedLen requirement from BuilderMethods::switch

The main use case of TrustedLen is allowing APIs to specialize on it,
but no use of it uses that specialization. Instead, only the .len()
function provided by ExactSizeIterator is used, which is already
required to be accurate.

Thus, the TrustedLen requirement on BuilderMethods::switch is redundant.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75454 (Explicitly document the size guarantees that Option makes.)
 - rust-lang#76631 (Add `x.py setup`)
 - rust-lang#77076 (Add missing code examples on slice iter types)
 - rust-lang#77093 (merge `need_type_info_err(_const)`)
 - rust-lang#77122 (Add `#![feature(const_fn_floating_point_arithmetic)]`)
 - rust-lang#77127 (Update mdBook)
 - rust-lang#77161 (Remove TrustedLen requirement from BuilderMethods::switch)
 - rust-lang#77166 (update Miri)
 - rust-lang#77181 (Add doc alias for pointer primitive)
 - rust-lang#77204 (Remove stray word from `ClosureKind::extends` docs)
 - rust-lang#77207 (Rename `whence` to `span`)
 - rust-lang#77211 (Remove unused #[allow(...)] statements from compiler/)

Failed merges:

 - rust-lang#77170 (Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`)

r? `@ghost`
@bors bors merged commit 7c47b1e into rust-lang:master Sep 26, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants