Skip to content

Disallow duplicate indexes using constant evaluation#653

Merged
bkchr merged 3 commits intomasterfrom
pkhry/duplicate-indexes-const-eval
Jan 25, 2025
Merged

Disallow duplicate indexes using constant evaluation#653
bkchr merged 3 commits intomasterfrom
pkhry/duplicate-indexes-const-eval

Conversation

@pkhry
Copy link
Copy Markdown
Contributor

@pkhry pkhry commented Nov 8, 2024

Description

following a suggestion from here #628 (comment)

Notes

It seems that it's not possible to have a good error message this way, as format! in cant be used in constant_blocks, so string interpolation is not possible unless we pick up const_format and re-export it from the library.

@pkhry pkhry requested a review from gui1117 November 8, 2024 15:05
@pkhry pkhry closed this Jan 17, 2025
@gui1117 @bkchr
Improve error message (#681)
65fae2f 
* improve error message

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>

* finally understanding rust const environment

* fmt

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
@gui1117 gui1117 reopened this Jan 25, 2025
@gui1117
Copy link
Copy Markdown
Contributor

gui1117 commented Jan 25, 2025

I was basing my PR on top of yours, so I re-opened it.

@bkchr if you like it we can merge it

@bkchr bkchr merged commit 8013d7d into master Jan 25, 2025
This was referenced Jan 27, 2025
Closed
Closed
Closed
Closed
@gui1117 gui1117 mentioned this pull request Feb 1, 2025
Merged
saiintbrisson added a commit to saiintbrisson/scale-info that referenced this pull request Feb 14, 2025
@saiintbrisson
Update tests.rs
f71cab5 
paritytech/parity-scale-codec/pull/653 disallowed duplicate indexes for variants.
saiintbrisson added a commit to saiintbrisson/scale-info that referenced this pull request Feb 14, 2025
@saiintbrisson
Update tests.rs
d6921ee 
paritytech/parity-scale-codec/pull/653 disallowed duplicate indexes for variants.
@saiintbrisson saiintbrisson mentioned this pull request Feb 14, 2025
Merged
niklasad1 pushed a commit to paritytech/scale-info that referenced this pull request Feb 19, 2025
@saiintbrisson
chore: mark TypeInfo derived impl as automatically_derived (#218)
7629a7f 
* chore: mark TypeInfo derived impl as automatically_derived

As per rust-lang/rust#120185, functions with their enclosing impl body marked as `#[automatically_derived]` won't be featured in coverage reports.

* chore: elide lifetimes

* Update tests.rs

paritytech/parity-scale-codec/pull/653 disallowed duplicate indexes for variants.

* fix: ui tests were outdated
@ggwpez
Copy link
Copy Markdown
Member

ggwpez commented Feb 21, 2025 •
edited
Loading

This was actually a major change that we released as a patch... it broke the docs build here https://docs.rs/crate/polkadot-sdk/0.12.1/builds/1788762
cc @gui1117 @nkpar

Also there was no CHANGELOG entry here?

@ggwpez
Copy link
Copy Markdown
Member

ggwpez commented Feb 21, 2025

Since this is arguably a breaking fix, do you think patch is still fine?

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Feb 21, 2025

Since this is arguably a breaking fix, do you think patch is still fine?

Yes, before this invalid code was emitted.

@gui1117
Copy link
Copy Markdown
Contributor

gui1117 commented Feb 22, 2025 •
edited
Loading

A fix should have been pushed for the crates in 2412, 2409 and 2407, maybe polkadot-sdk 0.12.1 is an earlier version? or some crates have been missed.

Because we released and yank versions, sorry for that, the automatic changelog missed, I manually added it.

EDIT: I think pallet-revive-eth-rpc is using a non-supported version of sc-network.
paritytech/polkadot-sdk#7437 (comment)

It is surprising, in polkadot-sdk 0.12.1, pallet-revive-eth-rpc is using a different sc-cli than in the workspace, they are using sc-cli v0.49.0 instead of sc-cli v0.50.0 that is used by all other crates.

sc-cli v0.49.0 is using sc-network v0.47.0 and this version has not been patched, apparently

We can patch sc-network v0.47.0 but I think the issue may actually be different: why is pallet-revive-eth-rpc using a different version for sc-cli ? Something seems wrong during the publish process

@serban300
Copy link
Copy Markdown
Contributor

serban300 commented Feb 22, 2025

Since there hasn't been any error report from any customer could we just fix the docs and keep this release ?

Sorry, I thought this change wasn't major since it doesn't change any interface or structure. It just adds a check for duplicate indexes, which should prevent misusing the library.

@bkchr bkchr deleted the pkhry/duplicate-indexes-const-eval branch February 22, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkchr bkchr bkchr approved these changes

@gui1117 gui1117 gui1117 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pkhry @gui1117 @ggwpez @bkchr @serban300