chore/error: remove from str conversion and add deprecation notificat… #7472

drahnr · 2020-10-30T15:01:19Z

Add explicit error variants for where Error::Msg is used today. Unfortunately this is a bit more tangled so this is just a minor cleanup.

…ions

primitives/blockchain/src/error.rs

ordian

Looks good in general, but it's not clear why we want both Foreign and Msg error types.

client/service/src/client/wasm_override.rs

primitives/blockchain/src/error.rs

drahnr · 2020-11-10T15:58:59Z

Looks good in general, but it's not clear why we want both Foreign and Msg error types.

We eventually don't want Msg anymore, but right now there are a few more tricky cases which are not resolved just yet.

Co-authored-by: Andronik Ordian <[email protected]>

client/service/src/error.rs

ordian · 2020-11-26T16:13:13Z

client/executor/common/src/error.rs

+	#[error(transparent)]
+	RuntimeConstruction(#[from] WasmError),
+
+	#[error("Shared memory is not supported")]
+	SharedMemUnsupported,
+
+	#[error("Imported globals are not supported yet")]
+	ImportedGlobalsUnsupported,
+
+	#[error("initializer expression can have only up to 2 expressions in wasm 1.0")]
+	InitializerHasTooManyExpressions,
+
+	#[error("Invalid initializer expression provided {0}")]
+	InvalidInitializerExpression(String),


We added here a bunch of error variants, but kept the Other(String). Could you elaborate on what is the plan wrt Other(String) error type, is it going to removed eventually? It's not clear when to stop adding more error types.

I'm starting to think we don't need to present the user all the error types, a typical use-case would not match on error variants, but will log them.

My intention is to provide sufficient depth so the chain of errors is sufficiently detailed to do a diagnostic right away.

I have no intention to dig any further any more of the Other rabbit holes at this point.

bkchr

Just some last nitpicks. Otherwise it now looks great, especially with all these "local" errors!
Thank you

bkchr · 2020-11-27T14:02:32Z

client/consensus/slots/src/lib.rs

+
+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
+pub enum Error<T> where T: SlotData + Clone + Debug + Send + Sync + 'static {


Why do we need all this bounds?

Putting bounds on the declaration of a struct should be prevented as much as possible. And I don't see any reason here?

Debug + Send + Sync + 'static are required, otherwise Error<T> will not be Send or Sync. Slot data is there for features. Clone can be removed though.

You only need the bounds on T where you need to have Error Send and Sync

Which is the case a few lines down in https://github.com/paritytech/substrate/pull/7472/files/4b752b06c5ddc7ed0bc0d0735651042f3eb24315#diff-2ff27f67b9a711481f3fe7f514fd50c9f63a2bd2852fd47610dbd1a4c758b061R545

bkchr · 2020-11-27T14:05:42Z

client/executor/src/integration_tests/mod.rs

 #[test_case(WasmExecutionMethod::Interpreted)]
 #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
-#[should_panic(expected = "Allocator ran out of space")]
+#[should_panic]


Nope, not a good idea to do this. This means any exception will be accepted. Please revert

By default the debug message is used, which is now the full chain of errors which is not something we want to match against. I'll see how the functionality can be restored.

@bkchr would it be acceptable that the two error messages differ slightly - native could have a chain, where wasmi could not? Or must they be exact matches?

Can we not just match on parts of the error message?

So remove the should_panic and do unwrap_error().to_string().contains()?

So remove the should_panic and do unwrap_error().to_string().contains()?

That implies a yes, the error types do not have to be the exact equiv :)

Can we not just match on parts of the error message?

Technically if only the first error is formatted, equiv relation should be sufficient..

client/service/src/client/wasm_override.rs

primitives/api/src/lib.rs

primitives/api/test/tests/decl_and_impl.rs

primitives/blockchain/src/error.rs

Co-authored-by: Bastian Köcher <[email protected]>

ordian · 2020-11-27T15:18:57Z

client/executor/common/src/error.rs

+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
 pub enum Error {
 	/// Unserializable Data


remove doc comments here for unification?

I would like to wrap this up and then do another sweep in a separate PR.

ordian · 2020-11-27T15:19:33Z

client/transaction-pool/graph/src/error.rs

+#[derive(Debug, thiserror::Error, derive_more::From)]
+#[allow(missing_docs)]
 pub enum Error {
 	/// Transaction is not verifiable yet, but might be in the future.


ordian · 2020-11-27T18:37:47Z

bot merge

ghost · 2020-11-27T18:37:52Z

Trying merge.

bkchr · 2020-11-28T08:38:35Z

@ordian why did you merge this with open issues?

ordian · 2020-11-28T12:37:18Z

@ordian why did you merge this with open issues?

#7472 (comment)

These issues are not critical and can be addressed separately.

bkchr · 2020-11-28T12:40:51Z

@ordian why did you merge this with open issues?

#7472 (comment)

These issues are not critical and can be addressed separately.

The pr was also not important in any way. If there is stuff that needs to be addressed, it should be done in the particular pr and not be merged eagerly.

bkchr · 2020-11-28T12:43:34Z

I approved the stuff because I expected that these changes are addresses before it gets merged.

ordian · 2020-11-28T12:51:39Z

I agree in general, but this PR has been open for a month now and the burden for resolving merge conflicts grows each day it is open.

drahnr · 2020-11-28T13:24:18Z

Going to address these on Monday, I generally agree that we should resolve them all before merging (especially test related TODOs).

* CI: build docs after test; publish docs after build (paritytech#7591) docs time test/build success on master pub * node-template: add aura to light block import pipeline (paritytech#7595) added aura to block import pipeline * Fix notifications sometimes not being sent (paritytech#7594) * Fix notifications sometimes not being sent * Add comment * Bump rpassword from 4.0.5 to 5.0.0 (paritytech#7597) Bumps [rpassword](https://github.com/conradkleinespel/rpassword) from 4.0.5 to 5.0.0. - [Release notes](https://github.com/conradkleinespel/rpassword/releases) - [Commits](conradkleinespel/rpassword@v4.0.5...v5.0.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remove std feature flags for assert macros (paritytech#7600) * remove std feature flags for assert macros * re-add note about availability in no_std envs * Add small header cache (paritytech#7516) * Remove header query * Header cache * Fix potential race issue * Simplify status query * Inform sync explicitly about new best block (paritytech#7604) * Inform sync explicitly about new best block Instead of "fishing" the new best block out of the processed blocks, we now tell sync directly that there is a new best block. It also makes sure that we update the corresponding sync handshake to the new best block. This is required for parachains as they first import blocks and declare the new best block after being made aware of it by the relay chain. * Adds test * Make sure async stuff had time to run * Bump directories from 2.0.2 to 3.0.1 (paritytech#7609) Bumps [directories](https://github.com/soc/directories-rs) from 2.0.2 to 3.0.1. - [Release notes](https://github.com/soc/directories-rs/releases) - [Commits](https://github.com/soc/directories-rs/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove `RpcMetrics` weirdness (paritytech#7608) * Remove `RpcMetrics` weirdness The metrics was returning an error when prometheus was not given. This was a really weird setup, especially when compared to all other metrics that just do nothing if there is no registry. * Fix browser build * Upgrade to libp2p-0.31. (paritytech#7606) * Upgrade to libp2p-0.31. * Address line width. * Add generous incoming connection limit. * Remove old noise configuration. * Add Key Subcommand to node-template (paritytech#7615) * Forward storage changes in manual seal (paritytech#7614) This prevents nodes from executing the same block 2 times. * chore/error: remove from str conversion and add deprecation notificat… (paritytech#7472) * chore/error: remove from str conversion and add deprecation notifications * fixup changes * fix test looking for gone ::Msg variant * another test fix * one is duplicate, the other is not, so duplicates reported are n-1 * darn spaces Co-authored-by: Andronik Ordian <[email protected]> * remove pointless doc comments of error variants without any value * low hanging fruits (for a tall person) * moar error type variants * avoid the storage modules for now They are in need of a refactor, and the pain is rather large removing all String error and DefaultError occurences. * chore remove pointless error generic * fix test for mocks, add a bunch of non_exhaustive * max line width * test fixes due to error changes * fin * error outputs... again * undo stderr adjustments * Update client/consensus/slots/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * remove closure clutter Co-authored-by: Bastian Köcher <[email protected]> * more error types * introduce ApiError * extract Mock error * ApiError refactor * even more error types * the last for now * chore unused deps * another extraction * reduce should panic, due to extended error messages * error test happiness * shift error lines by one * doc tests * white space Co-authored-by: Bastian Köcher <[email protected]> * Into -> From Co-authored-by: Bastian Köcher <[email protected]> * remove pointless codec Co-authored-by: Bastian Köcher <[email protected]> * avoid pointless self import Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> * network: don't force send block announcements (paritytech#7601) * Change TRACING_SET to static (paritytech#7607) * change TRACING_SET to static * Update primitives/io/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * modify test with nested spans Co-authored-by: Bastian Köcher <[email protected]> * sc-network: Log outgoing notifications too (paritytech#7624) * Log outgoing notifications too * Update client/network/src/protocol/generic_proto/handler.rs Co-authored-by: Max Inden <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> * Bump console_log from 0.1.2 to 0.2.0 (paritytech#7623) Bumps [console_log](https://github.com/iamcodemaker/console_log) from 0.1.2 to 0.2.0. - [Release notes](https://github.com/iamcodemaker/console_log/releases) - [Commits](https://github.com/iamcodemaker/console_log/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * `sudo_as` should return a result (paritytech#7620) * Fix wrong value put for pending_opening (paritytech#7633) * Fix wrong value put for pending_opening * Oops, didn't even try compiling it * Rename pallet trait `Trait` to `Config` (paritytech#7599) * rename Trait to Config * add test asserting using Trait is still valid. * fix ui tests * resolve unresolved error nits of paritytech#7617 (paritytech#7631) * handle executor should_panic test better * Revert "reduce should panic, due to extended error messages" This reverts commit c080594. * remove excessive constraints * remove duplicate documentation messages for error variants * reduce T: constraints to the abs minimum * whoops * fewer bounds again Co-authored-by: Bernhard Schuster <[email protected]> * Fix bad state transition with DisabledPendingEnable+OpenDesiredByRemote (paritytech#7638) * Renames of `Trait` to `Config` in README.md, weight templates and few minor ones (paritytech#7636) * manual rename * renamse in README.md * fix template * Fix CI Link Check (paritytech#7639) * fix trigger fingers * more * Update frame/example-offchain-worker/README.md Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> * Fix cargo clippy warning in peerset. (paritytech#7641) * Fix cargo clippy warning in peerset. * Update client/peerset/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * Apply suggestions from code review Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Denis Pisarev <[email protected]> Co-authored-by: André Silva <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Alexander Popiak <[email protected]> Co-authored-by: Arkadiy Paronyan <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Roman Borschel <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Andrew Plaza <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: jolestar <[email protected]>

paritytech#7472) * chore/error: remove from str conversion and add deprecation notifications * fixup changes * fix test looking for gone ::Msg variant * another test fix * one is duplicate, the other is not, so duplicates reported are n-1 * darn spaces Co-authored-by: Andronik Ordian <[email protected]> * remove pointless doc comments of error variants without any value * low hanging fruits (for a tall person) * moar error type variants * avoid the storage modules for now They are in need of a refactor, and the pain is rather large removing all String error and DefaultError occurences. * chore remove pointless error generic * fix test for mocks, add a bunch of non_exhaustive * max line width * test fixes due to error changes * fin * error outputs... again * undo stderr adjustments * Update client/consensus/slots/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * remove closure clutter Co-authored-by: Bastian Köcher <[email protected]> * more error types * introduce ApiError * extract Mock error * ApiError refactor * even more error types * the last for now * chore unused deps * another extraction * reduce should panic, due to extended error messages * error test happiness * shift error lines by one * doc tests * white space Co-authored-by: Bastian Köcher <[email protected]> * Into -> From Co-authored-by: Bastian Köcher <[email protected]> * remove pointless codec Co-authored-by: Bastian Köcher <[email protected]> * avoid pointless self import Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

drahnr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 30, 2020

drahnr self-assigned this Oct 30, 2020

chore/error: remove from str conversion and add deprecation notificat…

8467f4a

…ions

drahnr force-pushed the bernhard-error-chore branch from 8605465 to 8467f4a Compare October 30, 2020 17:22

bkchr suggested changes Oct 31, 2020

View reviewed changes

primitives/blockchain/src/error.rs Outdated Show resolved Hide resolved

drahnr requested a review from bkchr November 3, 2020 12:10

drahnr force-pushed the bernhard-error-chore branch from b02cbac to 53d16ea Compare November 3, 2020 12:11

fixup changes

91744cd

drahnr force-pushed the bernhard-error-chore branch from 53d16ea to 91744cd Compare November 3, 2020 13:45

fix test looking for gone ::Msg variant

c1085eb

drahnr force-pushed the bernhard-error-chore branch from e96ee69 to c1085eb Compare November 3, 2020 15:37

Bernhard Schuster added 2 commits November 4, 2020 11:03

another test fix

524ee71

one is duplicate, the other is not, so duplicates reported are n-1

2a09d3c

drahnr requested a review from ordian November 10, 2020 15:01

ordian reviewed Nov 10, 2020

View reviewed changes

drahnr and others added 2 commits November 10, 2020 17:03

darn spaces

dbbd20f

Co-authored-by: Andronik Ordian <[email protected]>

remove pointless doc comments of error variants without any value

8d021f0

drahnr requested a review from NikVolf as a code owner November 10, 2020 19:33

Merge remote-tracking branch 'origin/master' into bernhard-error-chore

311a58f

drahnr requested a review from ordian November 11, 2020 09:55

ordian previously approved these changes Nov 11, 2020

View reviewed changes

client/service/src/error.rs Show resolved Hide resolved

drahnr added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Nov 11, 2020

paritytech deleted a comment Nov 11, 2020

drahnr requested review from coriolinus, expenses and insipx November 12, 2020 10:20

chore unused deps

1d364ac

ordian reviewed Nov 26, 2020

View reviewed changes

Bernhard Schuster added 4 commits November 26, 2020 17:31

another extraction

56a0b2d

reduce should panic, due to extended error messages

c080594

error test happiness

077301e

shift error lines by one

0983c88

drahnr force-pushed the bernhard-error-chore branch from 8eec318 to 0983c88 Compare November 27, 2020 06:48

Bernhard Schuster added 2 commits November 27, 2020 11:34

doc tests

0e9d696

Merge remote-tracking branch 'origin/master' into bernhard-error-chore

4b752b0

drahnr requested a review from bkchr November 27, 2020 13:54

bkchr approved these changes Nov 27, 2020

View reviewed changes

drahnr and others added 4 commits November 27, 2020 16:06

white space

35d2814

Co-authored-by: Bastian Köcher <[email protected]>

Into -> From

13810eb

Co-authored-by: Bastian Köcher <[email protected]>

remove pointless codec

6df60d0

Co-authored-by: Bastian Köcher <[email protected]>

avoid pointless self import

d0c0d24

Co-authored-by: Bastian Köcher <[email protected]>

ordian approved these changes Nov 27, 2020

View reviewed changes

ghost merged commit d428fc7 into master Nov 27, 2020

ghost deleted the bernhard-error-chore branch November 27, 2020 18:37

drahnr mentioned this pull request Nov 28, 2020

Resolve error nits of PR #7472 #7617

Closed

pepyakin mentioned this pull request Dec 1, 2020

Update polkadot to the latest master paritytech/cumulus#251

Merged

This pull request was closed.

chore/error: remove from str conversion and add deprecation notificat… #7472

chore/error: remove from str conversion and add deprecation notificat… #7472

Uh oh!

Conversation

drahnr commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drahnr commented Nov 10, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ordian commented Nov 27, 2020

Uh oh!

ghost commented Nov 27, 2020

Uh oh!

bkchr commented Nov 28, 2020

Uh oh!

ordian commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Nov 28, 2020

Uh oh!

bkchr commented Nov 28, 2020

Uh oh!

ordian commented Nov 28, 2020

Uh oh!

drahnr commented Nov 28, 2020

Uh oh!

drahnr commented Oct 30, 2020 •

edited

Loading

drahnr Nov 27, 2020 •

edited

Loading

ordian commented Nov 28, 2020 •

edited

Loading