Parse the syntax described in RFC 2632#67820
Merged
bors merged 13 commits intorust-lang:masterfrom Jan 10, 2020
Merged
Conversation
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
changed the title
ecstatic-morse
force-pushed
the
const-trait
branch
from
980703b to
c8f3965
Compare
oli-obk
reviewed
Jan 3, 2020
Contributor
oli-obk
left a comment
oli-obk
left a comment
There was a problem hiding this comment.
Some nits, r=me with them addressed
Contributor
There was a problem hiding this comment.
That's fine for now. It'll immediately go away when the feature gate has some logic, so...
Contributor
Author
There was a problem hiding this comment.
I switched to "not yet implemented" for the non-feature gated errors anyways. It made it easier to track see when I had forgotten a feature gate in the tests.
Contributor
There was a problem hiding this comment.
Not sure if this should ever be accepted. Probably not without an epoch doing other const things. Please make this a hard error for now
Contributor
There was a problem hiding this comment.
But I don't think it should be a hard error under #[cfg(FALSE)]. Make sure to test syntax independently of semantics, and preferably in one file, as a // check-pass test.
Contributor
There was a problem hiding this comment.
I'd also like to see tests that includes impl Trait in various positions (syntactically vs. semantically -- different tests).
Contributor
Author
There was a problem hiding this comment.
?const is now forbidden in impl Trait and trait objects.
Contributor
There was a problem hiding this comment.
I think the tests with and without the feature gate should have the same content so we are testing all the syntax in both situations
Contributor
Author
There was a problem hiding this comment.
I reorganized the tests a bit and am using the revisions trick for now to guarantee this.
Centril
reviewed
Jan 3, 2020
Contributor
There was a problem hiding this comment.
Why is this flag necessary?
Contributor
There was a problem hiding this comment.
Remove this. There's no such directive.
Contributor
There was a problem hiding this comment.
And also this doesn't seem to be true of the test. The errors below are not parser errors.
Contributor
There was a problem hiding this comment.
But I don't think it should be a hard error under #[cfg(FALSE)]. Make sure to test syntax independently of semantics, and preferably in one file, as a // check-pass test.
Contributor
There was a problem hiding this comment.
I'd also like to see tests that includes impl Trait in various positions (syntactically vs. semantically -- different tests).
ecstatic-morse
force-pushed
the
const-trait
branch
from
c8f3965 to
ed8f27f
Compare
Contributor
Author
|
@Centril I added checks for |
ecstatic-morse
force-pushed
the
const-trait
branch
2 times, most recently
from
59a60ed to
e38e10f
Compare
Centril
reviewed
Jan 4, 2020
Contributor
There was a problem hiding this comment.
Seems like we came away with different interpretations of what Oliver said.
I think this would be perfectly well defined and semantically sane (same with dyn ?const Trait).
After some more reflection, I think the semantic property one would want is that ?const is legal if and only if we are in a const-by-default context (const fn) or const Trait for { ... }`). However, if you want to get this landed quicker, I'm fine with this for now (but leave the comment visible).
Contributor
There was a problem hiding this comment.
While these would be fine in the future, having them be a hard error for now and giving them their own feature gates in the future seems the best way forward to me. The RFC specifically talked about a minimal version that does not include impl Trait at all
Contributor
Author
There was a problem hiding this comment.
After porting these checks to AST validation, I also decided to error when they are found on trait bounds (e.g., trait T: ?const Super {}). While this is not explicitly called out as a future extension by the RFC, it will not be useful until const trait definitions have some meaning.
oli-obk
reviewed
Jan 4, 2020
Contributor
There was a problem hiding this comment.
While these would be fine in the future, having them be a hard error for now and giving them their own feature gates in the future seems the best way forward to me. The RFC specifically talked about a minimal version that does not include impl Trait at all
Contributor
|
The job 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 |
ecstatic-morse
force-pushed
the
const-trait
branch
from
f0d4f38 to
14c9ab8
Compare
Contributor
|
The job 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 |
oli-obk
added
F-const_trait_bound_opt_out
F-const_trait_impl
oli-obk
requested changes
Jan 5, 2020
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit 19f74cb has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jan 7, 2020
Parse the syntax described in RFC 2632 This adds support for both `impl const Trait for Ty` and `?const Trait` bound syntax from rust-lang/rfcs#2632 to the parser. For now, both modifiers end up in a newly-added `constness` field on `ast::TraitRef`, although this may change once the implementation is fleshed out. I was planning on using `delay_span_bug` when this syntax is encountered during lowering, but I can't write `should-ice` UI tests. I emit a normal error instead, which causes duplicates when the feature gate is not enabled (see the `.stderr` files for the feature gate tests). Not sure what the desired approach is; Maybe just do nothing when the syntax is encountered with the feature gate is enabled? @oli-obk I went with `const_trait_impl` and `const_trait_bound_opt_out` for the names of these features. Are these to your liking? cc rust-lang#67792 rust-lang#67794 r? @Centril
Collaborator
|
☔ The latest upstream changes (presumably #67770) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
bors
added a commit
that referenced
this pull request
Jan 8, 2020
Rollup of 10 pull requests Successful merges: - #67774 (Try statx for all linux-gnu target.) - #67781 (Move `is_min_const_fn` query to librustc_mir.) - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs) - #67849 (Add a check for swapped words when we can't find an identifier) - #67875 (Distinguish between private items and hidden items in rustdoc) - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`) - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates) - #67977 (Updates for VxWorks) - #67985 (Remove insignificant notes from CStr documentation) - #68003 (ci: fix wrong shared.sh import for publish_toolstate) Failed merges: - #67820 (Parse the syntax described in RFC 2632) - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup) r? @ghost
bors
added a commit
that referenced
this pull request
Jan 8, 2020
Rollup of 10 pull requests Successful merges: - #67774 (Try statx for all linux-gnu target.) - #67781 (Move `is_min_const_fn` query to librustc_mir.) - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs) - #67849 (Add a check for swapped words when we can't find an identifier) - #67875 (Distinguish between private items and hidden items in rustdoc) - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`) - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates) - #67977 (Updates for VxWorks) - #67985 (Remove insignificant notes from CStr documentation) - #68003 (ci: fix wrong shared.sh import for publish_toolstate) Failed merges: - #67820 (Parse the syntax described in RFC 2632) - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup) r? @ghost
This is used for both the `?const` syntax in bounds as well as the `impl const Trait` syntax. I also considered handling these separately by adding a variant of `TraitBoundModifier` and a field to `ItemKind::Impl`, but this approach was less intrusive.
The grammar also handles `?const ?Trait` even though this is semantically redundant.
This means the new syntax will always fail to compile, even when the feature gate is enabled. These checks will be removed in a later PR once the implementation is done.
ecstatic-morse
force-pushed
the
const-trait
branch
from
19f74cb to
fd1c003
Compare
Contributor
Author
|
@bors r=oli-obk |
Collaborator
|
📌 Commit fd1c003 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jan 10, 2020
Parse the syntax described in RFC 2632 This adds support for both `impl const Trait for Ty` and `?const Trait` bound syntax from rust-lang/rfcs#2632 to the parser. For now, both modifiers end up in a newly-added `constness` field on `ast::TraitRef`, although this may change once the implementation is fleshed out. I was planning on using `delay_span_bug` when this syntax is encountered during lowering, but I can't write `should-ice` UI tests. I emit a normal error instead, which causes duplicates when the feature gate is not enabled (see the `.stderr` files for the feature gate tests). Not sure what the desired approach is; Maybe just do nothing when the syntax is encountered with the feature gate is enabled? @oli-obk I went with `const_trait_impl` and `const_trait_bound_opt_out` for the names of these features. Are these to your liking? cc rust-lang#67792 rust-lang#67794 r? @Centril
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jan 10, 2020
Parse the syntax described in RFC 2632 This adds support for both `impl const Trait for Ty` and `?const Trait` bound syntax from rust-lang/rfcs#2632 to the parser. For now, both modifiers end up in a newly-added `constness` field on `ast::TraitRef`, although this may change once the implementation is fleshed out. I was planning on using `delay_span_bug` when this syntax is encountered during lowering, but I can't write `should-ice` UI tests. I emit a normal error instead, which causes duplicates when the feature gate is not enabled (see the `.stderr` files for the feature gate tests). Not sure what the desired approach is; Maybe just do nothing when the syntax is encountered with the feature gate is enabled? @oli-obk I went with `const_trait_impl` and `const_trait_bound_opt_out` for the names of these features. Are these to your liking? cc rust-lang#67792 rust-lang#67794 r? @Centril
bors
added a commit
that referenced
this pull request
Jan 10, 2020
Rollup of 6 pull requests Successful merges: - #66463 (Point at opaque and closure type definitions in type errors) - #67501 (Reduce special treatment for zsts) - #67820 (Parse the syntax described in RFC 2632) - #67922 (rustc_ast_lowering: misc cleanup & rustc dep reductions) - #68071 (Extend support of `_` in type parameters) - #68073 (expect `fn` after `const unsafe` / `const extern`) Failed merges: r? @ghost
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds support for both
impl const Trait for Tyand?const Traitbound syntax from rust-lang/rfcs#2632 to the parser. For now, both modifiers end up in a newly-addedconstnessfield onast::TraitRef, although this may change once the implementation is fleshed out.I was planning on using
delay_span_bugwhen this syntax is encountered during lowering, but I can't writeshould-iceUI tests. I emit a normal error instead, which causes duplicates when the feature gate is not enabled (see the.stderrfiles for the feature gate tests). Not sure what the desired approach is; Maybe just do nothing when the syntax is encountered with the feature gate is enabled?@oli-obk I went with
const_trait_implandconst_trait_bound_opt_outfor the names of these features. Are these to your liking?cc #67792 #67794
r? @Centril