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

allow eq constraints on associated constants #87648

Merged
merged 7 commits into from
Jan 18, 2022

Conversation

JulianKnodt
Copy link
Contributor

Updates #70256

(cc @varkor, @Centril)

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Jul 30, 2021
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2021

I have been musing about "just" adding a TyKind::Const and removing all the duplication we have around const generics vs type generics. Since this would also simplify our representation of associated const vs associated type as visible in this PR, maybe that's something we should pursue?

I'll give this PR a closer review on monday, but my fingers are still itching for TyKind::Const

@JulianKnodt
Copy link
Contributor Author

Hmmmm, I think if it's a useful tool to simplify implementations, then it's probably useful, but at the same time I can't really picture what a TyKind::Const means. Is it an instance of a type which is const? Or does it represent an abstract const instantiation of a type?

@rust-log-analyzer

This comment has been minimized.

@JulianKnodt JulianKnodt force-pushed the const_eq_constrain branch 2 times, most recently from 33ffbf3 to 58c97a9 Compare July 31, 2021 05:31
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 2, 2021

Hmmmm, I think if it's a useful tool to simplify implementations, then it's probably useful, but at the same time I can't really picture what a TyKind::Const means. Is it an instance of a type which is const? Or does it represent an abstract const instantiation of a type?

it is essentially the equivalent of a built-in struct Foo<T, const C: T>; without the struct/Adt.

think about it this way: before const generics we had typenum, this is essentially typenum but directly supported by the compiler. typenum encodes constants in the type system, with TyKind::Const we do the same thing. We don't have to support any surface syntax for this, so users can't impl Trait for 42 {}, but the type system would support constants as first class types.

If we want to we could even represent it as such. So not even adding a TyKind::Const, but having Foo be a lang item in libcore that all const generics desugar to, but I fear that is just as much of a problem as making bool an enum bool { true, false } lang item. Nice from a cleanliness perspective, but impractical and a performance problem.

@JulianKnodt
Copy link
Contributor Author

I've never heard of typenum, I've not been working on rustc long enough to have seen it 😂. If you're going to implement it, tag me in the changes and I'll hold off on this change until that's done?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 5, 2021

I'm going to do an MCP, and if it is declined, we go with your PR, if not, I'll implement TyKind::Const

Note: typenum is just a regular crate on crates.io, nothing rustc specific. It was the go-to workaround for lack of const generics and still is the go-to workaround for lack of const_evaluatable_checked.

@JulianKnodt
Copy link
Contributor Author

Sounds good, please cc in the MCP as well, I'm interested in those changes as well!

@bors
Copy link
Contributor

bors commented Aug 18, 2021

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

@inquisitivecrystal inquisitivecrystal added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 24, 2021
@jackh726
Copy link
Member

r? @oli-obk

@klensy
Copy link
Contributor

klensy commented Jan 18, 2022

Changes to llvm and cargo looks like not intended, bad rebase?

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7bc7be8): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.2% on incr-unchanged builds of deeply-nested-async check)
  • Small regression in instruction counts (up to 0.3% on full builds of diesel check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@klensy
Copy link
Contributor

klensy commented Jan 18, 2022

Changes to llvm and cargo looks like not intended, bad rebase?

@JulianKnodt @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2022

I don't see any submodule changes... is the mobile app hiding them from me?

@JulianKnodt
Copy link
Contributor Author

hmmm well the cargo lock file would fail CI if I didn't update @klensy, so I just put it in, not sure what caused it to change

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2022

Oof yea, the app just bails on me on these links... I guess no more reviews in the app :(

Please revert the submodule changes and the lockfile changes!

@klensy
Copy link
Contributor

klensy commented Jan 18, 2022

@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Jan 18, 2022

Do I have permission to revert the PR? I can push to the branch again but since it's already merged

edit: or what exactly should I do?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 19, 2022

You can revert the PR locally and open a new PR with the revert.

Revert all commits, squash the reverts, and only keep the reverts of the submodules and cargo lock, the functional changes can stay

@nikic nikic mentioned this pull request Jan 19, 2022
@JulianKnodt
Copy link
Contributor Author

@oli-obk I think there was another commit already for updating cargo, and the linked PR should revert the llvm change.
bfacc5c

@Urgau
Copy link
Member

Urgau commented Jan 20, 2022

This PR changed the crate rustdoc-json-types which is a public API (nightly, but still) without increasing the format version and without pinging a rustdoc maintainer. I've send #93132 to fix the format version.

cc @CraftSpider (as rustdoc json maintainer)

@klensy
Copy link
Contributor

klensy commented Jan 20, 2022

@Urgau simply bumping version is wrong, as this pr should be reverted.

@klensy
Copy link
Contributor

klensy commented Jan 20, 2022

rustdoc hitted too, btw.

Or it expected changes? Idk.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2022

The changes are expected from this PR, yes

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…on, r=oli-obk

Increase the format version of rustdoc-json-types

PR rust-lang#87648 changed `rustdoc-json-types` without increasing the format version.
rust-lang@e7529d6#diff-ede26372490522288745c5b3df2b6b2a1cc913dcd09b29af3a49935afe00c7e6

This PR increase the format version by +1 and move the `FORMAT_VERSION` constant to the start of the file to hopefully make it more clear that `rustdoc-json-types` is versioned.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…on, r=oli-obk

Increase the format version of rustdoc-json-types

PR rust-lang#87648 changed `rustdoc-json-types` without increasing the format version.
rust-lang@e7529d6#diff-ede26372490522288745c5b3df2b6b2a1cc913dcd09b29af3a49935afe00c7e6

This PR increase the format version by +1 and move the `FORMAT_VERSION` constant to the start of the file to hopefully make it more clear that `rustdoc-json-types` is versioned.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…on, r=oli-obk

Increase the format version of rustdoc-json-types

PR rust-lang#87648 changed `rustdoc-json-types` without increasing the format version.
rust-lang@e7529d6#diff-ede26372490522288745c5b3df2b6b2a1cc913dcd09b29af3a49935afe00c7e6

This PR increase the format version by +1 and move the `FORMAT_VERSION` constant to the start of the file to hopefully make it more clear that `rustdoc-json-types` is versioned.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
…-obk

allow eq constraints on associated constants

Updates rust-lang#70256

(cc `@varkor,` `@Centril)`
dario23 added a commit to dario23/rust-semverver that referenced this pull request May 4, 2022
dario23 added a commit to dario23/rust-semverver that referenced this pull request May 6, 2022
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. perf-regression Performance regression. 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-lang Relevant to the language 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.