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

Implement `?const` opt-out for trait bounds #68140

Merged
merged 12 commits into from Jan 21, 2020

Conversation

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jan 12, 2020

For now, such bounds are treated exactly the same as unprefixed ones in all contexts. RFC 2632 does not specify whether such bounds are forbidden outside of const contexts, so they are allowed at the moment.

Prior to this PR, the constness of a trait bound/impl was stored in TraitRef. Now, the constness of an impl is stored in ast::ItemKind::Impl and the constness of a bound in ast::TraitBoundModifer. Additionally, constness of trait bounds is now stored in an additional field of ty::Predicate::Trait, and the combination of the constness of the item along with any TraitBoundModifier determines the constness of the bound in accordance with the RFC. Encoding the constness of impls at the ty level is left for a later PR.

After a discussion in #wg-grammar on Discord, it was decided that the grammar should not encode the mutual exclusivity of trait bound modifiers. The grammar for trait bound modifiers remains [?const] [?]. To encode this, I add a dummy variant to ast::TraitBoundModifier that is used when the syntax ?const ? appears. This variant causes an error in AST validation and disappears during HIR lowering.

cc #67794

r? @oli-obk

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Jan 12, 2020

Unfortunately, the straightforward implementation adds a field to ty::TraitRef, which is widely used during trait solving. This is despite the fact that trait solving doesn't need to know about the constness of trait bounds.

I have a few other strategies in mind if the increase in memory usage is too large.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 12, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2020

⌛️ Trying commit c37571d with merge c6996b5...

bors added a commit that referenced this pull request Jan 12, 2020
Support `?const` opt-out for trait bounds

For now, such bounds are treated exactly the same as unprefixed ones in all contexts. [RFC 2632](rust-lang/rfcs#2632) does not specify whether such bounds are forbidden outside of `const` contexts, so they are allowed at the moment.

cc #67794

r? @oli-obk
@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 12, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2020

⌛️ Trying commit c37571d with merge 4c4563c...

bors added a commit that referenced this pull request Jan 12, 2020
Support `?const` opt-out for trait bounds

For now, such bounds are treated exactly the same as unprefixed ones in all contexts. [RFC 2632](rust-lang/rfcs#2632) does not specify whether such bounds are forbidden outside of `const` contexts, so they are allowed at the moment.

cc #67794

r? @oli-obk
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Jan 12, 2020

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ecstatic-morse ecstatic-morse changed the title Support `?const` opt-out for trait bounds Implement `?const` opt-out for trait bounds Jan 12, 2020
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 12, 2020

We need to think about the future checks for ?const. Those won't be implementable on the HIR datastructures since we may be calling generic functions from other crates. All the bounds will need to know about ?const in order for a call to know whether the argument must implement a trait or not. Instead of encoding it in traitref you may be able to encode it as a predicate variant

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2020

☀️ Try build successful - checks-azure
Build commit: 4c4563c (4c4563c022384469eb2c6e32b59ea207b319544f)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 12, 2020

Queued 4c4563c with parent f363745, future comparison URL.

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Jan 12, 2020

Instead of encoding it in traitref you may be able to encode it as a predicate variant.

That's a good idea. I would need to make changes in the trait solver to ensure that Predicate::Trait and Predicate::TraitConst are treated equivalently, but this may be the Right Way™ to implement this.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 12, 2020

Finished benchmarking try commit 4c4563c, comparison URL.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 12, 2020

Maybe it suffices to make it a field of Predicate::Trait? It could fit into the padding after the tag and before the value. Then you don't have to worry about the treating the same

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 12, 2020

Also doesn't really look like a max rss regression, but a perf regression

@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:const-trait-bound-opt-out branch 3 times, most recently from 213ab4d to 3739e6c Jan 14, 2020
@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Jan 14, 2020

After the most recent push, this is closer to the final version, but still not ready for review. The changes to ItemKind::Impl could be split into their own PR. The ConstnessAnd wrapper makes it easier to audit that constness of trait bounds is correctly propagated into ty::Predicate.

@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:const-trait-bound-opt-out branch from 3739e6c to 3fd239f Jan 14, 2020
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 14, 2020

I thunk it would be best if you'd move the first two commits into their own PR as you'll be rebasing all the time otherwise.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2020

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

tmandry added a commit to tmandry/rust that referenced this pull request Jan 18, 2020
…i-obk

Use named fields for `{ast,hir}::ItemKind::Impl`

Currently, the widely used `ItemKind::Impl` variant is a tuple with seven fields. I want to add an eighth in rust-lang#68140, which means I have to update most matches on this variant anyways. Giving a name to each field improves readability and makes future changes of this nature much simpler.

This change will cause several tools to break. I will fix them once this is merged.
@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:const-trait-bound-opt-out branch from 3fd239f to 23ea42c Jan 20, 2020
@ecstatic-morse ecstatic-morse marked this pull request as ready for review Jan 20, 2020
@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Jan 20, 2020

@oli-obk Okay I think this is ready for an initial review. I updated the PR description with a summary of the changes. The diff is pretty noisy, so you might be better off looking at the commits that mention ty::Predicate and ToPredicate separately.

Copy link
Contributor

oli-obk left a comment

r=me with the comment added

I added the "no ?const bounds in non-const scopes"-situation to the tracking issue

@@ -1254,7 +1254,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
| GenericBound::Trait(ref ty, TraitBoundModifier::MaybeConst) => {
Some(this.lower_poly_trait_ref(ty, itctx.reborrow()))
}
GenericBound::Trait(_, TraitBoundModifier::Maybe) => None,
GenericBound::Trait(_, TraitBoundModifier::Maybe)
| GenericBound::Trait(_, TraitBoundModifier::MaybeConstMaybe) => {

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 21, 2020

Contributor

Leave a comment here stating that these are ignored here because they have already errored in ast validation.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 21, 2020

@bors p=1 bitrotty

@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:const-trait-bound-opt-out branch from f1b97af to 6bd69a1 Jan 21, 2020
@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Jan 21, 2020

Comments added.

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

📌 Commit 6bd69a1 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

⌛️ Testing commit 6bd69a1 with merge 4c46ef7...

bors added a commit that referenced this pull request Jan 21, 2020
…i-obk

Implement `?const` opt-out for trait bounds

For now, such bounds are treated exactly the same as unprefixed ones in all contexts. [RFC 2632](rust-lang/rfcs#2632) does not specify whether such bounds are forbidden outside of `const` contexts, so they are allowed at the moment.

Prior to this PR, the constness of a trait bound/impl was stored in `TraitRef`. Now, the constness of an `impl` is stored in `ast::ItemKind::Impl` and the constness of a bound in `ast::TraitBoundModifer`. Additionally, constness of trait bounds is now stored in an additional field of `ty::Predicate::Trait`, and the combination of the constness of the item along with any `TraitBoundModifier` determines the constness of the bound in accordance with the RFC. Encoding the constness of impls at the `ty` level is left for a later PR.

After a discussion in \#wg-grammar on Discord, it was decided that the grammar should not encode the mutual exclusivity of trait bound modifiers. The grammar for trait bound modifiers remains `[?const] [?]`. To encode this, I add a dummy variant to `ast::TraitBoundModifier` that is used when the syntax `?const ?` appears. This variant causes an error in AST validation and disappears during HIR lowering.

cc #67794

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

💥 Test timed out

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 21, 2020

@bors retry

Centril added a commit to Centril/rust that referenced this pull request Jan 21, 2020
…t-out, r=oli-obk

Implement `?const` opt-out for trait bounds

For now, such bounds are treated exactly the same as unprefixed ones in all contexts. [RFC 2632](rust-lang/rfcs#2632) does not specify whether such bounds are forbidden outside of `const` contexts, so they are allowed at the moment.

Prior to this PR, the constness of a trait bound/impl was stored in `TraitRef`. Now, the constness of an `impl` is stored in `ast::ItemKind::Impl` and the constness of a bound in `ast::TraitBoundModifer`. Additionally, constness of trait bounds is now stored in an additional field of `ty::Predicate::Trait`, and the combination of the constness of the item along with any `TraitBoundModifier` determines the constness of the bound in accordance with the RFC. Encoding the constness of impls at the `ty` level is left for a later PR.

After a discussion in \#wg-grammar on Discord, it was decided that the grammar should not encode the mutual exclusivity of trait bound modifiers. The grammar for trait bound modifiers remains `[?const] [?]`. To encode this, I add a dummy variant to `ast::TraitBoundModifier` that is used when the syntax `?const ?` appears. This variant causes an error in AST validation and disappears during HIR lowering.

cc rust-lang#67794

r? @oli-obk
bors added a commit that referenced this pull request Jan 21, 2020
Rollup of 7 pull requests

Successful merges:

 - #67686 (Simplify NodeHeader by avoiding slices in BTreeMaps with shared roots)
 - #68140 (Implement `?const` opt-out for trait bounds)
 - #68313 (Options IP_MULTICAST_TTL and IP_MULTICAST_LOOP are 1 byte on BSD)
 - #68328 (Actually pass target LLVM args to LLVM)
 - #68399 (check_match: misc unifications and ICE fixes)
 - #68415 (tidy: fix most clippy warnings)
 - #68416 (lowering: cleanup some hofs)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

⌛️ Testing commit 6bd69a1 with merge 5e8897b...

@bors bors merged commit 6bd69a1 into rust-lang:master Jan 21, 2020
4 of 5 checks passed
4 of 5 checks passed
homu Testing commit 6bd69a10921785aa8ab68e58d9c7a7ea1ff6ef96 with merge 5e8897b7b51636f157630e6639b711d698e1d101...
Details
pr Build #20200121.16 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
JohnTitor added a commit to JohnTitor/rust-clippy that referenced this pull request Jan 21, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jan 21, 2020
Rustup to rust-lang/rust#68140

changelog: none
ecstatic-morse added a commit to ecstatic-morse/rust-clippy that referenced this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.