Skip to content

allow eq constraints on associated constants#87648

Merged
bors merged 7 commits into
rust-lang:masterfrom
JulianKnodt:const_eq_constrain
Jan 18, 2022
Merged

allow eq constraints on associated constants#87648
bors merged 7 commits into
rust-lang:masterfrom
JulianKnodt:const_eq_constrain

Conversation

@JulianKnodt

Copy link
Copy Markdown
Contributor

Updates #70256

(cc @varkor, @Centril)

@rust-highfive

Copy link
Copy Markdown
Contributor

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

@rust-highfive

Copy link
Copy Markdown
Contributor

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.

Comment thread compiler/rustc_ast/src/ast.rs Outdated
@oli-obk

oli-obk commented Jul 31, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

oli-obk commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

oli-obk commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

@bors

bors commented Aug 18, 2021

Copy link
Copy Markdown
Collaborator

☔ 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 labels Aug 24, 2021
@jackh726

Copy link
Copy Markdown
Member

r? @oli-obk

@klensy

klensy commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

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

@rust-timer

Copy link
Copy Markdown
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

klensy commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

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

@JulianKnodt @oli-obk

@oli-obk

oli-obk commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

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

@JulianKnodt

Copy link
Copy Markdown
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

oli-obk commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

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

klensy commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

@JulianKnodt

JulianKnodt commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

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

oli-obk commented Jan 19, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Urgau commented Jan 20, 2022

Copy link
Copy Markdown
Member

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

klensy commented Jan 20, 2022

Copy link
Copy Markdown
Contributor

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

@klensy

klensy commented Jan 20, 2022

Copy link
Copy Markdown
Contributor

rustdoc hitted too, btw.

Or it expected changes? Idk.

@oli-obk

oli-obk commented Jan 21, 2022

Copy link
Copy Markdown
Contributor

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
@fmease fmease added the F-associated_const_equality `#![feature(associated_const_equality)]` label Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-associated_const_equality `#![feature(associated_const_equality)]` 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 T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

Development

Successfully merging this pull request may close these issues.